Re: [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area

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

Hi,

On Wed Oct 9, 2024 at 2:12 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:37, Alejandro Vallejo wrote:
> > @@ -311,18 +310,15 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
> >  
> >      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.
> > +         * Don't expose topology information to PV guests. Exposed on HVM
> > +         * along with x2APIC because they are tightly coupled.
> >           */
> > -        if ( p->basic.x2apic )
> > +        if ( is_hvm_domain(d) && p->basic.x2apic )
>
> This change isn't mentioned at all in the description, despite it having the
> potential of introducing a (perceived) regression. See the comments near the
> top of calculate_pv_max_policy() and near the top of
> domain_cpu_policy_changed(). What's wrong with ...
>
> >          {
> >              *(uint8_t *)&res->c = subleaf;
> >  
> >              /* Fix the x2APIC identifier. */
> > -            res->d = v->vcpu_id * 2;
> > +            res->d = vlapic_x2apic_id(vcpu_vlapic(v));
>
> ...
>
>             res->d = is_hvm_domain(d) ? vlapic_x2apic_id(vcpu_vlapic(v))
>                                       : v->vcpu_id * 2;
>
> ?

Hmmm. I haven't seem problems with PV guests, but that's a good point. While I
suspect no PV guest would use this value for anything relevant (seeing how
there's no actual APIC), handing out zeroes might still have bad consequences.

Sure, I'll amend it.

>
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1090,7 +1090,7 @@ static uint32_t x2apic_ldr_from_id(uint32_t id)
> >  static void set_x2apic_id(struct vlapic *vlapic)
> >  {
> >      const struct vcpu *v = vlapic_vcpu(vlapic);
> > -    uint32_t apic_id = v->vcpu_id * 2;
> > +    uint32_t apic_id = vlapic->hw.x2apic_id;
>
> Any reason you're open-coding vlapic_x2apic_id() here and ...
>
> > @@ -1470,7 +1470,7 @@ void vlapic_reset(struct vlapic *vlapic)
> >      if ( v->vcpu_id == 0 )
> >          vlapic->hw.apic_base_msr |= APIC_BASE_BSP;
> >  
> > -    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
> > +    vlapic_set_reg(vlapic, APIC_ID, SET_xAPIC_ID(vlapic->hw.x2apic_id));
>
> ... here?

Not a good one. vlapic_x2apic_id() exists mostly to allow self-contained
accesses from outside this translation unit. It makes no harm using the
accessor even inside, sure.

>
> Jan

Cheers,
Alejandro