[PATCH 0/3] KVM: arm64: Fixes for resolving the fault IPA

[PATCH 0/3] KVM: arm64: Fixes for resolving the fault IPA

[Oliver Upton]

KVM's heuristics for determining the fault IPA are a bit shaky and don't
necessarily align with the letter of the architecture. On top of that,
HPFAR_EL2 is UNKNOWN if an SEA occurred during a table walk. Re-walking
the page tables will replay the SEA at EL2, becoming a panic in the hyp.

Oliver Upton (3):
  KVM: arm64: Only read HPFAR_EL2 when value is architecturally valid
  arm64: Convert HPFAR_EL2 to sysreg table
  KVM: arm64: Don't translate FAR if invalid/unsafe

 arch/arm64/include/asm/esr.h           | 23 +++++++++
 arch/arm64/include/asm/kvm_emulate.h   |  7 ++-
 arch/arm64/include/asm/kvm_ras.h       |  2 +-
 arch/arm64/kvm/hyp/include/hyp/fault.h | 70 ++++++++++++++++++--------
 arch/arm64/kvm/hyp/nvhe/mem_protect.c  |  2 +-
 arch/arm64/kvm/mmu.c                   | 31 +++++++-----
 arch/arm64/tools/sysreg                |  7 +++
 7 files changed, 105 insertions(+), 37 deletions(-)


base-commit: 369c0122682c4468a69f2454614eded71c5348f3
-- 
2.39.5

[PATCH 1/3] KVM: arm64: Only read HPFAR_EL2 when value is architecturally valid

[Oliver Upton]

KVM's logic for deciding when HPFAR_EL2 is UNKNOWN doesn't align with
the architecture. Most notably, KVM assumes HPFAR_EL2 contains the
faulting IPA even in the case of an SEA.

Align the logic with the architecture rather than attempting to
paraphrase it. Additionally, take the opportunity to improve the
language around ARM erratum #834220 such that it actually describes the
bug.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/esr.h           |  1 +
 arch/arm64/kvm/hyp/include/hyp/fault.h | 46 ++++++++++++++++----------
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index d1b1a33f9a8b..7b096ed87360 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -121,6 +121,7 @@
 #define ESR_ELx_FSC_SEA_TTW(n)	(0x14 + (n))
 #define ESR_ELx_FSC_SECC	(0x18)
 #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
+#define ESR_ELx_FSC_ADDRESS	(0x00)
 
 /* Status codes for individual page table levels */
 #define ESR_ELx_FSC_ACCESS_L(n)	(ESR_ELx_FSC_ACCESS + (n))
diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
index 17df94570f03..84d165e17bd0 100644
--- a/arch/arm64/kvm/hyp/include/hyp/fault.h
+++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
@@ -44,31 +44,41 @@ static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
 	return true;
 }
 
+/*
+ * Checks for the conditions when HPFAR_EL2 is written, per DDI0487L.a D24.2.70.
+ */
+static inline bool __hpfar_valid(u64 esr)
+{
+	/*
+	 * CPUs affected by ARM erratum #834220 may incorrectly report a
+	 * stage-2 translation fault when a stage-1 permission fault occurs.
+	 *
+	 * Re-walk the page tables to determine if a stage-1 fault actually
+	 * occurred.
+	 */
+	if (cpus_have_final_cap(ARM64_WORKAROUND_834220) &&
+	    esr_fsc_is_translation_fault(esr))
+		return false;
+
+	if (esr_fsc_is_translation_fault(esr) || esr_fsc_is_access_flag_fault(esr))
+		return true;
+
+	if ((esr & ESR_ELx_S1PTW) && esr_fsc_is_permission_fault(esr))
+		return true;
+
+	return (esr & ESR_ELx_FSC) == ESR_ELx_FSC_ADDRESS;
+}
+
 static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
 {
 	u64 hpfar, far;
 
 	far = read_sysreg_el2(SYS_FAR);
 
-	/*
-	 * The HPFAR can be invalid if the stage 2 fault did not
-	 * happen during a stage 1 page table walk (the ESR_EL2.S1PTW
-	 * bit is clear) and one of the two following cases are true:
-	 *   1. The fault was due to a permission fault
-	 *   2. The processor carries errata 834220
-	 *
-	 * Therefore, for all non S1PTW faults where we either have a
-	 * permission fault or the errata workaround is enabled, we
-	 * resolve the IPA using the AT instruction.
-	 */
-	if (!(esr & ESR_ELx_S1PTW) &&
-	    (cpus_have_final_cap(ARM64_WORKAROUND_834220) ||
-	     esr_fsc_is_permission_fault(esr))) {
-		if (!__translate_far_to_hpfar(far, &hpfar))
-			return false;
-	} else {
+	if (__hpfar_valid(esr))
 		hpfar = read_sysreg(hpfar_el2);
-	}
+	else if (!__translate_far_to_hpfar(far, &hpfar))
+		return false;
 
 	fault->far_el2 = far;
 	fault->hpfar_el2 = hpfar;
-- 
2.39.5

[PATCH 2/3] arm64: Convert HPFAR_EL2 to sysreg table

[Oliver Upton]

Switch over to the typical sysreg table for HPFAR_EL2 as we're about to
start using more fields in the register.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_emulate.h  | 2 +-
 arch/arm64/kvm/hyp/nvhe/mem_protect.c | 2 +-
 arch/arm64/tools/sysreg               | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d7cf66573aca..5d8c0832acbe 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -305,7 +305,7 @@ static __always_inline unsigned long kvm_vcpu_get_hfar(const struct kvm_vcpu *vc
 
 static __always_inline phys_addr_t kvm_vcpu_get_fault_ipa(const struct kvm_vcpu *vcpu)
 {
-	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_MASK) << 8;
+	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
 }
 
 static inline u64 kvm_vcpu_get_disr(const struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index f34f11c720d7..168893570703 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -578,7 +578,7 @@ void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)
 		return;
 	}
 
-	addr = (fault.hpfar_el2 & HPFAR_MASK) << 8;
+	addr = (fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
 	ret = host_stage2_idmap(addr);
 	BUG_ON(ret && ret != -EAGAIN);
 }
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 2c63662c1a48..31ad9ce2b91c 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -3433,3 +3433,10 @@ Field	5	F
 Field	4	P
 Field	3:0	Align
 EndSysreg
+
+Sysreg	HPFAR_EL2	3	4	6	0	4
+Field	63	NS
+Res0	62:48
+Field	47:4	FIPA
+Res0	3:0
+EndSysreg
-- 
2.39.5

[PATCH 3/3] KVM: arm64: Don't translate FAR if invalid/unsafe

[Oliver Upton]

Don't re-walk the page tables if an SEA occurred during the faulting
page table walk to avoid taking a fatal exception in the hyp.
Additionally, check that FAR_EL2 is valid for SEAs not taken on PTW
as the architecture doesn't guarantee it contains the fault VA.

Finally, fix up the rest of the abort path by checking for SEAs early
and bugging the VM if we get further along with an UNKNOWN fault IPA.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/esr.h           | 22 ++++++++++++++++++
 arch/arm64/include/asm/kvm_emulate.h   |  7 +++++-
 arch/arm64/include/asm/kvm_ras.h       |  2 +-
 arch/arm64/kvm/hyp/include/hyp/fault.h | 26 ++++++++++++++++-----
 arch/arm64/kvm/mmu.c                   | 31 ++++++++++++++++----------
 5 files changed, 69 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7b096ed87360..4b874ad95f2e 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -465,6 +465,28 @@ static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
 	       (esr == ESR_ELx_FSC_ACCESS_L(0));
 }
 
+static inline bool esr_fsc_is_sea_ttw(unsigned long esr)
+{
+	esr = esr & ESR_ELx_FSC;
+
+	return (esr == ESR_ELx_FSC_SEA_TTW(3)) ||
+	       (esr == ESR_ELx_FSC_SEA_TTW(2)) ||
+	       (esr == ESR_ELx_FSC_SEA_TTW(1)) ||
+	       (esr == ESR_ELx_FSC_SEA_TTW(0)) ||
+	       (esr == ESR_ELx_FSC_SEA_TTW(-1));
+}
+
+static inline bool esr_fsc_is_secc_ttw(unsigned long esr)
+{
+	esr = esr & ESR_ELx_FSC;
+
+	return (esr == ESR_ELx_FSC_SECC_TTW(3)) ||
+	       (esr == ESR_ELx_FSC_SECC_TTW(2)) ||
+	       (esr == ESR_ELx_FSC_SECC_TTW(1)) ||
+	       (esr == ESR_ELx_FSC_SECC_TTW(0)) ||
+	       (esr == ESR_ELx_FSC_SECC_TTW(-1));
+}
+
 /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
 static inline bool esr_iss_is_eretax(unsigned long esr)
 {
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 5d8c0832acbe..775e200a9be4 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -305,7 +305,12 @@ static __always_inline unsigned long kvm_vcpu_get_hfar(const struct kvm_vcpu *vc
 
 static __always_inline phys_addr_t kvm_vcpu_get_fault_ipa(const struct kvm_vcpu *vcpu)
 {
-	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
+	u64 hpfar = vcpu->arch.fault.hpfar_el2;
+
+	if (unlikely(!(hpfar & HPFAR_EL2_NS)))
+		return INVALID_GPA;
+
+	return ((phys_addr_t)hpfar & HPFAR_EL2_FIPA) << 8;
 }
 
 static inline u64 kvm_vcpu_get_disr(const struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/include/asm/kvm_ras.h b/arch/arm64/include/asm/kvm_ras.h
index 87e10d9a635b..9398ade632aa 100644
--- a/arch/arm64/include/asm/kvm_ras.h
+++ b/arch/arm64/include/asm/kvm_ras.h
@@ -14,7 +14,7 @@
  * Was this synchronous external abort a RAS notification?
  * Returns '0' for errors handled by some RAS subsystem, or -ENOENT.
  */
-static inline int kvm_handle_guest_sea(phys_addr_t addr, u64 esr)
+static inline int kvm_handle_guest_sea(void)
 {
 	/* apei_claim_sea(NULL) expects to mask interrupts itself */
 	lockdep_assert_irqs_enabled();
diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
index 84d165e17bd0..1ac9b23a382a 100644
--- a/arch/arm64/kvm/hyp/include/hyp/fault.h
+++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
@@ -12,6 +12,16 @@
 #include <asm/kvm_hyp.h>
 #include <asm/kvm_mmu.h>
 
+static inline bool __fault_safe_to_translate(u64 esr)
+{
+	u64 fsc = esr & ESR_ELx_FSC;
+
+	if (esr_fsc_is_sea_ttw(esr) || esr_fsc_is_secc_ttw(esr))
+		return false;
+
+	return !(fsc == ESR_ELx_FSC_EXTABT && (esr & ESR_ELx_FnV));
+}
+
 static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
 {
 	int ret;
@@ -71,17 +81,23 @@ static inline bool __hpfar_valid(u64 esr)
 
 static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
 {
-	u64 hpfar, far;
+	u64 hpfar;
 
-	far = read_sysreg_el2(SYS_FAR);
+	fault->far_el2		= read_sysreg_el2(SYS_FAR);
+	fault->hpfar_el2	= 0;
 
 	if (__hpfar_valid(esr))
 		hpfar = read_sysreg(hpfar_el2);
-	else if (!__translate_far_to_hpfar(far, &hpfar))
+	else if (unlikely(!__fault_safe_to_translate(esr)))
+		return true;
+	else if (!__translate_far_to_hpfar(fault->far_el2, &hpfar))
 		return false;
 
-	fault->far_el2 = far;
-	fault->hpfar_el2 = hpfar;
+	/*
+	 * Hijack HPFAR_EL2.NS (RES0 in Non-secure) to indicate a valid
+	 * HPFAR value.
+	 */
+	fault->hpfar_el2 = hpfar | HPFAR_EL2_NS;
 	return true;
 }
 
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 2feb6c6b63af..754f2fe0cc67 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1794,9 +1794,28 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 	gfn_t gfn;
 	int ret, idx;
 
+	/* Synchronous External Abort? */
+	if (kvm_vcpu_abt_issea(vcpu)) {
+		/*
+		 * For RAS the host kernel may handle this abort.
+		 * There is no need to pass the error into the guest.
+		 */
+		if (kvm_handle_guest_sea())
+			kvm_inject_vabt(vcpu);
+
+		return 1;
+	}
+
 	esr = kvm_vcpu_get_esr(vcpu);
 
+	/*
+	 * The fault IPA should be reliable at this point as we're not dealing
+	 * with an SEA.
+	 */
 	ipa = fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
+	if (KVM_BUG_ON(ipa == INVALID_GPA, vcpu->kvm))
+		return -EFAULT;
+
 	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
 
 	if (esr_fsc_is_translation_fault(esr)) {
@@ -1818,18 +1837,6 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
 		}
 	}
 
-	/* Synchronous External Abort? */
-	if (kvm_vcpu_abt_issea(vcpu)) {
-		/*
-		 * For RAS the host kernel may handle this abort.
-		 * There is no need to pass the error into the guest.
-		 */
-		if (kvm_handle_guest_sea(fault_ipa, kvm_vcpu_get_esr(vcpu)))
-			kvm_inject_vabt(vcpu);
-
-		return 1;
-	}
-
 	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_esr(vcpu),
 			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
 
-- 
2.39.5

Re: [PATCH 1/3] KVM: arm64: Only read HPFAR_EL2 when value is architecturally valid

[Marc Zyngier]

On Tue, 01 Apr 2025 23:42:32 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> KVM's logic for deciding when HPFAR_EL2 is UNKNOWN doesn't align with
> the architecture. Most notably, KVM assumes HPFAR_EL2 contains the
> faulting IPA even in the case of an SEA.
> 
> Align the logic with the architecture rather than attempting to
> paraphrase it. Additionally, take the opportunity to improve the
> language around ARM erratum #834220 such that it actually describes the
> bug.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/esr.h           |  1 +
>  arch/arm64/kvm/hyp/include/hyp/fault.h | 46 ++++++++++++++++----------
>  2 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index d1b1a33f9a8b..7b096ed87360 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -121,6 +121,7 @@
>  #define ESR_ELx_FSC_SEA_TTW(n)	(0x14 + (n))
>  #define ESR_ELx_FSC_SECC	(0x18)
>  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
> +#define ESR_ELx_FSC_ADDRESS	(0x00)

I think this should probably read "ADDRESS_SIZE", rather than just
"ADDRESS".

>
>  /* Status codes for individual page table levels */
>  #define ESR_ELx_FSC_ACCESS_L(n)	(ESR_ELx_FSC_ACCESS + (n))
> diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
> index 17df94570f03..84d165e17bd0 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/fault.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
> @@ -44,31 +44,41 @@ static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
>  	return true;
>  }
>  
> +/*
> + * Checks for the conditions when HPFAR_EL2 is written, per DDI0487L.a D24.2.70.
> + */

You could also quote R_FKLWR, which was clarified in C23700 (Known
Issues L.a-03) by making the text clearer *and* adding a couple of
embarrassing typos (PFAR_EL2 instead of HPFAR_EL2). If anything, the
rule is more or less guaranteed to keep its reference, while the
numbering above will definitely move around.

> +static inline bool __hpfar_valid(u64 esr)
> +{
> +	/*
> +	 * CPUs affected by ARM erratum #834220 may incorrectly report a
> +	 * stage-2 translation fault when a stage-1 permission fault occurs.
> +	 *
> +	 * Re-walk the page tables to determine if a stage-1 fault actually
> +	 * occurred.
> +	 */
> +	if (cpus_have_final_cap(ARM64_WORKAROUND_834220) &&
> +	    esr_fsc_is_translation_fault(esr))
> +		return false;
> +
> +	if (esr_fsc_is_translation_fault(esr) || esr_fsc_is_access_flag_fault(esr))
> +		return true;
> +
> +	if ((esr & ESR_ELx_S1PTW) && esr_fsc_is_permission_fault(esr))
> +		return true;
> +
> +	return (esr & ESR_ELx_FSC) == ESR_ELx_FSC_ADDRESS;

Maybe add a esr_fsc_is_addr_sz_fault()?

> +}
> +
>  static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
>  {
>  	u64 hpfar, far;
>  
>  	far = read_sysreg_el2(SYS_FAR);
>  
> -	/*
> -	 * The HPFAR can be invalid if the stage 2 fault did not
> -	 * happen during a stage 1 page table walk (the ESR_EL2.S1PTW
> -	 * bit is clear) and one of the two following cases are true:
> -	 *   1. The fault was due to a permission fault
> -	 *   2. The processor carries errata 834220
> -	 *
> -	 * Therefore, for all non S1PTW faults where we either have a
> -	 * permission fault or the errata workaround is enabled, we
> -	 * resolve the IPA using the AT instruction.
> -	 */
> -	if (!(esr & ESR_ELx_S1PTW) &&
> -	    (cpus_have_final_cap(ARM64_WORKAROUND_834220) ||
> -	     esr_fsc_is_permission_fault(esr))) {
> -		if (!__translate_far_to_hpfar(far, &hpfar))
> -			return false;
> -	} else {
> +	if (__hpfar_valid(esr))
>  		hpfar = read_sysreg(hpfar_el2);
> -	}
> +	else if (!__translate_far_to_hpfar(far, &hpfar))
> +		return false;
>  
>  	fault->far_el2 = far;
>  	fault->hpfar_el2 = hpfar;

Otherwise looks OK to me.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 1/3] KVM: arm64: Only read HPFAR_EL2 when value is architecturally valid

[Marc Zyngier]

On Wed, 02 Apr 2025 12:15:52 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Tue, 01 Apr 2025 23:42:32 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > KVM's logic for deciding when HPFAR_EL2 is UNKNOWN doesn't align with
> > the architecture. Most notably, KVM assumes HPFAR_EL2 contains the
> > faulting IPA even in the case of an SEA.
> > 
> > Align the logic with the architecture rather than attempting to
> > paraphrase it. Additionally, take the opportunity to improve the
> > language around ARM erratum #834220 such that it actually describes the
> > bug.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/include/asm/esr.h           |  1 +
> >  arch/arm64/kvm/hyp/include/hyp/fault.h | 46 ++++++++++++++++----------
> >  2 files changed, 29 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > index d1b1a33f9a8b..7b096ed87360 100644
> > --- a/arch/arm64/include/asm/esr.h
> > +++ b/arch/arm64/include/asm/esr.h
> > @@ -121,6 +121,7 @@
> >  #define ESR_ELx_FSC_SEA_TTW(n)	(0x14 + (n))
> >  #define ESR_ELx_FSC_SECC	(0x18)
> >  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
> > +#define ESR_ELx_FSC_ADDRESS	(0x00)
> 
> I think this should probably read "ADDRESS_SIZE", rather than just
> "ADDRESS".

Actually, we have

#define ESR_ELx_FSC_ADDRSZ	(0x00)

since

61e30b9eef7f ("KVM: arm64: nv: Implement nested Stage-2 page table walk logic")

It just isn't at the expected spot.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 3/3] KVM: arm64: Don't translate FAR if invalid/unsafe

[Marc Zyngier]

On Tue, 01 Apr 2025 23:42:34 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Don't re-walk the page tables if an SEA occurred during the faulting
> page table walk to avoid taking a fatal exception in the hyp.
> Additionally, check that FAR_EL2 is valid for SEAs not taken on PTW
> as the architecture doesn't guarantee it contains the fault VA.
> 
> Finally, fix up the rest of the abort path by checking for SEAs early
> and bugging the VM if we get further along with an UNKNOWN fault IPA.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/esr.h           | 22 ++++++++++++++++++
>  arch/arm64/include/asm/kvm_emulate.h   |  7 +++++-
>  arch/arm64/include/asm/kvm_ras.h       |  2 +-
>  arch/arm64/kvm/hyp/include/hyp/fault.h | 26 ++++++++++++++++-----
>  arch/arm64/kvm/mmu.c                   | 31 ++++++++++++++++----------
>  5 files changed, 69 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7b096ed87360..4b874ad95f2e 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -465,6 +465,28 @@ static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
>  	       (esr == ESR_ELx_FSC_ACCESS_L(0));
>  }
>  
> +static inline bool esr_fsc_is_sea_ttw(unsigned long esr)
> +{
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_SEA_TTW(3)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(2)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(1)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(0)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(-1));
> +}
> +
> +static inline bool esr_fsc_is_secc_ttw(unsigned long esr)
> +{
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_SECC_TTW(3)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(2)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(1)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(0)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(-1));
> +}
> +
>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
>  static inline bool esr_iss_is_eretax(unsigned long esr)
>  {
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 5d8c0832acbe..775e200a9be4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -305,7 +305,12 @@ static __always_inline unsigned long kvm_vcpu_get_hfar(const struct kvm_vcpu *vc
>  
>  static __always_inline phys_addr_t kvm_vcpu_get_fault_ipa(const struct kvm_vcpu *vcpu)
>  {
> -	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
> +	u64 hpfar = vcpu->arch.fault.hpfar_el2;
> +
> +	if (unlikely(!(hpfar & HPFAR_EL2_NS)))
> +		return INVALID_GPA;

Eh, why not ;-)

> +
> +	return ((phys_addr_t)hpfar & HPFAR_EL2_FIPA) << 8;

nit: as you are reworking this area, I'd rather see

	return FIELD_GET(HPFAR_EL2_FIPA, hpfar) << 12;

as it nicely shows what the FIPA field represents, instead of the
odd-looking 8 here. The compiler will nicely sort it out anyway.

Same thing in handle_host_mem_abort(), both of which could be fixed in
the previous patch.

>  }
>  
>  static inline u64 kvm_vcpu_get_disr(const struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/include/asm/kvm_ras.h b/arch/arm64/include/asm/kvm_ras.h
> index 87e10d9a635b..9398ade632aa 100644
> --- a/arch/arm64/include/asm/kvm_ras.h
> +++ b/arch/arm64/include/asm/kvm_ras.h
> @@ -14,7 +14,7 @@
>   * Was this synchronous external abort a RAS notification?
>   * Returns '0' for errors handled by some RAS subsystem, or -ENOENT.
>   */
> -static inline int kvm_handle_guest_sea(phys_addr_t addr, u64 esr)
> +static inline int kvm_handle_guest_sea(void)
>  {
>  	/* apei_claim_sea(NULL) expects to mask interrupts itself */
>  	lockdep_assert_irqs_enabled();
> diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
> index 84d165e17bd0..1ac9b23a382a 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/fault.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
> @@ -12,6 +12,16 @@
>  #include <asm/kvm_hyp.h>
>  #include <asm/kvm_mmu.h>
>  
> +static inline bool __fault_safe_to_translate(u64 esr)
> +{
> +	u64 fsc = esr & ESR_ELx_FSC;
> +
> +	if (esr_fsc_is_sea_ttw(esr) || esr_fsc_is_secc_ttw(esr))
> +		return false;
> +
> +	return !(fsc == ESR_ELx_FSC_EXTABT && (esr & ESR_ELx_FnV));
> +}

Ah, neat.

> +
>  static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
>  {
>  	int ret;
> @@ -71,17 +81,23 @@ static inline bool __hpfar_valid(u64 esr)
>  
>  static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
>  {
> -	u64 hpfar, far;
> +	u64 hpfar;
>  
> -	far = read_sysreg_el2(SYS_FAR);
> +	fault->far_el2		= read_sysreg_el2(SYS_FAR);
> +	fault->hpfar_el2	= 0;
>  
>  	if (__hpfar_valid(esr))
>  		hpfar = read_sysreg(hpfar_el2);
> -	else if (!__translate_far_to_hpfar(far, &hpfar))
> +	else if (unlikely(!__fault_safe_to_translate(esr)))
> +		return true;

This may cause some unexpected damage, see below.

> +	else if (!__translate_far_to_hpfar(fault->far_el2, &hpfar))
>  		return false;
>  
> -	fault->far_el2 = far;
> -	fault->hpfar_el2 = hpfar;
> +	/*
> +	 * Hijack HPFAR_EL2.NS (RES0 in Non-secure) to indicate a valid
> +	 * HPFAR value.
> +	 */
> +	fault->hpfar_el2 = hpfar | HPFAR_EL2_NS;
>  	return true;
>  }
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 2feb6c6b63af..754f2fe0cc67 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1794,9 +1794,28 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>  	gfn_t gfn;
>  	int ret, idx;
>  
> +	/* Synchronous External Abort? */
> +	if (kvm_vcpu_abt_issea(vcpu)) {
> +		/*
> +		 * For RAS the host kernel may handle this abort.
> +		 * There is no need to pass the error into the guest.
> +		 */
> +		if (kvm_handle_guest_sea())
> +			kvm_inject_vabt(vcpu);
> +
> +		return 1;
> +	}
> +

Maybe add a line to the commit message to indicate that you're moving
this around?

>  	esr = kvm_vcpu_get_esr(vcpu);
>  
> +	/*
> +	 * The fault IPA should be reliable at this point as we're not dealing
> +	 * with an SEA.
> +	 */
>  	ipa = fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +	if (KVM_BUG_ON(ipa == INVALID_GPA, vcpu->kvm))
> +		return -EFAULT;
> +

I think you're missing something similar in handle_host_mem_abort(),
which will take fault.hpfar_el2 at face value even when it is unsafe
to translate. At least testing for HPFAR_EL2_NS seems necessary.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH 3/3] KVM: arm64: Don't translate FAR if invalid/unsafe

[Oliver Upton]

Hey,

On Wed, Apr 02, 2025 at 01:19:28PM +0100, Marc Zyngier wrote:
> On Tue, 01 Apr 2025 23:42:34 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> >  static __always_inline phys_addr_t kvm_vcpu_get_fault_ipa(const struct kvm_vcpu *vcpu)
> >  {
> > -	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
> > +	u64 hpfar = vcpu->arch.fault.hpfar_el2;
> > +
> > +	if (unlikely(!(hpfar & HPFAR_EL2_NS)))
> > +		return INVALID_GPA;
> 
> Eh, why not ;-)
> 
> > +
> > +	return ((phys_addr_t)hpfar & HPFAR_EL2_FIPA) << 8;
> 
> nit: as you are reworking this area, I'd rather see
> 
> 	return FIELD_GET(HPFAR_EL2_FIPA, hpfar) << 12;
> 
> as it nicely shows what the FIPA field represents, instead of the
> odd-looking 8 here. The compiler will nicely sort it out anyway.
> 
> Same thing in handle_host_mem_abort(), both of which could be fixed in
> the previous patch.

Agreed, this reads better.

> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 2feb6c6b63af..754f2fe0cc67 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1794,9 +1794,28 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
> >  	gfn_t gfn;
> >  	int ret, idx;
> >  
> > +	/* Synchronous External Abort? */
> > +	if (kvm_vcpu_abt_issea(vcpu)) {
> > +		/*
> > +		 * For RAS the host kernel may handle this abort.
> > +		 * There is no need to pass the error into the guest.
> > +		 */
> > +		if (kvm_handle_guest_sea())
> > +			kvm_inject_vabt(vcpu);
> > +
> > +		return 1;
> > +	}
> > +
> 
> Maybe add a line to the commit message to indicate that you're moving
> this around?

I had this:

  Finally, fix up the rest of the abort path by checking for SEAs early
  and bugging the VM if we get further along with an UNKNOWN fault IPA.

But I'm happy to expand a bit to make that more clear.

> >  	esr = kvm_vcpu_get_esr(vcpu);
> >  
> > +	/*
> > +	 * The fault IPA should be reliable at this point as we're not dealing
> > +	 * with an SEA.
> > +	 */
> >  	ipa = fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> > +	if (KVM_BUG_ON(ipa == INVALID_GPA, vcpu->kvm))
> > +		return -EFAULT;
> > +
> 
> I think you're missing something similar in handle_host_mem_abort(),
> which will take fault.hpfar_el2 at face value even when it is unsafe
> to translate. At least testing for HPFAR_EL2_NS seems necessary.

Good point, I'll respin in a moment.

Thanks for having a look!

Best,
Oliver

Re: [PATCH 1/3] KVM: arm64: Only read HPFAR_EL2 when value is architecturally valid

[Oliver Upton]

On Wed, Apr 02, 2025 at 12:30:29PM +0100, Marc Zyngier wrote:
> On Wed, 02 Apr 2025 12:15:52 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > On Tue, 01 Apr 2025 23:42:32 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > KVM's logic for deciding when HPFAR_EL2 is UNKNOWN doesn't align with
> > > the architecture. Most notably, KVM assumes HPFAR_EL2 contains the
> > > faulting IPA even in the case of an SEA.
> > > 
> > > Align the logic with the architecture rather than attempting to
> > > paraphrase it. Additionally, take the opportunity to improve the
> > > language around ARM erratum #834220 such that it actually describes the
> > > bug.
> > > 
> > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > > ---
> > >  arch/arm64/include/asm/esr.h           |  1 +
> > >  arch/arm64/kvm/hyp/include/hyp/fault.h | 46 ++++++++++++++++----------
> > >  2 files changed, 29 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > > index d1b1a33f9a8b..7b096ed87360 100644
> > > --- a/arch/arm64/include/asm/esr.h
> > > +++ b/arch/arm64/include/asm/esr.h
> > > @@ -121,6 +121,7 @@
> > >  #define ESR_ELx_FSC_SEA_TTW(n)	(0x14 + (n))
> > >  #define ESR_ELx_FSC_SECC	(0x18)
> > >  #define ESR_ELx_FSC_SECC_TTW(n)	(0x1c + (n))
> > > +#define ESR_ELx_FSC_ADDRESS	(0x00)
> > 
> > I think this should probably read "ADDRESS_SIZE", rather than just
> > "ADDRESS".
> 
> Actually, we have
> 
> #define ESR_ELx_FSC_ADDRSZ	(0x00)
> 
> since
> 
> 61e30b9eef7f ("KVM: arm64: nv: Implement nested Stage-2 page table walk logic")
> 
> It just isn't at the expected spot.

Durrrrr :)

Thanks,
Oliver

Re: [PATCH 3/3] KVM: arm64: Don't translate FAR if invalid/unsafe

[Marc Zyngier]

On Wed, 02 Apr 2025 17:37:57 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 2feb6c6b63af..754f2fe0cc67 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -1794,9 +1794,28 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
> > >  	gfn_t gfn;
> > >  	int ret, idx;
> > >  
> > > +	/* Synchronous External Abort? */
> > > +	if (kvm_vcpu_abt_issea(vcpu)) {
> > > +		/*
> > > +		 * For RAS the host kernel may handle this abort.
> > > +		 * There is no need to pass the error into the guest.
> > > +		 */
> > > +		if (kvm_handle_guest_sea())
> > > +			kvm_inject_vabt(vcpu);
> > > +
> > > +		return 1;
> > > +	}
> > > +
> > 
> > Maybe add a line to the commit message to indicate that you're moving
> > this around?
> 
> I had this:
> 
>   Finally, fix up the rest of the abort path by checking for SEAs early
>   and bugging the VM if we get further along with an UNKNOWN fault IPA.
> 
> But I'm happy to expand a bit to make that more clear.

Ah, I somehow glanced over it. No need to change anything then.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.