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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

Hi,

On Wed Oct 30, 2024 at 6:37 AM GMT, 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.

I understood "fix" there as "pin" rather than "unbreak". Regardless I can also
rewrite it as "The x2APIC ID is per-vCPU and shown on all subleafs"

>
> >> --- 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

I'd prefer that to _rsvd, if anything to make it patently clear that leaving
rubble is not ok.

Cheers,
Alejandro