Re: [RFC PATCH v8 16/35] x86/apic: Simplify bitwise operations on APIC bitmap

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

On 7/14/2025 4:22 PM, Borislav Petkov wrote:
> On Wed, Jul 09, 2025 at 07:35:41AM -0700, Sean Christopherson wrote:
>> On Wed, Jul 09, 2025, Neeraj Upadhyay wrote:
>>> Use 'regs' as a contiguous linear bitmap for bitwise operations in
>>> apic_{set|clear|test}_vector(). This makes the code simpler by eliminating
>>
>> That's very debatable.  I don't find this code to be any simpler.  Quite the
>> opposite; it adds yet another open coded math exercise, which is so "simple"
>> that it warrants its own comment to explain what it's doing.
>>
>> I'm not dead set against this, but I'd strongly prefer to drop this patch.
> 
>>> +static inline unsigned int get_vec_bit(unsigned int vec)
>>> +{
>>> +	/*
>>> +	 * The registers are 32-bit wide and 16-byte aligned.
>>> +	 * Compensate for the resulting bit number spacing.
>>> +	 */
>>> +	return vec + 96 * (vec / 32);
> 
> I kinda agree. The naked 96 doesn't tell me anything. If we do this, the
> explaination of what this thing does should be crystal clear, perhaps even
> with an example. And the naked numbers need to be defines with proper names.
> 

Ok. I have removed this change from the current series.

https://github.com/AMDESE/linux-kvm/commits/savic-guest-latest/

> Also:
> 
>>     This change results in slight increase in generated code size for
>>     gcc-14.2.
>>     
>>     - Without change
> 
> What is the asm supposed to tell me?
> 

Intent was to show that the functional impact (perf/code-size) is not
noticeable.

> The new change gets a LEA which is noticeable or so?
> 

No, not noticeable.

> The generated code size increase is, what, a couple of bytes? Who cares?
> 
> We add asm to commit messages when it is really important. Doesn't seem so to
> me here but maybe I'm missing an angle...
> 

Ok. This commit was aimed at simplifying (which folks find debatable) the usage
of bitmap ops and to match how bitmap operations are typically used in other code.
The intent of adding asm was to show that functional impact is low (while
providing "simplicity").


- Neeraj

> Thx.
>