On Wed, Feb 28, 2024 at 3:46 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > Open code the bit number directly in the PFERR_* masks and drop the > intermediate PFERR_*_BIT defines, as having to bounce through two macros > just to see which flag corresponds to which bit is quite annoying, as is > having to define two macros just to add recognition of a new flag. > > Use ilog2() to derive the bit in permission_fault(), the one function that > actually needs the bit number (it does clever shifting to manipulate flags > in order to avoid conditional branches). > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > arch/x86/include/asm/kvm_host.h | 32 ++++++++++---------------------- > arch/x86/kvm/mmu.h | 4 ++-- > 2 files changed, 12 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index aaf5a25ea7ed..88cc523bafa8 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -254,28 +254,16 @@ enum x86_intercept_stage; > KVM_GUESTDBG_INJECT_DB | \ > KVM_GUESTDBG_BLOCKIRQ) > > - > -#define PFERR_PRESENT_BIT 0 > -#define PFERR_WRITE_BIT 1 > -#define PFERR_USER_BIT 2 > -#define PFERR_RSVD_BIT 3 > -#define PFERR_FETCH_BIT 4 > -#define PFERR_PK_BIT 5 > -#define PFERR_SGX_BIT 15 > -#define PFERR_GUEST_FINAL_BIT 32 > -#define PFERR_GUEST_PAGE_BIT 33 > -#define PFERR_IMPLICIT_ACCESS_BIT 48 > - > -#define PFERR_PRESENT_MASK BIT(PFERR_PRESENT_BIT) > -#define PFERR_WRITE_MASK BIT(PFERR_WRITE_BIT) > -#define PFERR_USER_MASK BIT(PFERR_USER_BIT) > -#define PFERR_RSVD_MASK BIT(PFERR_RSVD_BIT) > -#define PFERR_FETCH_MASK BIT(PFERR_FETCH_BIT) > -#define PFERR_PK_MASK BIT(PFERR_PK_BIT) > -#define PFERR_SGX_MASK BIT(PFERR_SGX_BIT) > -#define PFERR_GUEST_FINAL_MASK BIT_ULL(PFERR_GUEST_FINAL_BIT) > -#define PFERR_GUEST_PAGE_MASK BIT_ULL(PFERR_GUEST_PAGE_BIT) > -#define PFERR_IMPLICIT_ACCESS BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT) > +#define PFERR_PRESENT_MASK BIT(0) > +#define PFERR_WRITE_MASK BIT(1) > +#define PFERR_USER_MASK BIT(2) > +#define PFERR_RSVD_MASK BIT(3) > +#define PFERR_FETCH_MASK BIT(4) > +#define PFERR_PK_MASK BIT(5) > +#define PFERR_SGX_MASK BIT(15) > +#define PFERR_GUEST_FINAL_MASK BIT_ULL(32) > +#define PFERR_GUEST_PAGE_MASK BIT_ULL(33) > +#define PFERR_IMPLICIT_ACCESS BIT_ULL(48) > > #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK | \ > PFERR_WRITE_MASK | \ > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 60f21bb4c27b..e8b620a85627 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -213,7 +213,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > */ > u64 implicit_access = access & PFERR_IMPLICIT_ACCESS; > bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC; > - int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1; > + int index = (pfec + (not_smap << ilog2(PFERR_RSVD_MASK))) >> 1; Just use "(pfec + (not_smap ? PFERR_RSVD_MASK : 0)) >> 1". Likewise below, "pte_access & PT_USER_MASK ? PFERR_RSVD_MASK : 0"/ No need to even check what the compiler produces, it will be either exactly the same code or a bunch of cmov instructions. Paolo > u32 errcode = PFERR_PRESENT_MASK; > bool fault; > > @@ -235,7 +235,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, > > /* clear present bit, replace PFEC.RSVD with ACC_USER_MASK. */ > offset = (pfec & ~1) + > - ((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT)); > + ((pte_access & PT_USER_MASK) << (ilog2(PFERR_RSVD_MASK) - PT_USER_SHIFT)); > > pkru_bits &= mmu->pkru_mask >> offset; > errcode |= -pkru_bits & PFERR_PK_MASK; > -- > 2.44.0.278.ge034bb2e1d-goog >
