On Tue, Jun 10, 2025, Neeraj Upadhyay wrote:
> On 4/29/2025 8:12 PM, Sean Christopherson wrote:
> > Please slot the below in. And if there is any more code in this series that is
> > duplicating existing functionality, try to figure out a clean way to share code
> > instead of open coding yet another version.
> >
> > --
> > From: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Date: Tue, 29 Apr 2025 07:30:47 -0700
> > Subject: [PATCH] x86/apic: KVM: Deduplicate APIC vector => register+bit math
> >
> > 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.
> >
> > No functional change intended (clang-19 and gcc-14 generate bit-for-bit
> > identical code for all of kvm.ko).
> >
>
> With this change, I am observing difference in generated assembly for VEC_POS
> and REG_POS, as KVM code passes vector param with type "int" to these macros.
> Type casting "v" param of APIC_VECTOR_TO_BIT_NUMBER and APIC_VECTOR_TO_REG_OFFSET
> to "unsigned int" in the macro definition restores the original assembly. Can
> you have a look at this once? Below is the updated patch for this. Can you please
> share your feedback on this?
LGTM.
Ideally, KVM would probably pass around an "unsigned int", but some higher level
APIs in KVM use -1 to indicate an invalid vector (e.g. no IRQ pending), and mixing
and matching types would get a little weird and would require a decent amount of
churn. So casting in the macro where it matters seems like the best option, at
least for now.
Thanks much for taking care of this!
