[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |