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

[Jan Beulich <jbeulich@xxxxxxxx>]

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;

?

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

Jan