On Tue, 25 Aug 2026 17:00:35 +0100, "Lorenzo Stoakes (ARM)" <[email protected]> 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. > > No functional change intended. > > Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]> > --- > 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(). 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. Also, 'inline' is a bit of a problem given that the callers are __always_inline for good reasons (see 5c37f1ae1c3358). > +{ > + 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. > } > > 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? > > 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. Thanks, M. -- Without deviation from the norm, progress is not possible.
