Re: [PATCH v7 02/10] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/10/2024 6:37 am, Jan Beulich wrote:
> On 29.10.2024 21:30, Andrew Cooper wrote:
>> On 21/10/2024 4:45 pm, Alejandro Vallejo wrote:
>>> @@ -310,19 +309,16 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>>          break;
>>>  
>>>      case 0xb:
>>> -        /*
>>> -         * In principle, this leaf is Intel-only.  In practice, it is 
>>> tightly
>>> -         * coupled with x2apic, and we offer an x2apic-capable APIC 
>>> emulation
>>> -         * to guests on AMD hardware as well.
>>> -         *
>>> -         * TODO: Rework topology logic.
>>> -         */
>>>          if ( p->basic.x2apic )
>>>          {
>>>              *(uint8_t *)&res->c = subleaf;
>>>  
>>> -            /* Fix the x2APIC identifier. */
>>> -            res->d = v->vcpu_id * 2;
>>> +            /*
>>> +             * Fix the x2APIC identifier. The PV side is nonsensical, but
>>> +             * we've always shown it like this so it's kept for compat.
>>> +             */
>> In hindsight I should changed "Fix the x2APIC identifier." when I
>> reworked this logic, but oh well - better late than never.
>>
>> /* The x2APIC_ID is per-vCPU, and fixed irrespective of the requested
>> subleaf. */
> Can we perhaps avoid "fix" in this comment? "Adjusted", "overwritten", or
> some such ought to do, without carrying a hint towards some bug somewhere.

Not really.  This is actually a good example of why "fix" as is bugfix
is a weird corner of English, despite it being common in coding circles.


"Fix" means to attach one thing to another, along with a strong
implication that the other thing doesn't move.  This comes from Latin,
and the collective term for nails/screws/bolts/etc is "fixings".

Fix as in bugfix derives from "to repair" or "to mend", which in turn
comes from the fact that even today (and moreso several hundred years
ago), many repairs/mends involve affixing one thing back to something
else that doesn't move.


In this case, it is res->d which which is fixed (as in unmoving) with
respect to the subleaf index.  It is weird even for CPUID; it's the only
example I'm aware of where the content of the world logically the same
piece of information in any subleaf.

~Andrew