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
