|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
Hi,
I'll answer the policy-related feedback in the next email, as you
corrected yourself for that.
On 19/03/2024 16:20, Roger Pau Monné wrote:
> On Tue, Jan 09, 2024 at 03:38:29PM +0000, 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@xxxxxxxxx>
>> ---
>> xen/arch/x86/cpuid.c | 20 ++++---------------
>> xen/arch/x86/domain.c | 3 +++
>> xen/arch/x86/hvm/vlapic.c | 27 ++++++++++++++++++++++++--
>> 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, 56 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>> index 7290a979c6..6e259785d0 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 |= SET_xAPIC_ID(vlapic_x2apic_id(vcpu_vlapic(v)));
>
> SET_xAPIC_ID() was intended to be used with the APIC_ID register,
> which also shifts the ID. Not sure it's logically correct to use
> here, even if functionally equivalent (as is shifts left by 24).
Ack.
>
>>
>> /* TODO: Rework vPMU control in terms of toolstack choices. */
>> if ( vpmu_available(v) &&
>> @@ -311,20 +310,9 @@ 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 )
>> - {
>> - *(uint8_t *)&res->c = subleaf;
>> -
>> - /* Fix the x2APIC identifier. */
>> - res->d = v->vcpu_id * 2;
>> - }
>> + /* ecx != 0 if the subleaf is implemented */
>> + if ( res->c && p->basic.x2apic )
>> + res->d = vlapic_x2apic_id(vcpu_vlapic(v));
>
> This needs to be protected so it's reachable by HVM guests only,
> otherwise you will dereference v->arch.hvm.vlapic on a PV vCPU if it
> happens to have p->basic.x2apic set.
Very true. Ack.
>
> Why not just return the x2apic_id field from the cpu_policy object?
> (topo.subleaf[X].x2apic_id)
>
> Also, I'm not sure I get why the setting of res->d is gated on res->c
> != 0, won't res->c be 0 when the guest %ecx is 0, yet %edx must be
> valid for all %ecx inputs, the SDM states:
>
> "The EDX output of leaf 0BH is always valid and does not vary with
> input value in ECX."
>
> I think you need to keep the previous logic that doesn't gate setting
> ->d on anything other than p->basic.x2apic.
>
Ack.
Real HW says you're right. Oddly enough that quote is (AFAICS) for leaf
1FH, but it appears to also hold for 0BH.
>> break;
>>
>> case XSTATE_CPUID:
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 8a31d18f69..e0c7ed8d5d 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -288,7 +288,10 @@ void update_guest_memory_policy(struct vcpu *v,
>> static void cpu_policy_updated(struct vcpu *v)
>> {
>> if ( is_hvm_vcpu(v) )
>> + {
>> hvm_cpuid_policy_changed(v);
>> + vlapic_cpu_policy_changed(v);
>> + }
>> }
>>
>> void domain_cpu_policy_changed(struct domain *d)
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index cdb69d9742..f500d66543 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);
>> + 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,13 @@ 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);
>>
>> + /*
>> + * Guest with hardcoded assumptions about x2apic_id <-> vcpu_id
>> + * mappings. Recreate the mapping it used to have in old host.
>
> Wouldn't it be more appropriate to state "Loading record without
> hw.x2apic_id in the save stream, calculate using the vcpu_id * 2
> relation" or some such.>
> Current comment makes it looks like the guest has some kind of
> restriction with this relation, but that's just an internal Xen
> limitation.
Sure.
>
>> + */
>> + 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/vlapic.h
>> b/xen/arch/x86/include/asm/hvm/vlapic.h
>> index 88ef945243..e8d41313ab 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 7ecacadde1..1c2ec669ff 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;
>
> Do we really to add a new field, couldn't we get the lapic IDs from
> the cpu_policy>
>> };
>>
>> 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 d5e447e9dc..14724cedff 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 vcpu_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
>> vcpu_id);
>> +
>> #endif /* !XEN_LIB_X86_POLICIES_H */
>>
>> /*
>> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
>> index f033d22785..a3b24e6879 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
>> vcpu_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;
>
> As noted above, won't a suitable initial step would be to populate the
> apic_id and x2apic_id fields in struct cpu_policy with this relation
> (x{,2}apic_id == vcpu_id * 2), and avoid this extra handler?
>
> Thanks, Roger.
Cheers,
Alejandro
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |