[PATCH 0/2] arm64: KVM: Fixes for 4.4-rc2

[PATCH 0/2] arm64: KVM: Fixes for 4.4-rc2

[Marc Zyngier]

Here's a couple of fixes for KVM/arm64:

- The first one addresses a misinterpretation of the architecture
  spec, leading to the mishandling of I/O accesses generated from an
  AArch32 guest using banked registers.

- The second one is a workaround for a Cortex-A57 erratum.

Both patches are based on v4.4-rc1.

Marc Zyngier (2):
  arm64: KVM: Fix AArch32 to AArch64 register mapping
  arm64: KVM: Add workaround for Cortex-A57 erratum 834220

 arch/arm64/Kconfig                   | 21 +++++++++++++++++++++
 arch/arm64/include/asm/cpufeature.h  |  3 ++-
 arch/arm64/include/asm/kvm_emulate.h |  8 +++++---
 arch/arm64/kernel/cpu_errata.c       |  9 +++++++++
 arch/arm64/kvm/hyp.S                 |  6 ++++++
 arch/arm64/kvm/inject_fault.c        |  2 +-
 6 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 1/2] arm64: KVM: Fix AArch32 to AArch64 register mapping

[Marc Zyngier]

When running a 32bit guest under a 64bit hypervisor, the ARMv8
architecture defines a mapping of the 32bit registers in the 64bit
space. This includes banked registers that are being demultiplexed
over the 64bit ones.

On exception caused by an operation involving a 32bit register, the
HW exposes the register number in the ESR_EL2 register. It was so
far understood that SW had to compute which register was AArch64
register was used (based on the current AArch32 mode and register
number).

It turns out that I misinterpreted the ARM ARM, and the clue is in
D1.20.1: "For some exceptions, the exception syndrome given in the
ESR_ELx identifies one or more register numbers from the issued
instruction that generated the exception. Where the exception is
taken from an Exception level using AArch32 these register numbers
give the AArch64 view of the register."

Which means that the HW is already giving us the translated version,
and that we shouldn't try to interpret it at all (for example, doing
an MMIO operation from the IRQ mode using the LR register leads to
very unexpected behaviours).

The fix is thus not to perform a call to vcpu_reg32() at all from
vcpu_reg(), and use whatever register number is supplied directly.
The only case we need to find out about the mapping is when we
actively generate a register access, which only occurs when injecting
a fault in a guest.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 8 +++++---
 arch/arm64/kvm/inject_fault.c        | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 17e92f0..3ca894e 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
 	*vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT;
 }
 
+/*
+ * vcpu_reg should always be passed a register number coming from a
+ * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32
+ * with banked registers.
+ */
 static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
 {
-	if (vcpu_mode_is_32bit(vcpu))
-		return vcpu_reg32(vcpu, reg_num);
-
 	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
 }
 
diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index 85c5715..648112e 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
 
 	/* Note: These now point to the banked copies */
 	*vcpu_spsr(vcpu) = new_spsr_value;
-	*vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
+	*vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
 
 	/* Branch to exception vector */
 	if (sctlr & (1 << 13))
-- 
2.1.4

[PATCH 2/2] arm64: KVM: Add workaround for Cortex-A57 erratum 834220

[Marc Zyngier]

Cortex-A57 parts up to r1p2 can misreport Stage 2 translation faults
when a Stage 1 permission fault or device alignment fault should
have been reported.

This patch implements the workaround (which is to validate that the
Stage-1 translation actually succeeds) by using code patching.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/Kconfig                  | 21 +++++++++++++++++++++
 arch/arm64/include/asm/cpufeature.h |  3 ++-
 arch/arm64/kernel/cpu_errata.c      |  9 +++++++++
 arch/arm64/kvm/hyp.S                |  6 ++++++
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9ac16a4..746d985 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -316,6 +316,27 @@ config ARM64_ERRATUM_832075
 
 	  If unsure, say Y.
 
+config ARM64_ERRATUM_834220
+	bool "Cortex-A57: 834220: Stage 2 translation fault might be incorrectly reported in presence of a Stage 1 fault"
+	depends on KVM
+	default y
+	help
+	  This option adds an alternative code sequence to work around ARM
+	  erratum 834220 on Cortex-A57 parts up to r1p2.
+
+	  Affected Cortex-A57 parts might report a Stage 2 translation
+	  fault as a the result of a Stage 1 fault for load crossing a
+	  page boundary when there is a permission or device memory
+	  alignment fault at Stage 1 and a translation fault at Stage 2.
+
+	  The workaround is to verify that the Stage-1 translation
+	  doesn't generate a fault before handling the Stage-2 fault.
+	  Please note that this does not necessarily enable the workaround,
+	  as it depends on the alternative framework, which will only patch
+	  the kernel if an affected CPU is detected.
+
+	  If unsure, say Y.
+
 config ARM64_ERRATUM_845719
 	bool "Cortex-A53: 845719: a load might read incorrect data"
 	depends on COMPAT
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 11d5bb0f..52722ee 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -29,8 +29,9 @@
 #define ARM64_HAS_PAN				4
 #define ARM64_HAS_LSE_ATOMICS			5
 #define ARM64_WORKAROUND_CAVIUM_23154		6
+#define ARM64_WORKAROUND_834220			7
 
-#define ARM64_NCAPS				7
+#define ARM64_NCAPS				8
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 24926f2..feb6b4e 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -75,6 +75,15 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 			   (1 << MIDR_VARIANT_SHIFT) | 2),
 	},
 #endif
+#ifdef CONFIG_ARM64_ERRATUM_834220
+	{
+	/* Cortex-A57 r0p0 - r1p2 */
+		.desc = "ARM erratum 834220",
+		.capability = ARM64_WORKAROUND_834220,
+		MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
+			   (1 << MIDR_VARIANT_SHIFT) | 2),
+	},
+#endif
 #ifdef CONFIG_ARM64_ERRATUM_845719
 	{
 	/* Cortex-A53 r0p[01234] */
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 1599701..ff2e038 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -1015,9 +1015,15 @@ el1_trap:
 	b.ne	1f		// Not an abort we care about
 
 	/* This is an abort. Check for permission fault */
+alternative_if_not ARM64_WORKAROUND_834220
 	and	x2, x1, #ESR_ELx_FSC_TYPE
 	cmp	x2, #FSC_PERM
 	b.ne	1f		// Not a permission fault
+alternative_else
+	nop			// Use the permission fault path to
+	nop			// check for a valid S1 translation,
+	nop			// regardless of the ESR value.
+alternative_endif
 
 	/*
 	 * Check for Stage-1 page table walk, which is guaranteed
-- 
2.1.4

[PATCH 1/2] arm64: KVM: Fix AArch32 to AArch64 register mapping

[Robin Murphy]

Hi Marc,

On 16/11/15 10:28, Marc Zyngier wrote:
> When running a 32bit guest under a 64bit hypervisor, the ARMv8
> architecture defines a mapping of the 32bit registers in the 64bit
> space. This includes banked registers that are being demultiplexed
> over the 64bit ones.
>
> On exception caused by an operation involving a 32bit register, the
> HW exposes the register number in the ESR_EL2 register. It was so
> far understood that SW had to compute which register was AArch64
> register was used (based on the current AArch32 mode and register
> number).
>
> It turns out that I misinterpreted the ARM ARM, and the clue is in
> D1.20.1: "For some exceptions, the exception syndrome given in the
> ESR_ELx identifies one or more register numbers from the issued
> instruction that generated the exception. Where the exception is
> taken from an Exception level using AArch32 these register numbers
> give the AArch64 view of the register."
>
> Which means that the HW is already giving us the translated version,
> and that we shouldn't try to interpret it at all (for example, doing
> an MMIO operation from the IRQ mode using the LR register leads to
> very unexpected behaviours).
>
> The fix is thus not to perform a call to vcpu_reg32() at all from
> vcpu_reg(), and use whatever register number is supplied directly.
> The only case we need to find out about the mapping is when we
> actively generate a register access, which only occurs when injecting
> a fault in a guest.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm64/include/asm/kvm_emulate.h | 8 +++++---
>   arch/arm64/kvm/inject_fault.c        | 2 +-
>   2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 17e92f0..3ca894e 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -99,11 +99,13 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>   	*vcpu_cpsr(vcpu) |= COMPAT_PSR_T_BIT;
>   }
>
> +/*
> + * vcpu_reg should always be passed a register number coming from a
> + * read of ESR_EL2. Otherwise, it may give the wrong result on AArch32
> + * with banked registers.
> + */
>   static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)
>   {
> -	if (vcpu_mode_is_32bit(vcpu))
> -		return vcpu_reg32(vcpu, reg_num);
> -
>   	return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.regs[reg_num];
>   }
>
> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> index 85c5715..648112e 100644
> --- a/arch/arm64/kvm/inject_fault.c
> +++ b/arch/arm64/kvm/inject_fault.c
> @@ -48,7 +48,7 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
>
>   	/* Note: These now point to the banked copies */
>   	*vcpu_spsr(vcpu) = new_spsr_value;
> -	*vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
> +	*vcpu_reg32(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;

To the best of my knowledge after picking through all the uses of 
vcpu_reg, particularly in the shared 32-bit code, this does seem to be 
the only one which involves a potentially-banked register number that 
didn't originally come from an ESR read, and thus needs translation.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

(unfortunately I don't have an actual test-case as it was already a 
third-hand report when I started trying to look into it).

Thanks for picking this up,
Robin.

>
>   	/* Branch to exception vector */
>   	if (sctlr & (1 << 13))
>

[PATCH 2/2] arm64: KVM: Add workaround for Cortex-A57 erratum 834220

[Will Deacon]

Hi Marc,

On Mon, Nov 16, 2015 at 10:28:18AM +0000, Marc Zyngier wrote:
> Cortex-A57 parts up to r1p2 can misreport Stage 2 translation faults
> when a Stage 1 permission fault or device alignment fault should
> have been reported.
> 
> This patch implements the workaround (which is to validate that the
> Stage-1 translation actually succeeds) by using code patching.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/Kconfig                  | 21 +++++++++++++++++++++
>  arch/arm64/include/asm/cpufeature.h |  3 ++-
>  arch/arm64/kernel/cpu_errata.c      |  9 +++++++++
>  arch/arm64/kvm/hyp.S                |  6 ++++++
>  4 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 9ac16a4..746d985 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -316,6 +316,27 @@ config ARM64_ERRATUM_832075
>  
>  	  If unsure, say Y.
>  
> +config ARM64_ERRATUM_834220
> +	bool "Cortex-A57: 834220: Stage 2 translation fault might be incorrectly reported in presence of a Stage 1 fault"
> +	depends on KVM
> +	default y
> +	help
> +	  This option adds an alternative code sequence to work around ARM
> +	  erratum 834220 on Cortex-A57 parts up to r1p2.
> +
> +	  Affected Cortex-A57 parts might report a Stage 2 translation
> +	  fault as a the result of a Stage 1 fault for load crossing a

s/as a the/as the/
s/for load/for a load/

> +	  page boundary when there is a permission or device memory
> +	  alignment fault at Stage 1 and a translation fault at Stage 2.
> +
> +	  The workaround is to verify that the Stage-1 translation

Consistency between "Stage 1" and "Stage-1".

> +	  doesn't generate a fault before handling the Stage-2 fault.

Same here.

> +	  Please note that this does not necessarily enable the workaround,
> +	  as it depends on the alternative framework, which will only patch
> +	  the kernel if an affected CPU is detected.
> +
> +	  If unsure, say Y.
> +
>  config ARM64_ERRATUM_845719
>  	bool "Cortex-A53: 845719: a load might read incorrect data"
>  	depends on COMPAT
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 11d5bb0f..52722ee 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -29,8 +29,9 @@
>  #define ARM64_HAS_PAN				4
>  #define ARM64_HAS_LSE_ATOMICS			5
>  #define ARM64_WORKAROUND_CAVIUM_23154		6
> +#define ARM64_WORKAROUND_834220			7
>  
> -#define ARM64_NCAPS				7
> +#define ARM64_NCAPS				8
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 24926f2..feb6b4e 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -75,6 +75,15 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
>  			   (1 << MIDR_VARIANT_SHIFT) | 2),
>  	},
>  #endif
> +#ifdef CONFIG_ARM64_ERRATUM_834220
> +	{
> +	/* Cortex-A57 r0p0 - r1p2 */
> +		.desc = "ARM erratum 834220",
> +		.capability = ARM64_WORKAROUND_834220,
> +		MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
> +			   (1 << MIDR_VARIANT_SHIFT) | 2),
> +	},
> +#endif
>  #ifdef CONFIG_ARM64_ERRATUM_845719
>  	{
>  	/* Cortex-A53 r0p[01234] */
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 1599701..ff2e038 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -1015,9 +1015,15 @@ el1_trap:
>  	b.ne	1f		// Not an abort we care about
>  
>  	/* This is an abort. Check for permission fault */
> +alternative_if_not ARM64_WORKAROUND_834220
>  	and	x2, x1, #ESR_ELx_FSC_TYPE
>  	cmp	x2, #FSC_PERM
>  	b.ne	1f		// Not a permission fault
> +alternative_else
> +	nop			// Use the permission fault path to
> +	nop			// check for a valid S1 translation,
> +	nop			// regardless of the ESR value.
> +alternative_endif

With the cosmetic changes:

  Reviewed-by: Will Deacon <will.deacon@arm.com>

Can you cc stable as well, please?

Will

[PATCH 0/2] arm64: KVM: Fixes for 4.4-rc2

[Christoffer Dall]

On Mon, Nov 16, 2015 at 10:28:16AM +0000, Marc Zyngier wrote:
> Here's a couple of fixes for KVM/arm64:
> 
> - The first one addresses a misinterpretation of the architecture
>   spec, leading to the mishandling of I/O accesses generated from an
>   AArch32 guest using banked registers.
> 
> - The second one is a workaround for a Cortex-A57 erratum.
> 
Thanks, applied with cosmetic fixes and cc'ed to stable.

-Christoffer