| Message ID | 4095f31a88589ced2b620e8ebbb84cdc2fae8914.1715102098.git.alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
On Wed, May 08, 2024 at 01:39:20PM +0100, Alejandro Vallejo wrote: > This allows the initial x2APIC ID to be sent on the migration stream. The > hardcoded mapping x2apic_id=2*vcpu_id is maintained for the time being. > Given the vlapic data is zero-extended on restore, fix up migrations from > hosts without the field by setting it to the old convention if zero. > > x2APIC IDs are calculated from the CPU policy where the guest topology is > defined. For the time being, the function simply returns the old > relationship, but will eventually return results consistent with the > topology. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > v2: > * Removed usage of SET_xAPIC_ID(). > * Restored previous logic when exposing leaf 0xb, and gate it for HVM only. > * Rewrote comment in lapic_load_fixup, including the implicit assumption. > * Moved vlapic_cpu_policy_changed() into hvm_cpuid_policy_changed()) > * const-ified policy in vlapic_cpu_policy_changed() > --- > xen/arch/x86/cpuid.c | 15 ++++--------- > xen/arch/x86/hvm/vlapic.c | 30 ++++++++++++++++++++++++-- > xen/arch/x86/include/asm/hvm/hvm.h | 1 + > xen/arch/x86/include/asm/hvm/vlapic.h | 2 ++ > xen/include/public/arch-x86/hvm/save.h | 2 ++ > xen/include/xen/lib/x86/cpu-policy.h | 9 ++++++++ > xen/lib/x86/policy.c | 11 ++++++++++ > 7 files changed, 57 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c > index 7a38e032146a..242c21ec5bb6 100644 > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -139,10 +139,9 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > const struct cpu_user_regs *regs; > > case 0x1: > - /* TODO: Rework topology logic. */ > res->b &= 0x00ffffffu; > if ( is_hvm_domain(d) ) > - res->b |= (v->vcpu_id * 2) << 24; > + res->b |= vlapic_x2apic_id(vcpu_vlapic(v)) << 24; > > /* TODO: Rework vPMU control in terms of toolstack choices. */ > if ( vpmu_available(v) && > @@ -311,19 +310,13 @@ 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 ) > + /* Don't expose topology information to PV guests */ Not sure whether we want to keep part of the comment about exposing x2APIC to guests even when x2APIC is not present in the host. I think this code has changed and the comment is kind of stale now. > + if ( is_hvm_domain(d) && p->basic.x2apic ) > { > *(uint8_t *)&res->c = subleaf; > > /* Fix the x2APIC identifier. */ > - res->d = v->vcpu_id * 2; > + res->d = vlapic_x2apic_id(vcpu_vlapic(v)); > } > break; > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 05072a21bf38..61a96474006b 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1069,7 +1069,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; > uint32_t apic_ldr = x2apic_ldr_from_id(apic_id); > > /* > @@ -1083,6 +1083,22 @@ static void set_x2apic_id(struct vlapic *vlapic) > vlapic_set_reg(vlapic, APIC_LDR, apic_ldr); > } > > +void vlapic_cpu_policy_changed(struct vcpu *v) > +{ > + struct vlapic *vlapic = vcpu_vlapic(v); > + const struct cpu_policy *cp = v->domain->arch.cpu_policy; > + > + /* > + * Don't override the initial x2APIC ID if we have migrated it or > + * if the domain doesn't have vLAPIC at all. > + */ > + if ( !has_vlapic(v->domain) || vlapic->loaded.hw ) > + return; > + > + vlapic->hw.x2apic_id = x86_x2apic_id_from_vcpu_id(cp, v->vcpu_id); > + vlapic_set_reg(vlapic, APIC_ID, SET_xAPIC_ID(vlapic->hw.x2apic_id)); Nit: in case we decide to start APICs in x2APIC mode, might be good to take this into account here and use vlapic_x2apic_mode(vlapic) to select whether SET_xAPIC_ID() needs to be used or not: vlapic_set_reg(vlapic, APIC_ID, vlapic_x2apic_mode(vlapic) ? vlapic->hw.x2apic_id : SET_xAPIC_ID(vlapic->hw.x2apic_id)); Or similar. > +} > + > int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val) > { > const struct cpu_policy *cp = v->domain->arch.cpu_policy; > @@ -1449,7 +1465,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)); > vlapic_do_init(vlapic); > } > > @@ -1514,6 +1530,16 @@ static void lapic_load_fixup(struct vlapic *vlapic) > const struct vcpu *v = vlapic_vcpu(vlapic); > uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id); > > + /* > + * Loading record without hw.x2apic_id in the save stream, calculate using > + * the traditional "vcpu_id * 2" relation. There's an implicit assumption > + * that vCPU0 always has x2APIC0, which is true for the old relation, and > + * still holds under the new x2APIC generation algorithm. While that case > + * goes through the conditional it's benign because it still maps to zero. > + */ > + if ( !vlapic->hw.x2apic_id ) > + vlapic->hw.x2apic_id = v->vcpu_id * 2; > + > /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */ > if ( !vlapic_x2apic_mode(vlapic) || > (vlapic->loaded.ldr == good_ldr) ) > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > index 0c9e6f15645d..e1f0585d75a9 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -448,6 +448,7 @@ static inline void hvm_update_guest_efer(struct vcpu *v) > static inline void hvm_cpuid_policy_changed(struct vcpu *v) > { > alternative_vcall(hvm_funcs.cpuid_policy_changed, v); > + vlapic_cpu_policy_changed(v); Note sure whether this call would better be placed in cpu_policy_updated() inside the is_hvm_vcpu() conditional branch. hvm_cpuid_policy_changed() &c are just wrappers around the hvm_funcs hooks, pulling vlapic functions in there is likely to complicate the header dependencies in the long term. > } > > static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, > diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h > index 88ef94524339..e8d41313abd3 100644 > --- a/xen/arch/x86/include/asm/hvm/vlapic.h > +++ b/xen/arch/x86/include/asm/hvm/vlapic.h > @@ -44,6 +44,7 @@ > #define vlapic_xapic_mode(vlapic) \ > (!vlapic_hw_disabled(vlapic) && \ > !((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD)) > +#define vlapic_x2apic_id(vlapic) ((vlapic)->hw.x2apic_id) > > /* > * Generic APIC bitmap vector update & search routines. > @@ -107,6 +108,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack); > > int vlapic_init(struct vcpu *v); > void vlapic_destroy(struct vcpu *v); > +void vlapic_cpu_policy_changed(struct vcpu *v); > > void vlapic_reset(struct vlapic *vlapic); > > diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h > index 7ecacadde165..1c2ec669ffc9 100644 > --- 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; I think Jan requested for this field to be checked to be 0 for incoming migrations, yet I don't see such check added. > }; > > DECLARE_HVM_SAVE_TYPE(LAPIC, 5, struct hvm_hw_lapic); > diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h > index d5e447e9dc06..392320b9adbe 100644 > --- a/xen/include/xen/lib/x86/cpu-policy.h > +++ b/xen/include/xen/lib/x86/cpu-policy.h > @@ -542,6 +542,15 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, > const struct cpu_policy *guest, > struct cpu_policy_errors *err); > > +/** > + * Calculates the x2APIC ID of a vCPU given a CPU policy > + * > + * @param p CPU policy of the domain. > + * @param id vCPU ID of the vCPU. > + * @returns x2APIC ID of the vCPU. > + */ > +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id); > + > #endif /* !XEN_LIB_X86_POLICIES_H */ > > /* > diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c > index f033d22785be..4cef658feeb8 100644 > --- a/xen/lib/x86/policy.c > +++ b/xen/lib/x86/policy.c > @@ -2,6 +2,17 @@ > > #include <xen/lib/x86/cpu-policy.h> > > +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id) > +{ > + /* > + * TODO: Derive x2APIC ID from the topology information inside `p` > + * rather than from vCPU ID. This bodge is a temporary measure > + * until all infra is in place to retrieve or derive the initial > + * x2APIC ID from migrated domains. > + */ > + return vcpu_id * 2; I don't think this builds? Note the parameter is plain 'id' not 'vcpu_id'. Thanks, Roger.
On 23/05/2024 15:32, Roger Pau Monné wrote: >> 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 ) >> + /* Don't expose topology information to PV guests */ > > Not sure whether we want to keep part of the comment about exposing > x2APIC to guests even when x2APIC is not present in the host. I think > this code has changed and the comment is kind of stale now. The comment is definitely stale. Nowadays x2APIC is fully supported by AMD, as is leaf 0xb. The fact we emulate the x2APIC seems hardly relevant in a CPUID leaf about topology. I could keep a note showing... /* Exposed alongside x2apic, as it's tightly coupled with it */ ... although that's directly implied by the conditional. >> +void vlapic_cpu_policy_changed(struct vcpu *v) >> +{ >> + struct vlapic *vlapic = vcpu_vlapic(v); >> + const struct cpu_policy *cp = v->domain->arch.cpu_policy; >> + >> + /* >> + * Don't override the initial x2APIC ID if we have migrated it or >> + * if the domain doesn't have vLAPIC at all. >> + */ >> + if ( !has_vlapic(v->domain) || vlapic->loaded.hw ) >> + return; >> + >> + vlapic->hw.x2apic_id = x86_x2apic_id_from_vcpu_id(cp, v->vcpu_id); >> + vlapic_set_reg(vlapic, APIC_ID, SET_xAPIC_ID(vlapic->hw.x2apic_id)); > > Nit: in case we decide to start APICs in x2APIC mode, might be good to > take this into account here and use vlapic_x2apic_mode(vlapic) to > select whether SET_xAPIC_ID() needs to be used or not:> > vlapic_set_reg(vlapic, APIC_ID, > vlapic_x2apic_mode(vlapic) ? vlapic->hw.x2apic_id > : SET_xAPIC_ID(vlapic->hw.x2apic_id)); > > Or similar. > I like it. Sure. >> +} >> + >> int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val) >> { >> const struct cpu_policy *cp = v->domain->arch.cpu_policy; >> @@ -1449,7 +1465,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)); >> vlapic_do_init(vlapic); >> } >> >> @@ -1514,6 +1530,16 @@ static void lapic_load_fixup(struct vlapic *vlapic) >> const struct vcpu *v = vlapic_vcpu(vlapic); >> uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id); >> >> + /* >> + * Loading record without hw.x2apic_id in the save stream, calculate using >> + * the traditional "vcpu_id * 2" relation. There's an implicit assumption >> + * that vCPU0 always has x2APIC0, which is true for the old relation, and >> + * still holds under the new x2APIC generation algorithm. While that case >> + * goes through the conditional it's benign because it still maps to zero. >> + */ >> + if ( !vlapic->hw.x2apic_id ) >> + vlapic->hw.x2apic_id = v->vcpu_id * 2; >> + >> /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */ >> if ( !vlapic_x2apic_mode(vlapic) || >> (vlapic->loaded.ldr == good_ldr) ) >> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h >> index 0c9e6f15645d..e1f0585d75a9 100644 >> --- a/xen/arch/x86/include/asm/hvm/hvm.h >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >> @@ -448,6 +448,7 @@ static inline void hvm_update_guest_efer(struct vcpu *v) >> static inline void hvm_cpuid_policy_changed(struct vcpu *v) >> { >> alternative_vcall(hvm_funcs.cpuid_policy_changed, v); >> + vlapic_cpu_policy_changed(v); > > Note sure whether this call would better be placed in > cpu_policy_updated() inside the is_hvm_vcpu() conditional branch. > > hvm_cpuid_policy_changed() &c are just wrappers around the hvm_funcs > hooks, pulling vlapic functions in there is likely to complicate the > header dependencies in the long term. > That's how it was in v1 and I moved it in v2 answering one of Jan's feedback points. I don't mind either way. >> } >> >> static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, >> diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h >> index 88ef94524339..e8d41313abd3 100644 >> --- a/xen/arch/x86/include/asm/hvm/vlapic.h >> +++ b/xen/arch/x86/include/asm/hvm/vlapic.h >> @@ -44,6 +44,7 @@ >> #define vlapic_xapic_mode(vlapic) \ >> (!vlapic_hw_disabled(vlapic) && \ >> !((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD)) >> +#define vlapic_x2apic_id(vlapic) ((vlapic)->hw.x2apic_id) >> >> /* >> * Generic APIC bitmap vector update & search routines. >> @@ -107,6 +108,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack); >> >> int vlapic_init(struct vcpu *v); >> void vlapic_destroy(struct vcpu *v); >> +void vlapic_cpu_policy_changed(struct vcpu *v); >> >> void vlapic_reset(struct vlapic *vlapic); >> >> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h >> index 7ecacadde165..1c2ec669ffc9 100644 >> --- 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; > > I think Jan requested for this field to be checked to be 0 for > incoming migrations, yet I don't see such check added. > It's on the next patch, when the checks are moved into the check hook. Doing it all as part of the same patch seemed dirty. I guess I can invert them. >> }; >> >> DECLARE_HVM_SAVE_TYPE(LAPIC, 5, struct hvm_hw_lapic); >> diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h >> index d5e447e9dc06..392320b9adbe 100644 >> --- a/xen/include/xen/lib/x86/cpu-policy.h >> +++ b/xen/include/xen/lib/x86/cpu-policy.h >> @@ -542,6 +542,15 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, >> const struct cpu_policy *guest, >> struct cpu_policy_errors *err); >> >> +/** >> + * Calculates the x2APIC ID of a vCPU given a CPU policy >> + * >> + * @param p CPU policy of the domain. >> + * @param id vCPU ID of the vCPU. >> + * @returns x2APIC ID of the vCPU. >> + */ >> +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id); >> + >> #endif /* !XEN_LIB_X86_POLICIES_H */ >> >> /* >> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c >> index f033d22785be..4cef658feeb8 100644 >> --- a/xen/lib/x86/policy.c >> +++ b/xen/lib/x86/policy.c >> @@ -2,6 +2,17 @@ >> >> #include <xen/lib/x86/cpu-policy.h> >> >> +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id) >> +{ >> + /* >> + * TODO: Derive x2APIC ID from the topology information inside `p` >> + * rather than from vCPU ID. This bodge is a temporary measure >> + * until all infra is in place to retrieve or derive the initial >> + * x2APIC ID from migrated domains. >> + */ >> + return vcpu_id * 2; > > I don't think this builds? > > Note the parameter is plain 'id' not 'vcpu_id'. > > Thanks, Roger. Bah. mid-patches after v1 don't get much attention. Ack. Cheers, Alejandro
On Fri, May 24, 2024 at 11:58:44AM +0100, Alejandro Vallejo wrote: > On 23/05/2024 15:32, Roger Pau Monné wrote: > >> 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 ) > >> + /* Don't expose topology information to PV guests */ > > > > Not sure whether we want to keep part of the comment about exposing > > x2APIC to guests even when x2APIC is not present in the host. I think > > this code has changed and the comment is kind of stale now. > > The comment is definitely stale. Nowadays x2APIC is fully supported by > AMD, as is leaf 0xb. The fact we emulate the x2APIC seems hardly > relevant in a CPUID leaf about topology. I could keep a note showing... > > /* Exposed alongside x2apic, as it's tightly coupled with it */ > > ... although that's directly implied by the conditional. Yeah, I haven't gone through the history of this file, but I bet at some point before the introduction of CPUID policies we leaked (part of) the host CPUID contents in here. It's also no longer true that the leaf is Intel only. I fine either adding your newly proposed comment, or leaving it as-is. > >> +} > >> + > >> int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val) > >> { > >> const struct cpu_policy *cp = v->domain->arch.cpu_policy; Le> >> @@ -1449,7 +1465,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)); > >> vlapic_do_init(vlapic); > >> } > >> > >> @@ -1514,6 +1530,16 @@ static void lapic_load_fixup(struct vlapic *vlapic) > >> const struct vcpu *v = vlapic_vcpu(vlapic); > >> uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id); > >> > >> + /* > >> + * Loading record without hw.x2apic_id in the save stream, calculate using > >> + * the traditional "vcpu_id * 2" relation. There's an implicit assumption > >> + * that vCPU0 always has x2APIC0, which is true for the old relation, and > >> + * still holds under the new x2APIC generation algorithm. While that case > >> + * goes through the conditional it's benign because it still maps to zero. > >> + */ > >> + if ( !vlapic->hw.x2apic_id ) > >> + vlapic->hw.x2apic_id = v->vcpu_id * 2; > >> + > >> /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */ > >> if ( !vlapic_x2apic_mode(vlapic) || > >> (vlapic->loaded.ldr == good_ldr) ) > >> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > >> index 0c9e6f15645d..e1f0585d75a9 100644 > >> --- a/xen/arch/x86/include/asm/hvm/hvm.h > >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h > >> @@ -448,6 +448,7 @@ static inline void hvm_update_guest_efer(struct vcpu *v) > >> static inline void hvm_cpuid_policy_changed(struct vcpu *v) > >> { > >> alternative_vcall(hvm_funcs.cpuid_policy_changed, v); > >> + vlapic_cpu_policy_changed(v); > > > > Note sure whether this call would better be placed in > > cpu_policy_updated() inside the is_hvm_vcpu() conditional branch. > > > > hvm_cpuid_policy_changed() &c are just wrappers around the hvm_funcs > > hooks, pulling vlapic functions in there is likely to complicate the > > header dependencies in the long term. > > > > That's how it was in v1 and I moved it in v2 answering one of Jan's > feedback points. > > I don't mind either way. Oh (goes and reads Jan's reply to v1) I see. Let's leave it as-is then. > > >> } > >> > >> static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, > >> diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h > >> index 88ef94524339..e8d41313abd3 100644 > >> --- a/xen/arch/x86/include/asm/hvm/vlapic.h > >> +++ b/xen/arch/x86/include/asm/hvm/vlapic.h > >> @@ -44,6 +44,7 @@ > >> #define vlapic_xapic_mode(vlapic) \ > >> (!vlapic_hw_disabled(vlapic) && \ > >> !((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD)) > >> +#define vlapic_x2apic_id(vlapic) ((vlapic)->hw.x2apic_id) > >> > >> /* > >> * Generic APIC bitmap vector update & search routines. > >> @@ -107,6 +108,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack); > >> > >> int vlapic_init(struct vcpu *v); > >> void vlapic_destroy(struct vcpu *v); > >> +void vlapic_cpu_policy_changed(struct vcpu *v); > >> > >> void vlapic_reset(struct vlapic *vlapic); > >> > >> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h > >> index 7ecacadde165..1c2ec669ffc9 100644 > >> --- 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; > > > > I think Jan requested for this field to be checked to be 0 for > > incoming migrations, yet I don't see such check added. > > > > It's on the next patch, when the checks are moved into the check hook. > Doing it all as part of the same patch seemed dirty. I guess I can > invert them. Yeah, sorry, realized later. I think the introduction of the lapic record checking wants to be the first patch in the series, so that when you introduce the field here you can check it's zero. Thanks, Roger.
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 7a38e032146a..242c21ec5bb6 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -139,10 +139,9 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, const struct cpu_user_regs *regs; case 0x1: - /* TODO: Rework topology logic. */ res->b &= 0x00ffffffu; if ( is_hvm_domain(d) ) - res->b |= (v->vcpu_id * 2) << 24; + res->b |= vlapic_x2apic_id(vcpu_vlapic(v)) << 24; /* TODO: Rework vPMU control in terms of toolstack choices. */ if ( vpmu_available(v) && @@ -311,19 +310,13 @@ 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 ) + /* Don't expose topology information to PV guests */ + if ( is_hvm_domain(d) && p->basic.x2apic ) { *(uint8_t *)&res->c = subleaf; /* Fix the x2APIC identifier. */ - res->d = v->vcpu_id * 2; + res->d = vlapic_x2apic_id(vcpu_vlapic(v)); } break; diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 05072a21bf38..61a96474006b 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1069,7 +1069,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; uint32_t apic_ldr = x2apic_ldr_from_id(apic_id); /* @@ -1083,6 +1083,22 @@ static void set_x2apic_id(struct vlapic *vlapic) vlapic_set_reg(vlapic, APIC_LDR, apic_ldr); } +void vlapic_cpu_policy_changed(struct vcpu *v) +{ + struct vlapic *vlapic = vcpu_vlapic(v); + const struct cpu_policy *cp = v->domain->arch.cpu_policy; + + /* + * Don't override the initial x2APIC ID if we have migrated it or + * if the domain doesn't have vLAPIC at all. + */ + if ( !has_vlapic(v->domain) || vlapic->loaded.hw ) + return; + + vlapic->hw.x2apic_id = x86_x2apic_id_from_vcpu_id(cp, v->vcpu_id); + vlapic_set_reg(vlapic, APIC_ID, SET_xAPIC_ID(vlapic->hw.x2apic_id)); +} + int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val) { const struct cpu_policy *cp = v->domain->arch.cpu_policy; @@ -1449,7 +1465,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)); vlapic_do_init(vlapic); } @@ -1514,6 +1530,16 @@ static void lapic_load_fixup(struct vlapic *vlapic) const struct vcpu *v = vlapic_vcpu(vlapic); uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id); + /* + * Loading record without hw.x2apic_id in the save stream, calculate using + * the traditional "vcpu_id * 2" relation. There's an implicit assumption + * that vCPU0 always has x2APIC0, which is true for the old relation, and + * still holds under the new x2APIC generation algorithm. While that case + * goes through the conditional it's benign because it still maps to zero. + */ + if ( !vlapic->hw.x2apic_id ) + vlapic->hw.x2apic_id = v->vcpu_id * 2; + /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */ if ( !vlapic_x2apic_mode(vlapic) || (vlapic->loaded.ldr == good_ldr) ) diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 0c9e6f15645d..e1f0585d75a9 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -448,6 +448,7 @@ static inline void hvm_update_guest_efer(struct vcpu *v) static inline void hvm_cpuid_policy_changed(struct vcpu *v) { alternative_vcall(hvm_funcs.cpuid_policy_changed, v); + vlapic_cpu_policy_changed(v); } static inline void hvm_set_tsc_offset(struct vcpu *v, uint64_t offset, diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h index 88ef94524339..e8d41313abd3 100644 --- a/xen/arch/x86/include/asm/hvm/vlapic.h +++ b/xen/arch/x86/include/asm/hvm/vlapic.h @@ -44,6 +44,7 @@ #define vlapic_xapic_mode(vlapic) \ (!vlapic_hw_disabled(vlapic) && \ !((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD)) +#define vlapic_x2apic_id(vlapic) ((vlapic)->hw.x2apic_id) /* * Generic APIC bitmap vector update & search routines. @@ -107,6 +108,7 @@ int vlapic_ack_pending_irq(struct vcpu *v, int vector, bool force_ack); int vlapic_init(struct vcpu *v); void vlapic_destroy(struct vcpu *v); +void vlapic_cpu_policy_changed(struct vcpu *v); void vlapic_reset(struct vlapic *vlapic); diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index 7ecacadde165..1c2ec669ffc9 100644 --- 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; }; DECLARE_HVM_SAVE_TYPE(LAPIC, 5, struct hvm_hw_lapic); diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h index d5e447e9dc06..392320b9adbe 100644 --- a/xen/include/xen/lib/x86/cpu-policy.h +++ b/xen/include/xen/lib/x86/cpu-policy.h @@ -542,6 +542,15 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host, const struct cpu_policy *guest, struct cpu_policy_errors *err); +/** + * Calculates the x2APIC ID of a vCPU given a CPU policy + * + * @param p CPU policy of the domain. + * @param id vCPU ID of the vCPU. + * @returns x2APIC ID of the vCPU. + */ +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id); + #endif /* !XEN_LIB_X86_POLICIES_H */ /* diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c index f033d22785be..4cef658feeb8 100644 --- a/xen/lib/x86/policy.c +++ b/xen/lib/x86/policy.c @@ -2,6 +2,17 @@ #include <xen/lib/x86/cpu-policy.h> +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id) +{ + /* + * TODO: Derive x2APIC ID from the topology information inside `p` + * rather than from vCPU ID. This bodge is a temporary measure + * until all infra is in place to retrieve or derive the initial + * x2APIC ID from migrated domains. + */ + return vcpu_id * 2; +} + int x86_cpu_policies_are_compatible(const struct cpu_policy *host, const struct cpu_policy *guest, struct cpu_policy_errors *err)
This allows the initial x2APIC ID to be sent on the migration stream. The hardcoded mapping x2apic_id=2*vcpu_id is maintained for the time being. Given the vlapic data is zero-extended on restore, fix up migrations from hosts without the field by setting it to the old convention if zero. x2APIC IDs are calculated from the CPU policy where the guest topology is defined. For the time being, the function simply returns the old relationship, but will eventually return results consistent with the topology. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * Removed usage of SET_xAPIC_ID(). * Restored previous logic when exposing leaf 0xb, and gate it for HVM only. * Rewrote comment in lapic_load_fixup, including the implicit assumption. * Moved vlapic_cpu_policy_changed() into hvm_cpuid_policy_changed()) * const-ified policy in vlapic_cpu_policy_changed() --- xen/arch/x86/cpuid.c | 15 ++++--------- xen/arch/x86/hvm/vlapic.c | 30 ++++++++++++++++++++++++-- xen/arch/x86/include/asm/hvm/hvm.h | 1 + xen/arch/x86/include/asm/hvm/vlapic.h | 2 ++ xen/include/public/arch-x86/hvm/save.h | 2 ++ xen/include/xen/lib/x86/cpu-policy.h | 9 ++++++++ xen/lib/x86/policy.c | 11 ++++++++++ 7 files changed, 57 insertions(+), 13 deletions(-)