[RFC PATCH v7 03/37] x86/apic: KVM: Deduplicate APIC vector => register+bit math

Neeraj Upadhyay posted 37 patches 8 months ago
There is a newer version of this series
[RFC PATCH v7 03/37] x86/apic: KVM: Deduplicate APIC vector => register+bit math
Posted by Neeraj Upadhyay 8 months ago
From: Sean Christopherson <seanjc@google.com>

Consolidate KVM's {REG,VEC}_POS() macros and lapic_vector_set_in_irr()'s
open coded equivalent logic in anticipation of the kernel gaining more
usage of vector => reg+bit lookups.

Use lapic_vector_set_in_irr()'s math as using divides for both the bit
number and register offset makes it easier to connect the dots, and for at
least one user, fixup_irqs(), "/ 32 * 0x10" generates ever so slightly
better code with gcc-14 (shaves a whole 3 bytes from the code stream):

((v) >> 5) << 4:
  c1 ef 05           shr    $0x5,%edi
  c1 e7 04           shl    $0x4,%edi
  81 c7 00 02 00 00  add    $0x200,%edi

(v) / 32 * 0x10:
  c1 ef 05           shr    $0x5,%edi
  83 c7 20           add    $0x20,%edi
  c1 e7 04           shl    $0x4,%edi

Keep KVM's tersely named macros as "wrappers" to avoid unnecessary churn
in KVM, and because the shorter names yield more readable code overall in
KVM.

The new macros type cast the vector parameter to "unsigned int". This is
required from better code generation for cases where an "int" is passed
to these macros in kvm code.

int v;

((v) >> 5) << 4:

  c1 f8 05    sar    $0x5,%eax
  c1 e0 04    shl    $0x4,%eax

((v) / 32 * 0x10):

  85 ff       test   %edi,%edi
  8d 47 1f    lea    0x1f(%rdi),%eax
  0f 49 c7    cmovns %edi,%eax
  c1 f8 05    sar    $0x5,%eax
  c1 e0 04    shl    $0x4,%eax

((unsigned int)(v) / 32 * 0x10):

  c1 f8 05    sar    $0x5,%eax
  c1 e0 04    shl    $0x4,%eax

(v) & (32 - 1):

  89 f8       mov    %edi,%eax
  83 e0 1f    and    $0x1f,%eax

(v) % 32

  89 fa       mov    %edi,%edx
  c1 fa 1f    sar    $0x1f,%edx
  c1 ea 1b    shr    $0x1b,%edx
  8d 04 17    lea    (%rdi,%rdx,1),%eax
  83 e0 1f    and    $0x1f,%eax
  29 d0       sub    %edx,%eax

(unsigned int)(v) % 32:

  89 f8       mov    %edi,%eax
  83 e0 1f    and    $0x1f,%eax

Overall kvm.ko text size is impacted if "unsigned int" is not used.

Bin      Orig     New (w/o unsigned int)  New (w/ unsigned int)

lapic.o  28580        28772                 28580
kvm.o    670810       671002                670810
kvm.ko   708079       708271                708079

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
[Neeraj: Type cast vec macro param to "unsigned int", provide data
         in commit log on "unsigned int" requirement]
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v6:

 - Added "unsigned int" type cast in the macro definitions.
 - Added details to the commit log.

 arch/x86/include/asm/apic.h | 7 +++++--
 arch/x86/kvm/lapic.h        | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 23d86c9750b9..c84d4e86fe4e 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -488,11 +488,14 @@ static inline void apic_setup_apic_calls(void) { }
 
 extern void apic_ack_irq(struct irq_data *data);
 
+#define APIC_VECTOR_TO_BIT_NUMBER(v) ((unsigned int)(v) % 32)
+#define APIC_VECTOR_TO_REG_OFFSET(v) ((unsigned int)(v) / 32 * 0x10)
+
 static inline bool lapic_vector_set_in_irr(unsigned int vector)
 {
-	u32 irr = apic_read(APIC_IRR + (vector / 32 * 0x10));
+	u32 irr = apic_read(APIC_IRR + APIC_VECTOR_TO_REG_OFFSET(vector));
 
-	return !!(irr & (1U << (vector % 32)));
+	return !!(irr & (1U << APIC_VECTOR_TO_BIT_NUMBER(vector)));
 }
 
 static inline bool is_vector_pending(unsigned int vector)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 1638a3da383a..56369d331bfc 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -145,8 +145,8 @@ void kvm_lapic_exit(void);
 
 u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic);
 
-#define VEC_POS(v) ((v) & (32 - 1))
-#define REG_POS(v) (((v) >> 5) << 4)
+#define VEC_POS(v) APIC_VECTOR_TO_BIT_NUMBER(v)
+#define REG_POS(v) APIC_VECTOR_TO_REG_OFFSET(v)
 
 static inline void kvm_lapic_clear_vector(int vec, void *bitmap)
 {
-- 
2.34.1
Re: [RFC PATCH v7 03/37] x86/apic: KVM: Deduplicate APIC vector => register+bit math
Posted by Borislav Petkov 7 months, 2 weeks ago
On Tue, Jun 10, 2025 at 11:23:50PM +0530, Neeraj Upadhyay wrote:
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 23d86c9750b9..c84d4e86fe4e 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -488,11 +488,14 @@ static inline void apic_setup_apic_calls(void) { }
>  
>  extern void apic_ack_irq(struct irq_data *data);
>  
> +#define APIC_VECTOR_TO_BIT_NUMBER(v) ((unsigned int)(v) % 32)
> +#define APIC_VECTOR_TO_REG_OFFSET(v) ((unsigned int)(v) / 32 * 0x10)

Dunno - I'd probably shorten those macro names:

APIC_VEC_TO_BITNUM()
APIC_VEC_TO_REGOFF()

because this way of shortening those words is very common and is still very
readable, even if not fully written out...

LGTM regardless.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [RFC PATCH v7 03/37] x86/apic: KVM: Deduplicate APIC vector => register+bit math
Posted by Neeraj Upadhyay 7 months, 2 weeks ago

On 6/23/2025 5:19 PM, Borislav Petkov wrote:
> On Tue, Jun 10, 2025 at 11:23:50PM +0530, Neeraj Upadhyay wrote:
>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index 23d86c9750b9..c84d4e86fe4e 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -488,11 +488,14 @@ static inline void apic_setup_apic_calls(void) { }
>>  
>>  extern void apic_ack_irq(struct irq_data *data);
>>  
>> +#define APIC_VECTOR_TO_BIT_NUMBER(v) ((unsigned int)(v) % 32)
>> +#define APIC_VECTOR_TO_REG_OFFSET(v) ((unsigned int)(v) / 32 * 0x10)
> 
> Dunno - I'd probably shorten those macro names:
> 
> APIC_VEC_TO_BITNUM()
> APIC_VEC_TO_REGOFF()
> 
> because this way of shortening those words is very common and is still very
> readable, even if not fully written out...
> 

Sounds good to me. Will change this in next version (will also wait for Sean's
comment on this).

> LGTM regardless.
> 

Thanks!

- Neeraj

> Thx.
>
Re: [RFC PATCH v7 03/37] x86/apic: KVM: Deduplicate APIC vector => register+bit math
Posted by Sean Christopherson 7 months, 2 weeks ago
On Wed, Jun 25, 2025, Neeraj Upadhyay wrote:
> 
> 
> On 6/23/2025 5:19 PM, Borislav Petkov wrote:
> > On Tue, Jun 10, 2025 at 11:23:50PM +0530, Neeraj Upadhyay wrote:
> >> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> >> index 23d86c9750b9..c84d4e86fe4e 100644
> >> --- a/arch/x86/include/asm/apic.h
> >> +++ b/arch/x86/include/asm/apic.h
> >> @@ -488,11 +488,14 @@ static inline void apic_setup_apic_calls(void) { }
> >>  
> >>  extern void apic_ack_irq(struct irq_data *data);
> >>  
> >> +#define APIC_VECTOR_TO_BIT_NUMBER(v) ((unsigned int)(v) % 32)
> >> +#define APIC_VECTOR_TO_REG_OFFSET(v) ((unsigned int)(v) / 32 * 0x10)
> > 
> > Dunno - I'd probably shorten those macro names:
> > 
> > APIC_VEC_TO_BITNUM()
> > APIC_VEC_TO_REGOFF()
> > 
> > because this way of shortening those words is very common and is still very
> > readable, even if not fully written out...
> > 
> 
> Sounds good to me. Will change this in next version (will also wait for Sean's
> comment on this).

My vote is for the long names.  I find REG_OFFSET in particular to be much more
self-documenting.  My brain gets there eventually with REGOFF, but I just don't
see the point in shortening the names.  The only uses where line lengths get a
bit too long are apic_set_isr() and apic_clear_isr(), and the lines are still
quite long with the shorter names.