Re: [PATCH 1/8] KVM: arm64: Propagate and use esr in s2fd when handling guest aborts

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Sep 10, 2026 at 09:39:58AM +0100, Marc Zyngier wrote:
> On Tue, 25 Aug 2026 17:00:35 +0100,
> "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
> >
> > kvm_handle_guest_abort() establishes a kvm_s2_fault_desc data structure,
> > s2fd, to store and propagate state to either pkvm_mem_abort(), gmem_abort()
> > or user_mem_abort() handlers.
> >
> > Each of these, however, examines the Exception Syndrome Register (ESR) via
> > s2fd->vcpu.
> >
> > Introduce an s2fd->esr field to abstract this and propagate it to callers.
> >
> > The value of this (beyond refactoring) is to be able to later generate
> > faults with a synthetic esr, specifically to implement stage 2 page table
> > pre-faulting.
> >
> > Abstract esr-specific predicates and helpers to the esr.h header and either
> > have vcpu wrappers call these, or eliminate them if they are not used
> > elsewhere.
> >
> > Provide kvm_s2_fault_is_[write,exec,perm]() helpers for convenience.
> >
> > Since kvm_s2_fault_map() either sets perm_fault_granule to the permission
> > fault granule or 0 if not a permission fault, implement
> > kvm_s2_perm_fault_granule() to do this directly.
> >
> > Abort handlers which use kvm_s2_fault_desc - gmem_abort() and
> > user_mem_abort() - now only reference s2fd->esr and do not look it up in
> > any other way, which makes it safe to pass a synthetic s2fd->esr value to
> > these functions.
>
> Please split this. ESR helpers in one patch, hacking the MMU code to
> use it in another, s2fd->esr stuff last.

Ack will do!

>
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > ---
> >  arch/arm64/include/asm/esr.h         | 129 +++++++++++++++++++++++++----------
> >  arch/arm64/include/asm/kvm_emulate.h |  52 ++++----------
> >  arch/arm64/kvm/mmu.c                 |  80 +++++++++++++---------
> >  3 files changed, 156 insertions(+), 105 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > index f816f5d77f1a..162e90c832e9 100644
> > --- a/arch/arm64/include/asm/esr.h
> > +++ b/arch/arm64/include/asm/esr.h
> > @@ -437,6 +437,32 @@
> >  #ifndef __ASSEMBLER__
> >  #include <asm/types.h>
> >
> > +static inline u8 esr_trap_get_class(unsigned long esr)
>
> nit: there is no notion of trap here. This is simply extracting the EC
> from the ESR. My personal (and wholly unreliable) taste would be to go
> for something like esr_get_ec().

Ack will change!

>
> I appreciate that you are simply propagating the names used in KVM,
> but they were pretty poor the first place, and have only been kept to
> avoid churn.

Yeah, when in Rome etc. :)

>
> Also, 'inline' is a bit of a problem given that the callers are
> __always_inline for good reasons (see 5c37f1ae1c3358).

Ack will fix up.

>
> > +{
> > +	return ESR_ELx_EC(esr);
> > +}
> > +
> > +static inline bool esr_trap_is_iabt(unsigned long esr)
> > +{
> > +	return esr_trap_get_class(esr) == ESR_ELx_EC_IABT_LOW;
> > +}
> > +
> > +static inline bool esr_abt_is_s1ptw(unsigned long esr)
> > +{
> > +	return esr & ESR_ELx_S1PTW;
> > +}
> > +
> > +/* Always check for S1PTW *before* using this. */
> > +static inline bool esr_dabt_is_write(unsigned long esr)
> > +{
> > +	return esr & ESR_ELx_WNR;
> > +}
> > +
> > +static inline bool esr_dabt_is_cm(unsigned long esr)
> > +{
> > +	return esr & ESR_ELx_CM;
> > +}
> > +
> >  static inline unsigned long esr_brk_comment(unsigned long esr)
> >  {
> >  	return esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
> > @@ -460,75 +486,104 @@ static inline bool esr_is_ubsan_brk(unsigned long esr)
> >  	return (esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM;
> >  }
> >
> > +static inline u8 esr_fsc_get_fault(unsigned long esr)
> > +{
> > +	return esr & ESR_ELx_FSC;
> > +}
> > +
> >  static inline bool esr_fsc_is_translation_fault(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> >
> > -	return (esr == ESR_ELx_FSC_FAULT_L(3)) ||
> > -	       (esr == ESR_ELx_FSC_FAULT_L(2)) ||
> > -	       (esr == ESR_ELx_FSC_FAULT_L(1)) ||
> > -	       (esr == ESR_ELx_FSC_FAULT_L(0)) ||
> > -	       (esr == ESR_ELx_FSC_FAULT_L(-1));
> > +	return (fault == ESR_ELx_FSC_FAULT_L(3)) ||
> > +	       (fault == ESR_ELx_FSC_FAULT_L(2)) ||
> > +	       (fault == ESR_ELx_FSC_FAULT_L(1)) ||
> > +	       (fault == ESR_ELx_FSC_FAULT_L(0)) ||
> > +	       (fault == ESR_ELx_FSC_FAULT_L(-1));
>
> I really think we could do without this sort of churn.

Sure, will avoid the var name changes etc. on respin.

>
> >  }
> >
> >  static inline bool esr_fsc_is_permission_fault(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> >
> > -	return (esr == ESR_ELx_FSC_PERM_L(3)) ||
> > -	       (esr == ESR_ELx_FSC_PERM_L(2)) ||
> > -	       (esr == ESR_ELx_FSC_PERM_L(1)) ||
> > -	       (esr == ESR_ELx_FSC_PERM_L(0));
> > +	return (fault == ESR_ELx_FSC_PERM_L(3)) ||
> > +	       (fault == ESR_ELx_FSC_PERM_L(2)) ||
> > +	       (fault == ESR_ELx_FSC_PERM_L(1)) ||
> > +	       (fault == ESR_ELx_FSC_PERM_L(0));
> >  }
> >
> >  static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> >
> > -	return (esr == ESR_ELx_FSC_ACCESS_L(3)) ||
> > -	       (esr == ESR_ELx_FSC_ACCESS_L(2)) ||
> > -	       (esr == ESR_ELx_FSC_ACCESS_L(1)) ||
> > -	       (esr == ESR_ELx_FSC_ACCESS_L(0));
> > +	return (fault == ESR_ELx_FSC_ACCESS_L(3)) ||
> > +	       (fault == ESR_ELx_FSC_ACCESS_L(2)) ||
> > +	       (fault == ESR_ELx_FSC_ACCESS_L(1)) ||
> > +	       (fault == ESR_ELx_FSC_ACCESS_L(0));
> >  }
> >
> >  static inline bool esr_fsc_is_excl_atomic_fault(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > -
> > -	return esr == ESR_ELx_FSC_EXCL_ATOMIC;
> > +	return esr_fsc_get_fault(esr) == ESR_ELx_FSC_EXCL_ATOMIC;
> >  }
> >
> >  static inline bool esr_fsc_is_addr_sz_fault(unsigned long esr)
> >  {
> > -	esr &= ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> > +
> > +	return (fault == ESR_ELx_FSC_ADDRSZ_L(3)) ||
> > +	       (fault == ESR_ELx_FSC_ADDRSZ_L(2)) ||
> > +	       (fault == ESR_ELx_FSC_ADDRSZ_L(1)) ||
> > +	       (fault == ESR_ELx_FSC_ADDRSZ_L(0)) ||
> > +	       (fault == ESR_ELx_FSC_ADDRSZ_L(-1));
> > +}
> > +
> > +static inline bool esr_abt_is_exec_fault(unsigned long esr)
> > +{
> > +	return esr_trap_is_iabt(esr) && !esr_abt_is_s1ptw(esr);
> > +}
> >
> > -	return (esr == ESR_ELx_FSC_ADDRSZ_L(3))	||
> > -	       (esr == ESR_ELx_FSC_ADDRSZ_L(2))	||
> > -	       (esr == ESR_ELx_FSC_ADDRSZ_L(1)) ||
> > -	       (esr == ESR_ELx_FSC_ADDRSZ_L(0))	||
> > -	       (esr == ESR_ELx_FSC_ADDRSZ_L(-1));
> > +static inline bool esr_abt_is_sea(unsigned long esr)
> > +{
> > +	const u8 fault = esr_fsc_get_fault(esr);
> > +
> > +	switch (fault) {
> > +	case ESR_ELx_FSC_EXTABT:
> > +	case ESR_ELx_FSC_SEA_TTW(-1) ... ESR_ELx_FSC_SEA_TTW(3):
> > +	case ESR_ELx_FSC_SECC:
> > +	case ESR_ELx_FSC_SECC_TTW(-1) ... ESR_ELx_FSC_SECC_TTW(3):
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +/* Not valid for negative levels. */
> > +static inline u64 esr_fsc_get_level(unsigned long esr)
> > +{
> > +	return esr & ESR_ELx_FSC_LEVEL;
> >  }
>
> If that's such an unreliable helper, why is it exposed to everyone
> instead of being kept local to the single caller?

That's a fair point :) will open code instead.

>
> >
> >  static inline bool esr_fsc_is_sea_ttw(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> >
> > -	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));
> > +	return (fault == ESR_ELx_FSC_SEA_TTW(3)) ||
> > +	       (fault == ESR_ELx_FSC_SEA_TTW(2)) ||
> > +	       (fault == ESR_ELx_FSC_SEA_TTW(1)) ||
> > +	       (fault == ESR_ELx_FSC_SEA_TTW(0)) ||
> > +	       (fault == ESR_ELx_FSC_SEA_TTW(-1));
> >  }
> >
> >  static inline bool esr_fsc_is_secc_ttw(unsigned long esr)
> >  {
> > -	esr = esr & ESR_ELx_FSC;
> > +	const u8 fault = esr_fsc_get_fault(esr);
> >
> > -	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));
> > +	return (fault == ESR_ELx_FSC_SECC_TTW(3)) ||
> > +	       (fault == ESR_ELx_FSC_SECC_TTW(2)) ||
> > +	       (fault == ESR_ELx_FSC_SECC_TTW(1)) ||
> > +	       (fault == ESR_ELx_FSC_SECC_TTW(0)) ||
> > +	       (fault == ESR_ELx_FSC_SECC_TTW(-1));
> >  }
> >
> >  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index a3c1928bdf74..811d7a68a9f9 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -411,18 +411,13 @@ static __always_inline int kvm_vcpu_dabt_get_rd(const struct kvm_vcpu *vcpu)
> >
> >  static __always_inline bool kvm_vcpu_abt_iss1tw(const struct kvm_vcpu *vcpu)
> >  {
> > -	return !!(kvm_vcpu_get_esr(vcpu) & ESR_ELx_S1PTW);
> > +	return esr_abt_is_s1ptw(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  /* Always check for S1PTW *before* using this. */
> >  static __always_inline bool kvm_vcpu_dabt_iswrite(const struct kvm_vcpu *vcpu)
> >  {
> > -	return kvm_vcpu_get_esr(vcpu) & ESR_ELx_WNR;
> > -}
> > -
> > -static inline bool kvm_vcpu_dabt_is_cm(const struct kvm_vcpu *vcpu)
> > -{
> > -	return !!(kvm_vcpu_get_esr(vcpu) & ESR_ELx_CM);
> > +	return esr_dabt_is_write(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  static __always_inline unsigned int kvm_vcpu_dabt_get_as(const struct kvm_vcpu *vcpu)
> > @@ -438,17 +433,12 @@ static __always_inline bool kvm_vcpu_trap_il_is32bit(const struct kvm_vcpu *vcpu
> >
> >  static __always_inline u8 kvm_vcpu_trap_get_class(const struct kvm_vcpu *vcpu)
> >  {
> > -	return ESR_ELx_EC(kvm_vcpu_get_esr(vcpu));
> > +	return esr_trap_get_class(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  static inline bool kvm_vcpu_trap_is_iabt(const struct kvm_vcpu *vcpu)
> >  {
> > -	return kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_IABT_LOW;
> > -}
> > -
> > -static inline bool kvm_vcpu_trap_is_exec_fault(const struct kvm_vcpu *vcpu)
> > -{
> > -	return kvm_vcpu_trap_is_iabt(vcpu) && !kvm_vcpu_abt_iss1tw(vcpu);
> > +	return esr_trap_is_iabt(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu)
> > @@ -468,26 +458,9 @@ bool kvm_vcpu_trap_is_translation_fault(const struct kvm_vcpu *vcpu)
> >  	return esr_fsc_is_translation_fault(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> > -static inline
> > -u64 kvm_vcpu_trap_get_perm_fault_granule(const struct kvm_vcpu *vcpu)
> > -{
> > -	unsigned long esr = kvm_vcpu_get_esr(vcpu);
> > -
> > -	BUG_ON(!esr_fsc_is_permission_fault(esr));
> > -	return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(esr & ESR_ELx_FSC_LEVEL));
> > -}
> > -
> >  static __always_inline bool kvm_vcpu_abt_issea(const struct kvm_vcpu *vcpu)
> >  {
> > -	switch (kvm_vcpu_trap_get_fault(vcpu)) {
> > -	case ESR_ELx_FSC_EXTABT:
> > -	case ESR_ELx_FSC_SEA_TTW(-1) ... ESR_ELx_FSC_SEA_TTW(3):
> > -	case ESR_ELx_FSC_SECC:
> > -	case ESR_ELx_FSC_SECC_TTW(-1) ... ESR_ELx_FSC_SECC_TTW(3):
> > -		return true;
> > -	default:
> > -		return false;
> > -	}
> > +	return esr_abt_is_sea(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  static __always_inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
> > @@ -496,9 +469,9 @@ static __always_inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
> >  	return ESR_ELx_SYS64_ISS_RT(esr);
> >  }
> >
> > -static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> > +static inline bool esr_abt_is_write_fault(unsigned long esr)
> >  {
> > -	if (kvm_vcpu_abt_iss1tw(vcpu)) {
> > +	if (esr_abt_is_s1ptw(esr)) {
> >  		/*
> >  		 * Only a permission fault on a S1PTW should be
> >  		 * considered as a write. Otherwise, page tables baked
> > @@ -511,13 +484,18 @@ static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> >  		 * first), then a permission fault to allow the flags
> >  		 * to be set.
> >  		 */
> > -		return kvm_vcpu_trap_is_permission_fault(vcpu);
> > +		return esr_fsc_is_permission_fault(esr);
> >  	}
> >
> > -	if (kvm_vcpu_trap_is_iabt(vcpu))
> > +	if (esr_trap_is_iabt(esr))
> >  		return false;
> >
> > -	return kvm_vcpu_dabt_iswrite(vcpu);
> > +	return esr_dabt_is_write(esr);
> > +}
> > +
> > +static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> > +{
> > +	return esr_abt_is_write_fault(kvm_vcpu_get_esr(vcpu));
> >  }
> >
> >  static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 74e7e7f7564c..30d605e87b01 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1603,12 +1603,38 @@ struct kvm_s2_fault_desc {
> >  	struct kvm_s2_trans	*nested;
> >  	struct kvm_memory_slot	*memslot;
> >  	unsigned long		hva;
> > +	unsigned long		esr;
> >  };
> >
> > +static bool kvm_s2_fault_is_perm(const struct kvm_s2_fault_desc *s2fd)
> > +{
> > +	return esr_fsc_is_permission_fault(s2fd->esr);
> > +}
> > +
> > +static bool kvm_s2_fault_is_exec(const struct kvm_s2_fault_desc *s2fd)
> > +{
> > +	return esr_abt_is_exec_fault(s2fd->esr);
> > +}
> > +
> > +static bool kvm_s2_fault_is_write(const struct kvm_s2_fault_desc *s2fd)
> > +{
> > +	return esr_abt_is_write_fault(s2fd->esr);
> > +}
> > +
> > +static u64 kvm_s2_perm_fault_granule(const struct kvm_s2_fault_desc *s2fd)
> > +{
> > +	u64 level;
> > +
> > +	if (!kvm_s2_fault_is_perm(s2fd))
> > +		return 0;
> > +	level = esr_fsc_get_level(s2fd->esr);
> > +	return BIT(ARM64_HW_PGTABLE_LEVEL_SHIFT(level));
> > +}
> > +
> >  static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
> >  {
> >  	bool write_fault, exec_fault;
> > -	bool perm_fault = kvm_vcpu_trap_is_permission_fault(s2fd->vcpu);
> > +	const bool perm_fault = kvm_s2_fault_is_perm(s2fd);
>
> Please don't randomly introduce const local variables. I understand
> the benefit, but *if* we want to go down that road, then we do it for
> all the predicates, as a separate series, because this obviously
> applies to {exec,write}_fault as well.

Ack, force of habit :) will avoid on respin.

>
> Thanks,
>
> 	M.
>
> --
> Without deviation from the norm, progress is not possible.

--
Cheers, Lorenzo