Mailing List Archive: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area

[PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area

Jan 9, 2024, 7:38 AM

Post #1 of 8 (25 views)

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>
---
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)));

/* 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));
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.
+ */
+ 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;
};

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;
+}
+
int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
const struct cpu_policy *guest,
struct cpu_policy_errors *err)
--
2.34.1

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

roger.pau at citrix

Mar 19, 2024, 9:20 AM

Post #2 of 8 (19 views)

Permalink

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@cloud.com>
> ---
> 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).

>
> /* 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.

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.

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

> + */
> + 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.

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

roger.pau at citrix

Mar 20, 2024, 2:33 AM

Post #3 of 8 (19 views)

Permalink

On Tue, Mar 19, 2024 at 05:20:35PM +0100, 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@cloud.com>
> > ---
> > 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).
>
> >
> > /* 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.
>
> Why not just return the x2apic_id field from the cpu_policy object?
> (topo.subleaf[X].x2apic_id)

Scratch that, the cpu policy is per-domain, not per-vcpu, and hence
cannot hold the x{,2}apic IDs.

> 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.
>
> > 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.
>
> > + */
> > + 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?

Since getting from the cpu_policy is not possible, what about getting
it from the registers itself? It's already present in hvm_hw_lapic_regs.

Regards, Roger.

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

alejandro.vallejo at cloud

Mar 25, 2024, 8:44 AM

Post #4 of 8 (8 views)

Permalink

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@cloud.com>
>> ---
>> 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

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

jbeulich at suse

Mar 25, 2024, 9:45 AM

Post #5 of 8 (8 views)

Permalink

On 09.01.2024 16:38, Alejandro Vallejo wrote:
> --- 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);
> + }
> }

This is a layering violation imo; hvm_cpuid_policy_changed() wants
to call vlapic_cpu_policy_changed().

> @@ -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;

const please

> @@ -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.
> + */
> + if ( !vlapic->hw.x2apic_id )
> + vlapic->hw.x2apic_id = v->vcpu_id * 2;

This looks to depend upon it only ever being vCPU which may get a (new
style) APIC ID of 0. I think such at least wants mentioning in the
comment.

> --- 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 can't spot any checking of this last field indeed being zero.

Jan

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

alejandro.vallejo at cloud

Mar 25, 2024, 9:56 AM

Post #6 of 8 (8 views)

Permalink

On 20/03/2024 09:33, Roger Pau Monné wrote:
>>
>> Why not just return the x2apic_id field from the cpu_policy object?
>> (topo.subleaf[X].x2apic_id)
>
> Scratch that, the cpu policy is per-domain, not per-vcpu, and hence
> cannot hold the x{,2}apic IDs.
Yes, that :)

Originally I tried to make the policy per-vCPU, tbf, but that's a very,
very deep and complicated hole that was very hard to deal with and I'm
not tempted to try that again.

>>> 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?
>
> Since getting from the cpu_policy is not possible, what about getting
> it from the registers itself? It's already present in hvm_hw_lapic_regs.
>
> Regards, Roger.

If every APIC is in x2APIC mode, yes. But if any of them is in xAPIC
mode there's problems. The xAPIC ID is overridable by the guest simply
by writing into it, so it's tricky to know whether it's sane or not.
This new field is effectively an immutable "initial APIC ID", which we
can recreate to the old convention when not present in the stream.

If you can think of an alternative that doesn't involve adding a field
or fixing in stone the mapping strategy I'm happy to do that instead,
but I think this is the lesser evil.

Cheers,
Alejandro

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

alejandro.vallejo at cloud

Mar 25, 2024, 11:00 AM

Post #7 of 8 (7 views)

Permalink

Hi,

On 25/03/2024 16:45, Jan Beulich wrote:
> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>> --- 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);
>> + }
>> }
>
> This is a layering violation imo; hvm_cpuid_policy_changed() wants
> to call vlapic_cpu_policy_changed().

Sure.

>
>> @@ -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;
>
> const please

Ack

>
>> @@ -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.
>> + */
>> + if ( !vlapic->hw.x2apic_id )
>> + vlapic->hw.x2apic_id = v->vcpu_id * 2;
>
> This looks to depend upon it only ever being vCPU which may get a (new
> style) APIC ID of 0. I think such at least wants mentioning in the
> comment.

I don't quite follow you, I'm afraid. There is an implicit control flow
assumption that I can extract into a comment (I assume you were going
for that angle?). The implicit assumption that "vCPU0 always has
APIC_ID=0", which makes vCPU0 go through that path even when no
corrections are necessary. It's benign because it resolves to APIC_ID 0.

Is that what you meant? If so, I'll add it to v2.

>
>> --- 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 can't spot any checking of this last field indeed being zero.
>
> Jan

Huh. I was sure I zeroed that on vlapic_init(), but it must've been on a
previous discarded series. Good catch.

Do we also want a check on migrate so a migration from a future Xen in
which it's not zero fails?

Cheers,
Alejandro

Re: [PATCH 1/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area [ In reply to ]

jbeulich at suse

Mar 26, 2024, 12:16 AM

Post #8 of 8 (7 views)

Permalink

On 25.03.2024 19:00, Alejandro Vallejo wrote:
> On 25/03/2024 16:45, Jan Beulich wrote:
>> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>>> @@ -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.
>>> + */
>>> + if ( !vlapic->hw.x2apic_id )
>>> + vlapic->hw.x2apic_id = v->vcpu_id * 2;
>>
>> This looks to depend upon it only ever being vCPU which may get a (new
>> style) APIC ID of 0. I think such at least wants mentioning in the
>> comment.
>
> I don't quite follow you, I'm afraid. There is an implicit control flow
> assumption that I can extract into a comment (I assume you were going
> for that angle?). The implicit assumption that "vCPU0 always has
> APIC_ID=0", which makes vCPU0 go through that path even when no
> corrections are necessary. It's benign because it resolves to APIC_ID 0.
>
> Is that what you meant? If so, I'll add it to v2.

Yes, and even in your reply you make the same assumption without further
explanation. It does not go without saying that vCPU 0 necessarily has
APIC ID 0. On bare metal that may be what one can typically observe, but
I'm unaware of any architectural guarantees to this effect.

>>> --- 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 can't spot any checking of this last field indeed being zero.
>
> Huh. I was sure I zeroed that on vlapic_init(), but it must've been on a
> previous discarded series. Good catch.

No, explicit zeroing isn't needed, simply because all of struct vcpu
starts out zeroed.

> Do we also want a check on migrate so a migration from a future Xen in
> which it's not zero fails?

It's really only this that I meant. Recall we now even have a dedicated
checking hook, running ahead of any state loading.

Jan