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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -394,6 +394,8 @@ struct hvm_hw_lapic {
>>      uint32_t             disabled; /* VLAPIC_xx_DISABLED */
>>      uint32_t             timer_divisor;
>>      uint64_t             tdt_msr;
>> +    uint32_t             x2apic_id;
>> +    uint32_t             rsvd_zero;
> 
> ... we do normally spell it _rsvd; to make it extra extra clear that
> people shouldn't be doing anything with it.

Alternatively, to carry the "zero" in the name, how about _mbz?

Jan