linux-kernel - Re: [PATCH v5 01/20] KVM: x86: Move find_highest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aEhZaMuipi2qePHX@google.com>
Date: Tue, 10 Jun 2025 09:12:24 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Cc: linux-kernel@...r.kernel.org, bp@...en8.de, tglx@...utronix.de, 
	mingo@...hat.com, dave.hansen@...ux.intel.com, Thomas.Lendacky@....com, 
	nikunj@....com, Santosh.Shukla@....com, Vasant.Hegde@....com, 
	Suravee.Suthikulpanit@....com, David.Kaplan@....com, x86@...nel.org, 
	hpa@...or.com, peterz@...radead.org, pbonzini@...hat.com, kvm@...r.kernel.org, 
	kirill.shutemov@...ux.intel.com, huibo.wang@....com, naveen.rao@....com, 
	francescolavra.fl@...il.com
Subject: Re: [PATCH v5 01/20] KVM: x86: Move find_highest_vector() to a common header

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@...gle.com>
> > 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!