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
