* [PATCH v6 00/11] x86: Expose consistent topology to guests
@ 2024-10-01 12:37 Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
` (10 more replies)
0 siblings, 11 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:37 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Juergen Gross
(original cover letter at the bottom)
The series wasn't taking proper precautions to not break PVH. In particular,
because new APIC IDs break with the old convention, PVH is not something that
can be left for later as it otherwise suffers a mismatch between APIC IDs in
the vLAPICs and the MADT table.
This version is a rebased v5 with the additional fixes of:
1. PVH should now work, because libacpi was refactored to stop taking a
function pointer and start taking a LUT for the cpu->apic_id mapping.
2. Expose leaf 0xb to guests even on hosts that don't themselves do so.
(e.g: AMD Lisbon). Otherwise all such hosts are unable to create
guests with this patch series on, and there's no good reason not to do so.
Hypervisor prerequisites:
patch 1: lib/x86: Relax checks about policy compatibility
* new patch to properly operate (after this series) on older AMD hardware.
patch 2: x86/vlapic: Move lapic migration checks to the check hooks
* Same as in v5
patch 3: xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
* Same as in v5.
patch 4: xen/x86: Add supporting code for uploading LAPIC contexts during
domain create
* Same as in v5.
hvmloader prerequisites
patch 5: tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
* Same as in v5.
Toolstack prerequisites:
patch 6: tools/libacpi: Use LUT of APIC IDs rather than function pointer
* new patch to enable proper topology reporting to PVH guests.
patch 7: tools/libguest: Always set vCPU context in vcpu_hvm()
* Same as in v5.
No functional changes:
patch 8: xen/lib: Add topology generator for x86
* Same as in v5.
patch 9: xen/x86: Derive topologically correct x2APIC IDs from the policy
* Same as in v5.
Final toolstack/xen stitching:
patch 10: tools/libguest: Set distinct x2APIC IDs for each vCPU
* Unlikely in v5, this patch takes the APIC IDs from a LUT stored in
the xc_dom_image structure.
patch 11: xen/x86: Synthesise domain topologies
* Same as v5.
====================================================================
v5: https://lore.kernel.org/xen-devel/20240808134251.29995-1-alejandro.vallejo@cloud.com/
v4 -> v5:
Largely unchanged and resent for review after the 4.20 dev cycle started.
* Addressed Andrew's nits in v4/patch1
* Addressed Jan's concern with MTRR overrides in v4/patch6 by keeping the
same MTRR data in the vCPU contexts for HVM domain creations.
v4: https://lore.kernel.org/xen-devel/cover.1719416329.git.alejandro.vallejo@cloud.com/
v3 -> v4:
* Fixed cpuid() bug in hvmloader, causing UB in v3
* Fixed a bogus assert in hvmloader, also causing a crash in v3
* Used HVM contexts rather than sync'd algos between Xen and toolstack in
order to initialise the per-vCPU LAPIC state.
* Formatting asjustments.
v3: https://lore.kernel.org/xen-devel/cover.1716976271.git.alejandro.vallejo@cloud.com/
v2 -> v3:
(v2/patch2 and v2/patch4 are already committed)
* Moved the vlapic check hook addition to v3/patch1
* And created a check hook for the architectural state too for consistency.
* Fixed migrations from Xen <= 4.13 by reconstructing the previous topology.
* Correctly set the APIC ID after a policy change when vlapic is already in
x2APIC mode.
* Removed bogus assumption introduced in v1 and v2 on hvmloader about which
8bit APIC IDs represent ids > 254. (it's "id % 0xff", not "min(id, 0xff)".
* Used an x2apic flag check instead.
* Various formatting adjustments.
v2: https://lore.kernel.org/xen-devel/cover.1715102098.git.alejandro.vallejo@cloud.com/
v1 -> v2:
* v1/patch 4 replaced by a different strategy (See patches 4 and 5 in v2):
* Have hvmloader populate MADT with the real APIC IDs as read by the APs
themselves rather than giving it knowledge on how to derive them.
* Removed patches 2 and 3 in v1, as no longer relevant.
* Split v1/patch6 in two parts ((a) creating the generator and (b) plugging it
in) and use the generator in the unit tests of the vcpuid->apicid mapping
function. Becomes patches 6 and 8 in v2.
Patch 1: Same as v1/patch1.
Patch 2: Header dependency cleanup in preparation for patch3.
Patch 3: Adds vlapic_hidden check for the newly introduced reserved area.
Patch 4: [hvmloader] Replaces INIT+SIPI+SIPI sequences with hypercalls.
Patch 5: [hvmloader] Retrieve the per-CPU APIC IDs from the APs themselves.
Patch 6: Split from v1/patch6.
Patch 7: Logically matching v1/patch5, but using v2/patch6 for testing.
Patch 8: Split from v1/patch6.
v1: https://lore.kernel.org/xen-devel/20240109153834.4192-1-alejandro.vallejo@cloud.com/
=== Original cover letter ===
Current topology handling is close to non-existent. As things stand, APIC
IDs are allocated through the apic_id=vcpu_id*2 relation without giving any
hints to the OS on how to parse the x2APIC ID of a given CPU and assuming
the guest will assume 2 threads per core.
This series involves bringing x2APIC IDs into the migration stream, so
older guests keep operating as they used to and enhancing Xen+toolstack so
new guests get topology information consistent with their x2APIC IDs. As a
side effect of this, x2APIC IDs are now packed and don't have (unless under
a pathological case) gaps.
Further work ought to allow combining this topology configurations with
gang-scheduling of guest hyperthreads into affine physical hyperthreads.
For the time being it purposefully keeps the configuration of "1 socket" +
"1 thread per core" + "1 core per vCPU".
Patch 1: Includes x2APIC IDs in the migration stream. This allows Xen to
reconstruct the right x2APIC IDs on migrated-in guests, and
future-proofs itself in the face of x2APIC ID derivation changes.
Patch 2: Minor refactor to expose xc_cpu_policy in libxl
Patch 3: Refactors xen/lib/x86 to work on non-Xen freestanding environments
(e.g: hvmloader)
Patch 4: Remove old assumptions about vcpu_id<->apic_id relationship in hvmloader
Patch 5: Add logic to derive x2APIC IDs given a CPU policy and vCPU IDs
Patch 6: Includes a simple topology generator for toolstack so new guests
have topologically consistent information in CPUID
===================================================================
v6:
Alejandro Vallejo (11):
lib/x86: Relax checks about policy compatibility
x86/vlapic: Move lapic migration checks to the check hooks
xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
xen/x86: Add supporting code for uploading LAPIC contexts during
domain create
tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
tools/libacpi: Use LUT of APIC IDs rather than function pointer
tools/libguest: Always set vCPU context in vcpu_hvm()
xen/lib: Add topology generator for x86
xen/x86: Derive topologically correct x2APIC IDs from the policy
tools/libguest: Set distinct x2APIC IDs for each vCPU
xen/x86: Synthesise domain topologies
tools/firmware/hvmloader/config.h | 5 +-
tools/firmware/hvmloader/hvmloader.c | 6 +-
tools/firmware/hvmloader/mp_tables.c | 4 +-
tools/firmware/hvmloader/smp.c | 54 ++++--
tools/firmware/hvmloader/util.c | 7 +-
tools/include/xen-tools/common-macros.h | 5 +
tools/include/xenguest.h | 8 +
tools/libacpi/build.c | 6 +-
tools/libacpi/libacpi.h | 2 +-
tools/libs/guest/xg_cpuid_x86.c | 29 +++-
tools/libs/guest/xg_dom_x86.c | 93 ++++++----
tools/libs/light/libxl_dom.c | 25 +++
tools/libs/light/libxl_x86_acpi.c | 7 +-
tools/tests/cpu-policy/test-cpu-policy.c | 207 ++++++++++++++++++++++-
xen/arch/x86/cpu-policy.c | 9 +-
xen/arch/x86/cpuid.c | 14 +-
xen/arch/x86/hvm/vlapic.c | 126 ++++++++++----
xen/arch/x86/include/asm/hvm/vlapic.h | 1 +
xen/include/public/arch-x86/hvm/save.h | 2 +
xen/include/xen/lib/x86/cpu-policy.h | 27 +++
xen/lib/x86/policy.c | 175 ++++++++++++++++++-
xen/lib/x86/private.h | 4 +
22 files changed, 704 insertions(+), 112 deletions(-)
--
2.46.0
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
@ 2024-10-01 12:37 ` Alejandro Vallejo
2024-10-09 9:40 ` Jan Beulich
2024-10-09 21:58 ` Andrew Cooper
2024-10-01 12:37 ` [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks Alejandro Vallejo
` (9 subsequent siblings)
10 siblings, 2 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:37 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD
Allow a guest policy have up to leaf 0xb even if the host doesn't.
Otherwise it's not possible to show leaf 0xb to guests we're emulating
an x2APIC for on old AMD machines.
No externally visible changes though because toolstack doesn't yet
populate that leaf.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/tests/cpu-policy/test-cpu-policy.c | 6 +++++-
xen/lib/x86/policy.c | 11 ++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 301df2c00285..9216010b1c5d 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -586,6 +586,10 @@ static void test_is_compatible_success(void)
.platform_info.cpuid_faulting = true,
},
},
+ {
+ .name = "Host missing leaf 0xb, Guest wanted",
+ .guest.basic.max_leaf = 0xb,
+ },
};
struct cpu_policy_errors no_errors = INIT_CPU_POLICY_ERRORS;
@@ -614,7 +618,7 @@ static void test_is_compatible_failure(void)
} tests[] = {
{
.name = "Host basic.max_leaf out of range",
- .guest.basic.max_leaf = 1,
+ .guest.basic.max_leaf = 0xc,
.e = { 0, -1, -1 },
},
{
diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
index f033d22785be..63bc96451d2c 100644
--- a/xen/lib/x86/policy.c
+++ b/xen/lib/x86/policy.c
@@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
#define FAIL_MSR(m) \
do { e.msr = (m); goto out; } while ( 0 )
- if ( guest->basic.max_leaf > host->basic.max_leaf )
+ /*
+ * Old AMD hardware doesn't expose topology information in leaf 0xb. We
+ * want to emulate that leaf with credible information because it must be
+ * present on systems in which we emulate the x2APIC.
+ *
+ * For that reason, allow the max basic guest leaf to be larger than the
+ * hosts' up until 0xb.
+ */
+ if ( guest->basic.max_leaf > 0xb &&
+ guest->basic.max_leaf > host->basic.max_leaf )
FAIL_CPUID(0, NA);
if ( guest->feat.max_subleaf > host->feat.max_subleaf )
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
@ 2024-10-01 12:37 ` Alejandro Vallejo
2024-10-08 15:41 ` Jan Beulich
2024-10-01 12:37 ` [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area Alejandro Vallejo
` (8 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:37 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné
While doing this, factor out checks common to architectural and hidden
state.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--
Last reviewed in the topology series v3. Fell under the cracks.
https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/
---
xen/arch/x86/hvm/vlapic.c | 84 ++++++++++++++++++++++++++-------------
1 file changed, 56 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 992355e511cd..101902cff889 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1571,60 +1571,88 @@ static void lapic_load_fixup(struct vlapic *vlapic)
v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr);
}
-static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
-{
- unsigned int vcpuid = hvm_load_instance(h);
- struct vcpu *v;
- struct vlapic *s;
+static int lapic_check_common(const struct domain *d, unsigned int vcpuid)
+{
if ( !has_vlapic(d) )
return -ENODEV;
/* Which vlapic to load? */
- if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
+ if ( !domain_vcpu(d, vcpuid) )
{
- dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
+ dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vCPU %u\n",
d->domain_id, vcpuid);
return -EINVAL;
}
- s = vcpu_vlapic(v);
+
+ return 0;
+}
+
+static int cf_check lapic_check_hidden(const struct domain *d,
+ hvm_domain_context_t *h)
+{
+ unsigned int vcpuid = hvm_load_instance(h);
+ struct hvm_hw_lapic s;
+ int rc = lapic_check_common(d, vcpuid);
+
+ if ( rc )
+ return rc;
+
+ if ( hvm_load_entry_zeroextend(LAPIC, h, &s) != 0 )
+ return -ENODATA;
+
+ /* EN=0 with EXTD=1 is illegal */
+ if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) ==
+ APIC_BASE_EXTD )
+ return -EINVAL;
+
+ return 0;
+}
+
+static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
+{
+ unsigned int vcpuid = hvm_load_instance(h);
+ struct vcpu *v = d->vcpu[vcpuid];
+ struct vlapic *s = vcpu_vlapic(v);
if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
+ {
+ ASSERT_UNREACHABLE();
return -EINVAL;
+ }
s->loaded.hw = 1;
if ( s->loaded.regs )
lapic_load_fixup(s);
- if ( !(s->hw.apic_base_msr & APIC_BASE_ENABLE) &&
- unlikely(vlapic_x2apic_mode(s)) )
- return -EINVAL;
-
hvm_update_vlapic_mode(v);
return 0;
}
-static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
+static int cf_check lapic_check_regs(const struct domain *d,
+ hvm_domain_context_t *h)
{
unsigned int vcpuid = hvm_load_instance(h);
- struct vcpu *v;
- struct vlapic *s;
+ int rc;
- if ( !has_vlapic(d) )
- return -ENODEV;
+ if ( (rc = lapic_check_common(d, vcpuid)) )
+ return rc;
- /* Which vlapic to load? */
- if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
- {
- dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n",
- d->domain_id, vcpuid);
- return -EINVAL;
- }
- s = vcpu_vlapic(v);
+ if ( !hvm_get_entry(LAPIC_REGS, h) )
+ return -ENODATA;
+
+ return 0;
+}
+
+static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
+{
+ unsigned int vcpuid = hvm_load_instance(h);
+ struct vcpu *v = d->vcpu[vcpuid];
+ struct vlapic *s = vcpu_vlapic(v);
if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 )
- return -EINVAL;
+ ASSERT_UNREACHABLE();
s->loaded.id = vlapic_get_reg(s, APIC_ID);
s->loaded.ldr = vlapic_get_reg(s, APIC_LDR);
@@ -1641,9 +1669,9 @@ static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h)
return 0;
}
-HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL,
+HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, lapic_check_hidden,
lapic_load_hidden, 1, HVMSR_PER_VCPU);
-HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL,
+HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_check_regs,
lapic_load_regs, 1, HVMSR_PER_VCPU);
int vlapic_init(struct vcpu *v)
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks Alejandro Vallejo
@ 2024-10-01 12:37 ` Alejandro Vallejo
2024-10-09 13:12 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create Alejandro Vallejo
` (7 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:37 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné
This allows the initial x2APIC ID to be sent on the migration stream.
This allows further changes to topology and APIC ID assignment without
breaking existing hosts. 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.
The hardcoded mapping x2apic_id=2*vcpu_id is kept for the time being,
but it's meant to be overriden by toolstack on a later patch with
appropriate values.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
xen/arch/x86/cpuid.c | 14 +++++---------
xen/arch/x86/hvm/vlapic.c | 22 ++++++++++++++++++++--
xen/arch/x86/include/asm/hvm/vlapic.h | 1 +
xen/include/public/arch-x86/hvm/save.h | 2 ++
4 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 2a777436ee27..dcbdeabadce9 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -138,10 +138,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,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 )
{
*(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 101902cff889..02570f9dd63a 100644
--- 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;
uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
/*
@@ -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));
vlapic_do_init(vlapic);
}
@@ -1538,6 +1538,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) )
@@ -1606,6 +1616,13 @@ static int cf_check lapic_check_hidden(const struct domain *d,
APIC_BASE_EXTD )
return -EINVAL;
+ /*
+ * Fail migrations from newer versions of Xen where
+ * rsvd_zero is interpreted as something else.
+ */
+ if ( s.rsvd_zero )
+ return -EINVAL;
+
return 0;
}
@@ -1687,6 +1704,7 @@ int vlapic_init(struct vcpu *v)
}
vlapic->pt.source = PTSRC_lapic;
+ vlapic->hw.x2apic_id = 2 * v->vcpu_id;
vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
if ( !vlapic->regs_page )
diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h
index 2c4ff94ae7a8..85c4a236b9f6 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.
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);
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (2 preceding siblings ...)
2024-10-01 12:37 ` [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-09 13:28 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves Alejandro Vallejo
` (6 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné
If toolstack were to upload LAPIC contexts as part of domain creation it
would encounter a problem were the architectural state does not reflect
the APIC ID in the hidden state. This patch ensures updates to the
hidden state trigger an update in the architectural registers so the
APIC ID in both is consistent.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 02570f9dd63a..a8183c3023da 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
s->loaded.hw = 1;
if ( s->loaded.regs )
+ {
+ /*
+ * We already processed architectural regs in lapic_load_regs(), so
+ * this must be a migration. Fix up inconsistencies from any older Xen.
+ */
lapic_load_fixup(s);
+ }
+ else
+ {
+ /*
+ * We haven't seen architectural regs so this could be a migration or a
+ * plain domain create. In the domain create case it's fine to modify
+ * the architectural state to align it to the APIC ID that was just
+ * uploaded and in the migrate case it doesn't matter because the
+ * architectural state will be replaced by the LAPIC_REGS ctx later on.
+ */
+ if ( vlapic_x2apic_mode(s) )
+ set_x2apic_id(s);
+ else
+ vlapic_set_reg(s, APIC_ID, SET_xAPIC_ID(s->hw.x2apic_id));
+ }
hvm_update_vlapic_mode(v);
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (3 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-09 14:03 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer Alejandro Vallejo
` (5 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD
Make it so the APs expose their own APIC IDs in a LUT. We can use that
LUT to populate the MADT, decoupling the algorithm that relates CPU IDs
and APIC IDs from hvmloader.
While at this also remove ap_callin, as writing the APIC ID may serve
the same purpose.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/firmware/hvmloader/config.h | 5 ++-
tools/firmware/hvmloader/hvmloader.c | 6 +--
tools/firmware/hvmloader/mp_tables.c | 4 +-
tools/firmware/hvmloader/smp.c | 54 ++++++++++++++++++++-----
tools/firmware/hvmloader/util.c | 2 +-
tools/include/xen-tools/common-macros.h | 5 +++
6 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index cd716bf39245..17666a1fdb01 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -4,6 +4,8 @@
#include <stdint.h>
#include <stdbool.h>
+#include <xen/hvm/hvm_info_table.h>
+
enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
extern enum virtual_vga virtual_vga;
@@ -48,8 +50,9 @@ extern uint8_t ioapic_version;
#define IOAPIC_ID 0x01
+extern uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
+
#define LAPIC_BASE_ADDRESS 0xfee00000
-#define LAPIC_ID(vcpu_id) ((vcpu_id) * 2)
#define PCI_ISA_DEVFN 0x08 /* dev 1, fn 0 */
#define PCI_ISA_IRQ_MASK 0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index f8af88fabf24..0ff190ff4ec0 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -224,7 +224,7 @@ static void apic_setup(void)
/* 8259A ExtInts are delivered through IOAPIC pin 0 (Virtual Wire Mode). */
ioapic_write(0x10, APIC_DM_EXTINT);
- ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
+ ioapic_write(0x11, SET_APIC_ID(CPU_TO_X2APICID[0]));
}
struct bios_info {
@@ -341,11 +341,11 @@ int main(void)
printf("CPU speed is %u MHz\n", get_cpu_mhz());
+ smp_initialise();
+
apic_setup();
pci_setup();
- smp_initialise();
-
perform_tests();
if ( bios->bios_info_setup )
diff --git a/tools/firmware/hvmloader/mp_tables.c b/tools/firmware/hvmloader/mp_tables.c
index 77d3010406d0..494f5bb3d813 100644
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -198,8 +198,10 @@ static void fill_mp_config_table(struct mp_config_table *mpct, int length)
/* fills in an MP processor entry for VCPU 'vcpu_id' */
static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
{
+ ASSERT(CPU_TO_X2APICID[vcpu_id] < 0xFF );
+
mppe->type = ENTRY_TYPE_PROCESSOR;
- mppe->lapic_id = LAPIC_ID(vcpu_id);
+ mppe->lapic_id = CPU_TO_X2APICID[vcpu_id];
mppe->lapic_version = 0x11;
mppe->cpu_flags = CPU_FLAG_ENABLED;
if ( vcpu_id == 0 )
diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
index 1b940cefd071..b0d4da111904 100644
--- a/tools/firmware/hvmloader/smp.c
+++ b/tools/firmware/hvmloader/smp.c
@@ -29,7 +29,34 @@
#include <xen/vcpu.h>
-static int ap_callin;
+/**
+ * Lookup table of x2APIC IDs.
+ *
+ * Each entry is populated its respective CPU as they come online. This is required
+ * for generating the MADT with minimal assumptions about ID relationships.
+ */
+uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
+
+/** Tristate about x2apic being supported. -1=unknown */
+static int has_x2apic = -1;
+
+static uint32_t read_apic_id(void)
+{
+ uint32_t apic_id;
+
+ if ( has_x2apic )
+ cpuid(0xb, NULL, NULL, NULL, &apic_id);
+ else
+ {
+ cpuid(1, NULL, &apic_id, NULL, NULL);
+ apic_id >>= 24;
+ }
+
+ /* Never called by cpu0, so should never return 0 */
+ ASSERT(apic_id);
+
+ return apic_id;
+}
static void cpu_setup(unsigned int cpu)
{
@@ -37,13 +64,17 @@ static void cpu_setup(unsigned int cpu)
cacheattr_init();
printf("done.\n");
- if ( !cpu ) /* Used on the BSP too */
+ /* The BSP exits early because its APIC ID is known to be zero */
+ if ( !cpu )
return;
wmb();
- ap_callin = 1;
+ ACCESS_ONCE(CPU_TO_X2APICID[cpu]) = read_apic_id();
- /* After this point, the BSP will shut us down. */
+ /*
+ * After this point the BSP will shut us down. A write to
+ * CPU_TO_X2APICID[cpu] signals the BSP to bring down `cpu`.
+ */
for ( ;; )
asm volatile ( "hlt" );
@@ -54,10 +85,6 @@ static void boot_cpu(unsigned int cpu)
static uint8_t ap_stack[PAGE_SIZE] __attribute__ ((aligned (16)));
static struct vcpu_hvm_context ap;
- /* Initialise shared variables. */
- ap_callin = 0;
- wmb();
-
/* Wake up the secondary processor */
ap = (struct vcpu_hvm_context) {
.mode = VCPU_HVM_MODE_32B,
@@ -90,10 +117,11 @@ static void boot_cpu(unsigned int cpu)
BUG();
/*
- * Wait for the secondary processor to complete initialisation.
+ * Wait for the secondary processor to complete initialisation,
+ * which is signaled by its x2APIC ID being written to the LUT.
* Do not touch shared resources meanwhile.
*/
- while ( !ap_callin )
+ while ( !ACCESS_ONCE(CPU_TO_X2APICID[cpu]) )
cpu_relax();
/* Take the secondary processor offline. */
@@ -104,6 +132,12 @@ static void boot_cpu(unsigned int cpu)
void smp_initialise(void)
{
unsigned int i, nr_cpus = hvm_info->nr_vcpus;
+ uint32_t ecx;
+
+ cpuid(1, NULL, NULL, &ecx, NULL);
+ has_x2apic = (ecx >> 21) & 1;
+ if ( has_x2apic )
+ printf("x2APIC supported\n");
printf("Multiprocessor initialisation:\n");
cpu_setup(0);
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index d3b3f9038e64..7e1e105d79dd 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -827,7 +827,7 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
static uint32_t acpi_lapic_id(unsigned cpu)
{
- return LAPIC_ID(cpu);
+ return CPU_TO_X2APIC_ID[cpu];
}
void hvmloader_acpi_build_tables(struct acpi_config *config,
diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 60912225cb7a..336c6309d96e 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -108,4 +108,9 @@
#define get_unaligned(ptr) get_unaligned_t(typeof(*(ptr)), ptr)
#define put_unaligned(val, ptr) put_unaligned_t(typeof(*(ptr)), val, ptr)
+#define __ACCESS_ONCE(x) ({ \
+ (void)(typeof(x))0; /* Scalar typecheck. */ \
+ (volatile typeof(x) *)&(x); })
+#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
+
#endif /* __XEN_TOOLS_COMMON_MACROS__ */
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (4 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-09 14:25 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 07/11] tools/libguest: Always set vCPU context in vcpu_hvm() Alejandro Vallejo
` (4 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Juergen Gross
Refactors libacpi so that a single LUT is the authoritative source of
truth for the CPU to APIC ID mappings. This has a know-on effect in
reducing complexity on future patches, as the same LUT can be used for
configuring the APICs and configuring the ACPI tables for PVH.
Not functional change intended, because the same mappings are preserved.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/firmware/hvmloader/util.c | 7 +------
tools/include/xenguest.h | 5 +++++
tools/libacpi/build.c | 6 +++---
tools/libacpi/libacpi.h | 2 +-
tools/libs/light/libxl_dom.c | 5 +++++
tools/libs/light/libxl_x86_acpi.c | 7 +------
6 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 7e1e105d79dd..4a6303bbae8c 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -825,11 +825,6 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
/* ACPI builder currently doesn't free memory so this is just a stub */
}
-static uint32_t acpi_lapic_id(unsigned cpu)
-{
- return CPU_TO_X2APIC_ID[cpu];
-}
-
void hvmloader_acpi_build_tables(struct acpi_config *config,
unsigned int physical)
{
@@ -859,7 +854,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
}
config->lapic_base_address = LAPIC_BASE_ADDRESS;
- config->lapic_id = acpi_lapic_id;
+ config->cpu_to_apicid = CPU_TO_X2APICID;
config->ioapic_base_address = IOAPIC_BASE_ADDRESS;
config->ioapic_id = IOAPIC_ID;
config->pci_isa_irq_mask = PCI_ISA_IRQ_MASK;
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b772a..aa50b78dfb89 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -22,6 +22,8 @@
#ifndef XENGUEST_H
#define XENGUEST_H
+#include "xen/hvm/hvm_info_table.h"
+
#define XC_NUMA_NO_NODE (~0U)
#define XCFLAGS_LIVE (1 << 0)
@@ -236,6 +238,9 @@ struct xc_dom_image {
#if defined(__i386__) || defined(__x86_64__)
struct e820entry *e820;
unsigned int e820_entries;
+
+ /* LUT mapping cpu id to (x2)APIC ID */
+ uint32_t cpu_to_apicid[HVM_MAX_VCPUS];
#endif
xen_pfn_t vuart_gfn;
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 2f29863db154..2ad1d461a2ec 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -74,7 +74,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
const struct hvm_info_table *hvminfo = config->hvminfo;
int i, sz;
- if ( config->lapic_id == NULL )
+ if ( config->cpu_to_apicid == NULL )
return NULL;
sz = sizeof(struct acpi_20_madt);
@@ -148,7 +148,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
lapic->length = sizeof(*lapic);
/* Processor ID must match processor-object IDs in the DSDT. */
lapic->acpi_processor_id = i;
- lapic->apic_id = config->lapic_id(i);
+ lapic->apic_id = config->cpu_to_apicid[i];
lapic->flags = (test_bit(i, hvminfo->vcpu_online)
? ACPI_LOCAL_APIC_ENABLED : 0);
lapic++;
@@ -236,7 +236,7 @@ static struct acpi_20_srat *construct_srat(struct acpi_ctxt *ctxt,
processor->type = ACPI_PROCESSOR_AFFINITY;
processor->length = sizeof(*processor);
processor->domain = config->numa.vcpu_to_vnode[i];
- processor->apic_id = config->lapic_id(i);
+ processor->apic_id = config->cpu_to_apicid[i];
processor->flags = ACPI_LOCAL_APIC_AFFIN_ENABLED;
processor++;
}
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index deda39e5dbc4..8b010212448c 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -84,7 +84,7 @@ struct acpi_config {
unsigned long rsdp;
/* x86-specific parameters */
- uint32_t (*lapic_id)(unsigned cpu);
+ uint32_t *cpu_to_apicid; /* LUT mapping cpu id to (x2)APIC ID */
uint32_t lapic_base_address;
uint32_t ioapic_base_address;
uint16_t pci_isa_irq_mask;
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 94fef374014e..7f9c6eaa8b24 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -1082,6 +1082,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
dom->container_type = XC_DOM_HVM_CONTAINER;
+#if defined(__i386__) || defined(__x86_64__)
+ for ( uint32_t i = 0; i < info->max_vcpus; i++ )
+ dom->cpu_to_apicid[i] = 2 * i; /* TODO: Replace by topo calculation */
+#endif
+
/* The params from the configuration file are in Mb, which are then
* multiplied by 1 Kb. This was then divided off when calling
* the old xc_hvm_build_target_mem() which then turned them to bytes.
diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 5cf261bd6794..585d4c8755cb 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -75,11 +75,6 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
{
}
-static uint32_t acpi_lapic_id(unsigned cpu)
-{
- return cpu * 2;
-}
-
static int init_acpi_config(libxl__gc *gc,
struct xc_dom_image *dom,
const libxl_domain_build_info *b_info,
@@ -144,7 +139,7 @@ static int init_acpi_config(libxl__gc *gc,
config->hvminfo = hvminfo;
config->lapic_base_address = LAPIC_BASE_ADDRESS;
- config->lapic_id = acpi_lapic_id;
+ config->cpu_to_apicid = dom->cpu_to_apicid;
config->acpi_revision = 5;
rc = 0;
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 07/11] tools/libguest: Always set vCPU context in vcpu_hvm()
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (5 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 08/11] xen/lib: Add topology generator for x86 Alejandro Vallejo
` (3 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel; +Cc: Alejandro Vallejo, Anthony PERARD, Juergen Gross
Currently used by PVH to set MTRR, will be used by a later patch to set
APIC state. Unconditionally send the hypercall, and gate overriding the
MTRR so it remains functionally equivalent.
While at it, add a missing "goto out" to what was the error condition
in the loop.
In principle this patch shouldn't affect functionality. An extra record
(the MTRR) is sent to the hypervisor per vCPU on HVM, but these records
are identical to those retrieved in the first place so there's no
expected functional change.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/libs/guest/xg_dom_x86.c | 84 ++++++++++++++++++-----------------
1 file changed, 44 insertions(+), 40 deletions(-)
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index cba01384ae75..c98229317db7 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -989,6 +989,7 @@ const static void *hvm_get_save_record(const void *ctx, unsigned int type,
static int vcpu_hvm(struct xc_dom_image *dom)
{
+ /* Initialises the BSP */
struct {
struct hvm_save_descriptor header_d;
HVM_SAVE_TYPE(HEADER) header;
@@ -997,6 +998,18 @@ static int vcpu_hvm(struct xc_dom_image *dom)
struct hvm_save_descriptor end_d;
HVM_SAVE_TYPE(END) end;
} bsp_ctx;
+ /* Initialises APICs and MTRRs of every vCPU */
+ struct {
+ struct hvm_save_descriptor header_d;
+ HVM_SAVE_TYPE(HEADER) header;
+ struct hvm_save_descriptor mtrr_d;
+ HVM_SAVE_TYPE(MTRR) mtrr;
+ struct hvm_save_descriptor end_d;
+ HVM_SAVE_TYPE(END) end;
+ } vcpu_ctx;
+ /* Context from full_ctx */
+ const HVM_SAVE_TYPE(MTRR) *mtrr_record;
+ /* Raw context as taken from Xen */
uint8_t *full_ctx = NULL;
int rc;
@@ -1083,51 +1096,42 @@ static int vcpu_hvm(struct xc_dom_image *dom)
bsp_ctx.end_d.instance = 0;
bsp_ctx.end_d.length = HVM_SAVE_LENGTH(END);
- /* TODO: maybe this should be a firmware option instead? */
- if ( !dom->device_model )
+ /* TODO: maybe setting MTRRs should be a firmware option instead? */
+ mtrr_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
+
+ if ( !mtrr_record)
{
- struct {
- struct hvm_save_descriptor header_d;
- HVM_SAVE_TYPE(HEADER) header;
- struct hvm_save_descriptor mtrr_d;
- HVM_SAVE_TYPE(MTRR) mtrr;
- struct hvm_save_descriptor end_d;
- HVM_SAVE_TYPE(END) end;
- } mtrr = {
- .header_d = bsp_ctx.header_d,
- .header = bsp_ctx.header,
- .mtrr_d.typecode = HVM_SAVE_CODE(MTRR),
- .mtrr_d.length = HVM_SAVE_LENGTH(MTRR),
- .end_d = bsp_ctx.end_d,
- .end = bsp_ctx.end,
- };
- const HVM_SAVE_TYPE(MTRR) *mtrr_record =
- hvm_get_save_record(full_ctx, HVM_SAVE_CODE(MTRR), 0);
- unsigned int i;
-
- if ( !mtrr_record )
- {
- xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
- "%s: unable to get MTRR save record", __func__);
- goto out;
- }
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: unable to get MTRR save record", __func__);
+ goto out;
+ }
- memcpy(&mtrr.mtrr, mtrr_record, sizeof(mtrr.mtrr));
+ vcpu_ctx.header_d = bsp_ctx.header_d;
+ vcpu_ctx.header = bsp_ctx.header;
+ vcpu_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
+ vcpu_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
+ vcpu_ctx.mtrr = *mtrr_record;
+ vcpu_ctx.end_d = bsp_ctx.end_d;
+ vcpu_ctx.end = bsp_ctx.end;
- /*
- * Enable MTRR, set default type to WB.
- * TODO: add MMIO areas as UC when passthrough is supported.
- */
- mtrr.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK | MTRR_DEF_TYPE_ENABLE;
+ /*
+ * Enable MTRR, set default type to WB.
+ * TODO: add MMIO areas as UC when passthrough is supported in PVH
+ */
+ if ( !dom->device_model )
+ vcpu_ctx.mtrr.msr_mtrr_def_type = MTRR_TYPE_WRBACK | MTRR_DEF_TYPE_ENABLE;
+
+ for ( unsigned int i = 0; i < dom->max_vcpus; i++ )
+ {
+ vcpu_ctx.mtrr_d.instance = i;
- for ( i = 0; i < dom->max_vcpus; i++ )
+ rc = xc_domain_hvm_setcontext(dom->xch, dom->guest_domid,
+ (uint8_t *)&vcpu_ctx, sizeof(vcpu_ctx));
+ if ( rc != 0 )
{
- mtrr.mtrr_d.instance = i;
- rc = xc_domain_hvm_setcontext(dom->xch, dom->guest_domid,
- (uint8_t *)&mtrr, sizeof(mtrr));
- if ( rc != 0 )
- xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
- "%s: SETHVMCONTEXT failed (rc=%d)", __func__, rc);
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: SETHVMCONTEXT failed (rc=%d)", __func__, rc);
+ goto out;
}
}
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 08/11] xen/lib: Add topology generator for x86
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (6 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 07/11] tools/libguest: Always set vCPU context in vcpu_hvm() Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-09 14:45 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy Alejandro Vallejo
` (2 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD
Add a helper to populate topology leaves in the cpu policy from
threads/core and cores/package counts. It's unit-tested in
test-cpu-policy.c,
but it's not connected to the rest of the code yet.
Adds the ASSERT() macro to xen/lib/x86/private.h, as it was missing.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/tests/cpu-policy/test-cpu-policy.c | 133 +++++++++++++++++++++++
xen/include/xen/lib/x86/cpu-policy.h | 16 +++
xen/lib/x86/policy.c | 88 +++++++++++++++
xen/lib/x86/private.h | 4 +
4 files changed, 241 insertions(+)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 9216010b1c5d..870f7ecee0e5 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -654,6 +654,137 @@ static void test_is_compatible_failure(void)
}
}
+static void test_topo_from_parts(void)
+{
+ static const struct test {
+ unsigned int threads_per_core;
+ unsigned int cores_per_pkg;
+ struct cpu_policy policy;
+ } tests[] = {
+ {
+ .threads_per_core = 3, .cores_per_pkg = 1,
+ .policy = {
+ .x86_vendor = X86_VENDOR_AMD,
+ .topo.subleaf = {
+ { .nr_logical = 3, .level = 0, .type = 1, .id_shift = 2, },
+ { .nr_logical = 1, .level = 1, .type = 2, .id_shift = 2, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 1, .cores_per_pkg = 3,
+ .policy = {
+ .x86_vendor = X86_VENDOR_AMD,
+ .topo.subleaf = {
+ { .nr_logical = 1, .level = 0, .type = 1, .id_shift = 0, },
+ { .nr_logical = 3, .level = 1, .type = 2, .id_shift = 2, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 7, .cores_per_pkg = 5,
+ .policy = {
+ .x86_vendor = X86_VENDOR_AMD,
+ .topo.subleaf = {
+ { .nr_logical = 7, .level = 0, .type = 1, .id_shift = 3, },
+ { .nr_logical = 5, .level = 1, .type = 2, .id_shift = 6, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 2, .cores_per_pkg = 128,
+ .policy = {
+ .x86_vendor = X86_VENDOR_AMD,
+ .topo.subleaf = {
+ { .nr_logical = 2, .level = 0, .type = 1, .id_shift = 1, },
+ { .nr_logical = 128, .level = 1, .type = 2,
+ .id_shift = 8, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 3, .cores_per_pkg = 1,
+ .policy = {
+ .x86_vendor = X86_VENDOR_INTEL,
+ .topo.subleaf = {
+ { .nr_logical = 3, .level = 0, .type = 1, .id_shift = 2, },
+ { .nr_logical = 3, .level = 1, .type = 2, .id_shift = 2, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 1, .cores_per_pkg = 3,
+ .policy = {
+ .x86_vendor = X86_VENDOR_INTEL,
+ .topo.subleaf = {
+ { .nr_logical = 1, .level = 0, .type = 1, .id_shift = 0, },
+ { .nr_logical = 3, .level = 1, .type = 2, .id_shift = 2, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 7, .cores_per_pkg = 5,
+ .policy = {
+ .x86_vendor = X86_VENDOR_INTEL,
+ .topo.subleaf = {
+ { .nr_logical = 7, .level = 0, .type = 1, .id_shift = 3, },
+ { .nr_logical = 35, .level = 1, .type = 2, .id_shift = 6, },
+ },
+ },
+ },
+ {
+ .threads_per_core = 2, .cores_per_pkg = 128,
+ .policy = {
+ .x86_vendor = X86_VENDOR_INTEL,
+ .topo.subleaf = {
+ { .nr_logical = 2, .level = 0, .type = 1, .id_shift = 1, },
+ { .nr_logical = 256, .level = 1, .type = 2,
+ .id_shift = 8, },
+ },
+ },
+ },
+ };
+
+ printf("Testing topology synthesis from parts:\n");
+
+ for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+ {
+ const struct test *t = &tests[i];
+ struct cpu_policy actual = { .x86_vendor = t->policy.x86_vendor };
+ int rc = x86_topo_from_parts(&actual, t->threads_per_core,
+ t->cores_per_pkg);
+
+ if ( rc || memcmp(&actual.topo, &t->policy.topo, sizeof(actual.topo)) )
+ {
+#define TOPO(n, f) t->policy.topo.subleaf[(n)].f, actual.topo.subleaf[(n)].f
+ fail("FAIL[%d] - '%s %u t/c, %u c/p'\n",
+ rc,
+ x86_cpuid_vendor_to_str(t->policy.x86_vendor),
+ t->threads_per_core, t->cores_per_pkg);
+ printf(" subleaf=%u expected_n=%u actual_n=%u\n"
+ " expected_lvl=%u actual_lvl=%u\n"
+ " expected_type=%u actual_type=%u\n"
+ " expected_shift=%u actual_shift=%u\n",
+ 0,
+ TOPO(0, nr_logical),
+ TOPO(0, level),
+ TOPO(0, type),
+ TOPO(0, id_shift));
+
+ printf(" subleaf=%u expected_n=%u actual_n=%u\n"
+ " expected_lvl=%u actual_lvl=%u\n"
+ " expected_type=%u actual_type=%u\n"
+ " expected_shift=%u actual_shift=%u\n",
+ 1,
+ TOPO(1, nr_logical),
+ TOPO(1, level),
+ TOPO(1, type),
+ TOPO(1, id_shift));
+#undef TOPO
+ }
+ }
+}
+
int main(int argc, char **argv)
{
printf("CPU Policy unit tests\n");
@@ -671,6 +802,8 @@ int main(int argc, char **argv)
test_is_compatible_success();
test_is_compatible_failure();
+ test_topo_from_parts();
+
if ( nr_failures )
printf("Done: %u failures\n", nr_failures);
else
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index f43e1a3b21e9..116b305a1d7f 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -542,6 +542,22 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
const struct cpu_policy *guest,
struct cpu_policy_errors *err);
+/**
+ * Synthesise topology information in `p` given high-level constraints
+ *
+ * Topology is given in various fields accross several leaves, some of
+ * which are vendor-specific. This function uses the policy itself to
+ * derive such leaves from threads/core and cores/package.
+ *
+ * @param p CPU policy of the domain.
+ * @param threads_per_core threads/core. Doesn't need to be a power of 2.
+ * @param cores_per_package cores/package. Doesn't need to be a power of 2.
+ * @return 0 on success; -errno on failure
+ */
+int x86_topo_from_parts(struct cpu_policy *p,
+ unsigned int threads_per_core,
+ unsigned int cores_per_pkg);
+
#endif /* !XEN_LIB_X86_POLICIES_H */
/*
diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
index 63bc96451d2c..16b09a427841 100644
--- a/xen/lib/x86/policy.c
+++ b/xen/lib/x86/policy.c
@@ -2,6 +2,94 @@
#include <xen/lib/x86/cpu-policy.h>
+static unsigned int order(unsigned int n)
+{
+ ASSERT(n); /* clz(0) is UB */
+
+ return 8 * sizeof(n) - __builtin_clz(n);
+}
+
+int x86_topo_from_parts(struct cpu_policy *p,
+ unsigned int threads_per_core,
+ unsigned int cores_per_pkg)
+{
+ unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
+ unsigned int apic_id_size;
+
+ if ( !p || !threads_per_core || !cores_per_pkg )
+ return -EINVAL;
+
+ p->basic.max_leaf = MAX(0xb, p->basic.max_leaf);
+
+ memset(p->topo.raw, 0, sizeof(p->topo.raw));
+
+ /* thread level */
+ p->topo.subleaf[0].nr_logical = threads_per_core;
+ p->topo.subleaf[0].id_shift = 0;
+ p->topo.subleaf[0].level = 0;
+ p->topo.subleaf[0].type = 1;
+ if ( threads_per_core > 1 )
+ p->topo.subleaf[0].id_shift = order(threads_per_core - 1);
+
+ /* core level */
+ p->topo.subleaf[1].nr_logical = cores_per_pkg;
+ if ( p->x86_vendor == X86_VENDOR_INTEL )
+ p->topo.subleaf[1].nr_logical = threads_per_pkg;
+ p->topo.subleaf[1].id_shift = p->topo.subleaf[0].id_shift;
+ p->topo.subleaf[1].level = 1;
+ p->topo.subleaf[1].type = 2;
+ if ( cores_per_pkg > 1 )
+ p->topo.subleaf[1].id_shift += order(cores_per_pkg - 1);
+
+ apic_id_size = p->topo.subleaf[1].id_shift;
+
+ /*
+ * Contrary to what the name might seem to imply. HTT is an enabler for
+ * SMP and there's no harm in setting it even with a single vCPU.
+ */
+ p->basic.htt = true;
+ p->basic.lppp = MIN(0xff, threads_per_pkg);
+
+ switch ( p->x86_vendor )
+ {
+ case X86_VENDOR_INTEL: {
+ struct cpuid_cache_leaf *sl = p->cache.subleaf;
+
+ for ( size_t i = 0; sl->type &&
+ i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
+ {
+ sl->cores_per_package = cores_per_pkg - 1;
+ sl->threads_per_cache = threads_per_core - 1;
+ if ( sl->type == 3 /* unified cache */ )
+ sl->threads_per_cache = threads_per_pkg - 1;
+ }
+ break;
+ }
+
+ case X86_VENDOR_AMD:
+ case X86_VENDOR_HYGON:
+ /* Expose p->basic.lppp */
+ p->extd.cmp_legacy = true;
+
+ /* Clip NC to the maximum value it can hold */
+ p->extd.nc = MIN(0xff, threads_per_pkg - 1);
+
+ /* TODO: Expose leaf e1E */
+ p->extd.topoext = false;
+
+ /*
+ * Clip APIC ID to 8 bits, as that's what high core-count machines do.
+ *
+ * That's what AMD EPYC 9654 does with >256 CPUs.
+ */
+ p->extd.apic_id_size = MIN(8, apic_id_size);
+
+ break;
+ }
+
+ return 0;
+}
+
int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
const struct cpu_policy *guest,
struct cpu_policy_errors *err)
diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
index 60bb82a400b7..2ec9dbee33c2 100644
--- a/xen/lib/x86/private.h
+++ b/xen/lib/x86/private.h
@@ -4,6 +4,7 @@
#ifdef __XEN__
#include <xen/bitops.h>
+#include <xen/bug.h>
#include <xen/guest_access.h>
#include <xen/kernel.h>
#include <xen/lib.h>
@@ -17,6 +18,7 @@
#else
+#include <assert.h>
#include <errno.h>
#include <inttypes.h>
#include <stdbool.h>
@@ -28,6 +30,8 @@
#include <xen-tools/common-macros.h>
+#define ASSERT(x) assert(x)
+
static inline bool test_bit(unsigned int bit, const void *vaddr)
{
const char *addr = vaddr;
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (7 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 08/11] xen/lib: Add topology generator for x86 Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-09 14:53 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 10/11] tools/libguest: Set distinct x2APIC IDs for each vCPU Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 11/11] tools/x86: Synthesise domain topologies Alejandro Vallejo
10 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD
Implements the helper for mapping vcpu_id to x2apic_id given a valid
topology in a policy. The algo is written with the intention of
extending it to leaves 0x1f and extended 0x26 in the future.
Toolstack doesn't set leaf 0xb and the HVM default policy has it
cleared, so the leaf is not implemented. In that case, the new helper
just returns the legacy mapping.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
tools/tests/cpu-policy/test-cpu-policy.c | 68 +++++++++++++++++++++
xen/include/xen/lib/x86/cpu-policy.h | 11 ++++
xen/lib/x86/policy.c | 76 ++++++++++++++++++++++++
3 files changed, 155 insertions(+)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index 870f7ecee0e5..ae7fc46a47d2 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -785,6 +785,73 @@ static void test_topo_from_parts(void)
}
}
+static void test_x2apic_id_from_vcpu_id_success(void)
+{
+ static const struct test {
+ unsigned int vcpu_id;
+ unsigned int threads_per_core;
+ unsigned int cores_per_pkg;
+ uint32_t x2apic_id;
+ uint8_t x86_vendor;
+ } tests[] = {
+ {
+ .vcpu_id = 3, .threads_per_core = 3, .cores_per_pkg = 8,
+ .x2apic_id = 1 << 2,
+ },
+ {
+ .vcpu_id = 6, .threads_per_core = 3, .cores_per_pkg = 8,
+ .x2apic_id = 2 << 2,
+ },
+ {
+ .vcpu_id = 24, .threads_per_core = 3, .cores_per_pkg = 8,
+ .x2apic_id = 1 << 5,
+ },
+ {
+ .vcpu_id = 35, .threads_per_core = 3, .cores_per_pkg = 8,
+ .x2apic_id = (35 % 3) | (((35 / 3) % 8) << 2) | ((35 / 24) << 5),
+ },
+ {
+ .vcpu_id = 96, .threads_per_core = 7, .cores_per_pkg = 3,
+ .x2apic_id = (96 % 7) | (((96 / 7) % 3) << 3) | ((96 / 21) << 5),
+ },
+ };
+
+ const uint8_t vendors[] = {
+ X86_VENDOR_INTEL,
+ X86_VENDOR_AMD,
+ X86_VENDOR_CENTAUR,
+ X86_VENDOR_SHANGHAI,
+ X86_VENDOR_HYGON,
+ };
+
+ printf("Testing x2apic id from vcpu id success:\n");
+
+ /* Perform the test run on every vendor we know about */
+ for ( size_t i = 0; i < ARRAY_SIZE(vendors); ++i )
+ {
+ for ( size_t j = 0; j < ARRAY_SIZE(tests); ++j )
+ {
+ struct cpu_policy policy = { .x86_vendor = vendors[i] };
+ const struct test *t = &tests[j];
+ uint32_t x2apic_id;
+ int rc = x86_topo_from_parts(&policy, t->threads_per_core,
+ t->cores_per_pkg);
+
+ if ( rc ) {
+ fail("FAIL[%d] - 'x86_topo_from_parts() failed", rc);
+ continue;
+ }
+
+ x2apic_id = x86_x2apic_id_from_vcpu_id(&policy, t->vcpu_id);
+ if ( x2apic_id != t->x2apic_id )
+ fail("FAIL - '%s cpu%u %u t/c %u c/p'. bad x2apic_id: expected=%u actual=%u\n",
+ x86_cpuid_vendor_to_str(policy.x86_vendor),
+ t->vcpu_id, t->threads_per_core, t->cores_per_pkg,
+ t->x2apic_id, x2apic_id);
+ }
+ }
+}
+
int main(int argc, char **argv)
{
printf("CPU Policy unit tests\n");
@@ -803,6 +870,7 @@ int main(int argc, char **argv)
test_is_compatible_failure();
test_topo_from_parts();
+ test_x2apic_id_from_vcpu_id_success();
if ( nr_failures )
printf("Done: %u failures\n", nr_failures);
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index 116b305a1d7f..6fe19490d290 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -542,6 +542,17 @@ 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
+ *
+ * If the policy lacks leaf 0xb falls back to legacy mapping of apic_id=cpu*2
+ *
+ * @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);
+
/**
* Synthesise topology information in `p` given high-level constraints
*
diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
index 16b09a427841..6dd9a2900ad7 100644
--- a/xen/lib/x86/policy.c
+++ b/xen/lib/x86/policy.c
@@ -2,6 +2,82 @@
#include <xen/lib/x86/cpu-policy.h>
+static uint32_t parts_per_higher_scoped_level(const struct cpu_policy *p,
+ size_t lvl)
+{
+ /*
+ * `nr_logical` reported by Intel is the number of THREADS contained in
+ * the next topological scope. For example, assuming a system with 2
+ * threads/core and 3 cores/module in a fully symmetric topology,
+ * `nr_logical` at the core level will report 6. Because it's reporting
+ * the number of threads in a module.
+ *
+ * On AMD/Hygon, nr_logical is already normalized by the higher scoped
+ * level (cores/complex, etc) so we can return it as-is.
+ */
+ if ( p->x86_vendor != X86_VENDOR_INTEL || !lvl )
+ return p->topo.subleaf[lvl].nr_logical;
+
+ return p->topo.subleaf[lvl].nr_logical /
+ p->topo.subleaf[lvl - 1].nr_logical;
+}
+
+uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id)
+{
+ uint32_t shift = 0, x2apic_id = 0;
+
+ /* In the absence of topology leaves, fallback to traditional mapping */
+ if ( !p->topo.subleaf[0].type )
+ return id * 2;
+
+ /*
+ * `id` means different things at different points of the algo
+ *
+ * At lvl=0: global thread_id (same as vcpu_id)
+ * At lvl=1: global core_id
+ * At lvl=2: global socket_id (actually complex_id in AMD, module_id
+ * in Intel, but the name is inconsequential)
+ *
+ * +--+
+ * ____ |#0| ______ <= 1 socket
+ * / +--+ \+--+
+ * __#0__ __|#1|__ <= 2 cores/socket
+ * / | \ +--+/ +-|+ \
+ * #0 #1 #2 |#3| #4 #5 <= 3 threads/core
+ * +--+
+ *
+ * ... and so on. Global in this context means that it's a unique
+ * identifier for the whole topology, and not relative to the level
+ * it's in. For example, in the diagram shown above, we're looking at
+ * thread #3 in the global sense, though it's #0 within its core.
+ *
+ * Note that dividing a global thread_id by the number of threads per
+ * core returns the global core id that contains it. e.g: 0, 1 or 2
+ * divided by 3 returns core_id=0. 3, 4 or 5 divided by 3 returns core
+ * 1, and so on. An analogous argument holds for higher levels. This is
+ * the property we exploit to derive x2apic_id from vcpu_id.
+ *
+ * NOTE: `topo` is currently derived from leaf 0xb, which is bound to two
+ * levels, but once we track leaves 0x1f (or extended 0x26) there will be a
+ * few more. The algorithm is written to cope with that case.
+ */
+ for ( uint32_t i = 0; i < ARRAY_SIZE(p->topo.raw); i++ )
+ {
+ uint32_t nr_parts;
+
+ if ( !p->topo.subleaf[i].type )
+ /* sentinel subleaf */
+ break;
+
+ nr_parts = parts_per_higher_scoped_level(p, i);
+ x2apic_id |= (id % nr_parts) << shift;
+ id /= nr_parts;
+ shift = p->topo.subleaf[i].id_shift;
+ }
+
+ return (id << shift) | x2apic_id;
+}
+
static unsigned int order(unsigned int n)
{
ASSERT(n); /* clz(0) is UB */
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 10/11] tools/libguest: Set distinct x2APIC IDs for each vCPU
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (8 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 11/11] tools/x86: Synthesise domain topologies Alejandro Vallejo
10 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel; +Cc: Alejandro Vallejo, Anthony PERARD, Juergen Gross
Have toolstack populate the new x2APIC ID in the LAPIC save record with
the proper IDs intended for each vCPU.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v6:
* Rely on the new LUT in xc_dom_image rather than recalculating APIC IDs.
---
tools/libs/guest/xg_dom_x86.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index c98229317db7..38486140ed15 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -1004,11 +1004,14 @@ static int vcpu_hvm(struct xc_dom_image *dom)
HVM_SAVE_TYPE(HEADER) header;
struct hvm_save_descriptor mtrr_d;
HVM_SAVE_TYPE(MTRR) mtrr;
+ struct hvm_save_descriptor lapic_d;
+ HVM_SAVE_TYPE(LAPIC) lapic;
struct hvm_save_descriptor end_d;
HVM_SAVE_TYPE(END) end;
} vcpu_ctx;
- /* Context from full_ctx */
+ /* Contexts from full_ctx */
const HVM_SAVE_TYPE(MTRR) *mtrr_record;
+ const HVM_SAVE_TYPE(LAPIC) *lapic_record;
/* Raw context as taken from Xen */
uint8_t *full_ctx = NULL;
int rc;
@@ -1111,6 +1114,8 @@ static int vcpu_hvm(struct xc_dom_image *dom)
vcpu_ctx.mtrr_d.typecode = HVM_SAVE_CODE(MTRR);
vcpu_ctx.mtrr_d.length = HVM_SAVE_LENGTH(MTRR);
vcpu_ctx.mtrr = *mtrr_record;
+ vcpu_ctx.lapic_d.typecode = HVM_SAVE_CODE(LAPIC);
+ vcpu_ctx.lapic_d.length = HVM_SAVE_LENGTH(LAPIC);
vcpu_ctx.end_d = bsp_ctx.end_d;
vcpu_ctx.end = bsp_ctx.end;
@@ -1125,6 +1130,18 @@ static int vcpu_hvm(struct xc_dom_image *dom)
{
vcpu_ctx.mtrr_d.instance = i;
+ lapic_record = hvm_get_save_record(full_ctx, HVM_SAVE_CODE(LAPIC), i);
+ if ( !lapic_record )
+ {
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: unable to get LAPIC[%d] save record", __func__, i);
+ goto out;
+ }
+
+ vcpu_ctx.lapic = *lapic_record;
+ vcpu_ctx.lapic.x2apic_id = dom->cpu_to_apicid[i];
+ vcpu_ctx.lapic_d.instance = i;
+
rc = xc_domain_hvm_setcontext(dom->xch, dom->guest_domid,
(uint8_t *)&vcpu_ctx, sizeof(vcpu_ctx));
if ( rc != 0 )
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v6 11/11] tools/x86: Synthesise domain topologies
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
` (9 preceding siblings ...)
2024-10-01 12:38 ` [PATCH v6 10/11] tools/libguest: Set distinct x2APIC IDs for each vCPU Alejandro Vallejo
@ 2024-10-01 12:38 ` Alejandro Vallejo
10 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-01 12:38 UTC (permalink / raw)
To: Xen-devel
Cc: Alejandro Vallejo, Anthony PERARD, Juergen Gross, Jan Beulich,
Andrew Cooper, Roger Pau Monné
Expose sensible topologies in leaf 0xb. At the moment it synthesises
non-HT systems, in line with the previous code intent.
Leaf 0xb in the host policy is no longer zapped and the guest {max,def}
policies have their topology leaves zapped instead. The intent is for
toolstack to populate them. There's no current use for the topology
information in the host policy, but it makes no harm.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v6:
* Assign accurate APIC IDs to xc_dom_img->cpu_to_apicid
* New field in v6. Allows ACPI generation to be correct on PVH too.
---
tools/include/xenguest.h | 3 +++
tools/libs/guest/xg_cpuid_x86.c | 29 ++++++++++++++++++++++++++++-
tools/libs/light/libxl_dom.c | 22 +++++++++++++++++++++-
xen/arch/x86/cpu-policy.c | 9 ++++++---
4 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index aa50b78dfb89..dcabf219b9cb 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -831,6 +831,9 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
uint32_t xc_get_cpu_featureset_size(void);
+/* Returns the APIC ID of the `cpu`-th CPU according to `policy` */
+uint32_t xc_cpu_to_apicid(const xc_cpu_policy_t *policy, unsigned int cpu);
+
enum xc_static_cpu_featuremask {
XC_FEATUREMASK_KNOWN,
XC_FEATUREMASK_SPECIAL,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 4453178100ad..c591f8732a1a 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -725,8 +725,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
p->policy.basic.htt = test_bit(X86_FEATURE_HTT, host_featureset);
p->policy.extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
}
- else
+ else if ( restore )
{
+ /*
+ * Reconstruct the topology exposed on Xen <= 4.13. It makes very little
+ * sense, but it's what those guests saw so it's set in stone now.
+ *
+ * Guests from Xen 4.14 onwards carry their own CPUID leaves in the
+ * migration stream so they don't need special treatment.
+ */
+
/*
* Topology for HVM guests is entirely controlled by Xen. For now, we
* hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
@@ -782,6 +790,20 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
break;
}
}
+ else
+ {
+ /* TODO: Expose the ability to choose a custom topology for HVM/PVH */
+ unsigned int threads_per_core = 1;
+ unsigned int cores_per_pkg = di.max_vcpu_id + 1;
+
+ rc = x86_topo_from_parts(&p->policy, threads_per_core, cores_per_pkg);
+ if ( rc )
+ {
+ ERROR("Failed to generate topology: rc=%d t/c=%u c/p=%u",
+ rc, threads_per_core, cores_per_pkg);
+ goto out;
+ }
+ }
nr_leaves = ARRAY_SIZE(p->leaves);
rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
@@ -1028,3 +1050,8 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
return false;
}
+
+uint32_t xc_cpu_to_apicid(const xc_cpu_policy_t *policy, unsigned int cpu)
+{
+ return x86_x2apic_id_from_vcpu_id(&policy->policy, cpu);
+}
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 7f9c6eaa8b24..8dbfc5b7df61 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -1063,6 +1063,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
libxl_domain_build_info *const info = &d_config->b_info;
struct xc_dom_image *dom = NULL;
bool device_model = info->type == LIBXL_DOMAIN_TYPE_HVM ? true : false;
+#if defined(__i386__) || defined(__x86_64__)
+ struct xc_cpu_policy *policy = NULL;
+#endif
xc_dom_loginit(ctx->xch);
@@ -1083,8 +1086,22 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
dom->container_type = XC_DOM_HVM_CONTAINER;
#if defined(__i386__) || defined(__x86_64__)
+ policy = xc_cpu_policy_init();
+ if (!policy) {
+ LOGE(ERROR, "xc_cpu_policy_get_domain failed d%u", domid);
+ rc = ERROR_NOMEM;
+ goto out;
+ }
+
+ rc = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
+ if (rc != 0) {
+ LOGE(ERROR, "xc_cpu_policy_get_domain failed d%u", domid);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
for ( uint32_t i = 0; i < info->max_vcpus; i++ )
- dom->cpu_to_apicid[i] = 2 * i; /* TODO: Replace by topo calculation */
+ dom->cpu_to_apicid[i] = xc_cpu_to_apicid(policy, i);
#endif
/* The params from the configuration file are in Mb, which are then
@@ -1214,6 +1231,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
out:
assert(rc != 0);
if (dom != NULL) xc_dom_release(dom);
+#if defined(__i386__) || defined(__x86_64__)
+ xc_cpu_policy_destroy(policy);
+#endif
return rc;
}
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index b6d9fad56773..39c0aba9ea36 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -266,9 +266,6 @@ static void recalculate_misc(struct cpu_policy *p)
p->basic.raw[0x8] = EMPTY_LEAF;
- /* TODO: Rework topology logic. */
- memset(p->topo.raw, 0, sizeof(p->topo.raw));
-
p->basic.raw[0xc] = EMPTY_LEAF;
p->extd.e1d &= ~CPUID_COMMON_1D_FEATURES;
@@ -616,6 +613,9 @@ static void __init calculate_pv_max_policy(void)
recalculate_xstate(p);
p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
+
+ /* Wipe host topology. Populated by toolstack */
+ memset(p->topo.raw, 0, sizeof(p->topo.raw));
}
static void __init calculate_pv_def_policy(void)
@@ -779,6 +779,9 @@ static void __init calculate_hvm_max_policy(void)
/* It's always possible to emulate CPUID faulting for HVM guests */
p->platform_info.cpuid_faulting = true;
+
+ /* Wipe host topology. Populated by toolstack */
+ memset(p->topo.raw, 0, sizeof(p->topo.raw));
}
static void __init calculate_hvm_def_policy(void)
--
2.46.0
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks
2024-10-01 12:37 ` [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks Alejandro Vallejo
@ 2024-10-08 15:41 ` Jan Beulich
2024-10-09 16:11 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-08 15:41 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel
On 01.10.2024 14:37, Alejandro Vallejo wrote:
> While doing this, factor out checks common to architectural and hidden
> state.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> --
> Last reviewed in the topology series v3. Fell under the cracks.
>
> https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/
It's not the 1st patch in the series, and I can't spot anywhere that it is
made clear that this one can go in ahead of patch 1. I may have overlooked
something in the long-ish cover letter.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
@ 2024-10-09 9:40 ` Jan Beulich
2024-10-09 15:57 ` Alejandro Vallejo
2024-10-09 21:58 ` Andrew Cooper
1 sibling, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 9:40 UTC (permalink / raw)
To: Alejandro Vallejo, Andrew Cooper
Cc: Roger Pau Monné, Anthony PERARD, Xen-devel
On 01.10.2024 14:37, Alejandro Vallejo wrote:
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> #define FAIL_MSR(m) \
> do { e.msr = (m); goto out; } while ( 0 )
>
> - if ( guest->basic.max_leaf > host->basic.max_leaf )
> + /*
> + * Old AMD hardware doesn't expose topology information in leaf 0xb. We
> + * want to emulate that leaf with credible information because it must be
> + * present on systems in which we emulate the x2APIC.
> + *
> + * For that reason, allow the max basic guest leaf to be larger than the
> + * hosts' up until 0xb.
> + */
> + if ( guest->basic.max_leaf > 0xb &&
> + guest->basic.max_leaf > host->basic.max_leaf )
> FAIL_CPUID(0, NA);
>
> if ( guest->feat.max_subleaf > host->feat.max_subleaf )
I'm concerned by this in multiple ways:
1) It's pretty ad hoc, and hence doesn't make clear how to deal with similar
situations in the future.
2) Why would we permit going up to leaf 0xb when x2APIC is off in the respective
leaf?
3) We similarly force a higher extended leaf in order to accommodate the LFENCE-
is-dispatch-serializing bit. Yet there's no similar extra logic there in the
function here.
4) While there the guest vs host check won't matter, the situation with AMX and
AVX10 leaves imo still wants considering here right away. IOW (taken together
with at least 3) above) I think we need to first settle on a model for
collectively all max (sub)leaf handling. That in particular needs to properly
spell out who's responsible for what (tool stack vs Xen).
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
2024-10-01 12:37 ` [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area Alejandro Vallejo
@ 2024-10-09 13:12 ` Jan Beulich
2024-10-09 16:39 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 13:12 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Roger Pau Monné, Xen-devel, Andrew Cooper
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create
2024-10-01 12:38 ` [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create Alejandro Vallejo
@ 2024-10-09 13:28 ` Jan Beulich
2024-10-09 16:44 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 13:28 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel
On 01.10.2024 14:38, Alejandro Vallejo wrote:
> If toolstack were to upload LAPIC contexts as part of domain creation it
If it were to - yes. But it doesn't, an peeking ahead in the series I also
couldn't spot this changing. Hence I don#t think I see why this change
would be needed, and why ...
> would encounter a problem were the architectural state does not reflect
> the APIC ID in the hidden state. This patch ensures updates to the
> hidden state trigger an update in the architectural registers so the
> APIC ID in both is consistent.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 02570f9dd63a..a8183c3023da 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
>
> s->loaded.hw = 1;
> if ( s->loaded.regs )
> + {
> + /*
> + * We already processed architectural regs in lapic_load_regs(), so
> + * this must be a migration. Fix up inconsistencies from any older Xen.
> + */
> lapic_load_fixup(s);
> + }
> + else
> + {
> + /*
> + * We haven't seen architectural regs so this could be a migration or a
> + * plain domain create. In the domain create case it's fine to modify
> + * the architectural state to align it to the APIC ID that was just
> + * uploaded and in the migrate case it doesn't matter because the
> + * architectural state will be replaced by the LAPIC_REGS ctx later on.
> + */
... a comment would need to mention a case that never really happens, thus
only risking to cause confusion.
Jan
> + if ( vlapic_x2apic_mode(s) )
> + set_x2apic_id(s);
> + else
> + vlapic_set_reg(s, APIC_ID, SET_xAPIC_ID(s->hw.x2apic_id));
> + }
>
> hvm_update_vlapic_mode(v);
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
2024-10-01 12:38 ` [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves Alejandro Vallejo
@ 2024-10-09 14:03 ` Jan Beulich
2024-10-09 17:19 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 14:03 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 01.10.2024 14:38, Alejandro Vallejo wrote:
> Make it so the APs expose their own APIC IDs in a LUT. We can use that
> LUT to populate the MADT, decoupling the algorithm that relates CPU IDs
> and APIC IDs from hvmloader.
>
> While at this also remove ap_callin, as writing the APIC ID may serve
> the same purpose.
... on the assumption that no AP will have an APIC ID of zero.
> @@ -341,11 +341,11 @@ int main(void)
>
> printf("CPU speed is %u MHz\n", get_cpu_mhz());
>
> + smp_initialise();
> +
> apic_setup();
> pci_setup();
>
> - smp_initialise();
I can see that you may want cpu_setup(0) to run ahead of apic_setup(). Yet
is it really appropriate to run boot_cpu() ahead of apic_setup() as well?
At the very least it feels logically wrong, even if at the moment there
may not be any direct dependency (which might change, however, down the
road).
> --- a/tools/firmware/hvmloader/mp_tables.c
> +++ b/tools/firmware/hvmloader/mp_tables.c
> @@ -198,8 +198,10 @@ static void fill_mp_config_table(struct mp_config_table *mpct, int length)
> /* fills in an MP processor entry for VCPU 'vcpu_id' */
> static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
> {
> + ASSERT(CPU_TO_X2APICID[vcpu_id] < 0xFF );
Nit: Excess blank before closing paren.
And of course this will need doing differently anyway once we get to
support for more than 128 vCPU-s.
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -29,7 +29,34 @@
>
> #include <xen/vcpu.h>
>
> -static int ap_callin;
> +/**
> + * Lookup table of x2APIC IDs.
> + *
> + * Each entry is populated its respective CPU as they come online. This is required
> + * for generating the MADT with minimal assumptions about ID relationships.
> + */
> +uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
I can kind of accept keeping it simple in the name (albeit - why all caps?),
but at least the comment should mention that it may also be an xAPIC ID
that's stored here.
> @@ -104,6 +132,12 @@ static void boot_cpu(unsigned int cpu)
> void smp_initialise(void)
> {
> unsigned int i, nr_cpus = hvm_info->nr_vcpus;
> + uint32_t ecx;
> +
> + cpuid(1, NULL, NULL, &ecx, NULL);
> + has_x2apic = (ecx >> 21) & 1;
Would be really nice to avoid the literal 21 here by including
xen/arch-x86/cpufeatureset.h. Can this be arranged for?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer
2024-10-01 12:38 ` [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer Alejandro Vallejo
@ 2024-10-09 14:25 ` Jan Beulich
2024-10-09 17:20 ` Alejandro Vallejo
2024-10-11 16:17 ` Alejandro Vallejo
0 siblings, 2 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 14:25 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Juergen Gross, Xen-devel
On 01.10.2024 14:38, Alejandro Vallejo wrote:
> @@ -148,7 +148,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
> lapic->length = sizeof(*lapic);
> /* Processor ID must match processor-object IDs in the DSDT. */
> lapic->acpi_processor_id = i;
> - lapic->apic_id = config->lapic_id(i);
> + lapic->apic_id = config->cpu_to_apicid[i];
Perhaps assert (like you do in an earlier patch) that the ID is small
enough?
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -84,7 +84,7 @@ struct acpi_config {
> unsigned long rsdp;
>
> /* x86-specific parameters */
> - uint32_t (*lapic_id)(unsigned cpu);
> + uint32_t *cpu_to_apicid; /* LUT mapping cpu id to (x2)APIC ID */
const uint32_t *?
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -1082,6 +1082,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>
> dom->container_type = XC_DOM_HVM_CONTAINER;
>
> +#if defined(__i386__) || defined(__x86_64__)
> + for ( uint32_t i = 0; i < info->max_vcpus; i++ )
Plain unsigned int?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86
2024-10-01 12:38 ` [PATCH v6 08/11] xen/lib: Add topology generator for x86 Alejandro Vallejo
@ 2024-10-09 14:45 ` Jan Beulich
2024-10-09 17:57 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 14:45 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 01.10.2024 14:38, Alejandro Vallejo wrote:
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -542,6 +542,22 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> const struct cpu_policy *guest,
> struct cpu_policy_errors *err);
>
> +/**
> + * Synthesise topology information in `p` given high-level constraints
> + *
> + * Topology is given in various fields accross several leaves, some of
> + * which are vendor-specific. This function uses the policy itself to
> + * derive such leaves from threads/core and cores/package.
Isn't it more like s/uses/fills/ (and the rest of the sentence then
possibly adjust some to match)? The policy looks to be purely an output
here (except for the vendor field).
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -2,6 +2,94 @@
>
> #include <xen/lib/x86/cpu-policy.h>
>
> +static unsigned int order(unsigned int n)
> +{
> + ASSERT(n); /* clz(0) is UB */
> +
> + return 8 * sizeof(n) - __builtin_clz(n);
> +}
> +
> +int x86_topo_from_parts(struct cpu_policy *p,
> + unsigned int threads_per_core,
> + unsigned int cores_per_pkg)
> +{
> + unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
What about the (admittedly absurd) case of this overflowing?
> + unsigned int apic_id_size;
> +
> + if ( !p || !threads_per_core || !cores_per_pkg )
> + return -EINVAL;
> +
> + p->basic.max_leaf = MAX(0xb, p->basic.max_leaf);
Better use the type-safe max() (and min() further down)?
> + memset(p->topo.raw, 0, sizeof(p->topo.raw));
> +
> + /* thread level */
> + p->topo.subleaf[0].nr_logical = threads_per_core;
> + p->topo.subleaf[0].id_shift = 0;
> + p->topo.subleaf[0].level = 0;
> + p->topo.subleaf[0].type = 1;
> + if ( threads_per_core > 1 )
> + p->topo.subleaf[0].id_shift = order(threads_per_core - 1);
> +
> + /* core level */
> + p->topo.subleaf[1].nr_logical = cores_per_pkg;
> + if ( p->x86_vendor == X86_VENDOR_INTEL )
> + p->topo.subleaf[1].nr_logical = threads_per_pkg;
> + p->topo.subleaf[1].id_shift = p->topo.subleaf[0].id_shift;
> + p->topo.subleaf[1].level = 1;
> + p->topo.subleaf[1].type = 2;
> + if ( cores_per_pkg > 1 )
> + p->topo.subleaf[1].id_shift += order(cores_per_pkg - 1);
> +
> + apic_id_size = p->topo.subleaf[1].id_shift;
> +
> + /*
> + * Contrary to what the name might seem to imply. HTT is an enabler for
> + * SMP and there's no harm in setting it even with a single vCPU.
> + */
> + p->basic.htt = true;
> + p->basic.lppp = MIN(0xff, threads_per_pkg);
> +
> + switch ( p->x86_vendor )
> + {
> + case X86_VENDOR_INTEL: {
> + struct cpuid_cache_leaf *sl = p->cache.subleaf;
> +
> + for ( size_t i = 0; sl->type &&
> + i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
> + {
> + sl->cores_per_package = cores_per_pkg - 1;
> + sl->threads_per_cache = threads_per_core - 1;
> + if ( sl->type == 3 /* unified cache */ )
> + sl->threads_per_cache = threads_per_pkg - 1;
I wasn't able to find documentation for this, well, anomaly. Can you please
point me at where this is spelled out?
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy
2024-10-01 12:38 ` [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy Alejandro Vallejo
@ 2024-10-09 14:53 ` Jan Beulich
2024-10-09 17:29 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-09 14:53 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 01.10.2024 14:38, Alejandro Vallejo wrote:
> Implements the helper for mapping vcpu_id to x2apic_id given a valid
> topology in a policy. The algo is written with the intention of
> extending it to leaves 0x1f and extended 0x26 in the future.
>
> Toolstack doesn't set leaf 0xb and the HVM default policy has it
> cleared, so the leaf is not implemented. In that case, the new helper
> just returns the legacy mapping.
Is the first sentence of this latter paragraph missing an "If" or "When"
at the beginning? As written I'm afraid I can't really make sense of it.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility
2024-10-09 9:40 ` Jan Beulich
@ 2024-10-09 15:57 ` Alejandro Vallejo
2024-10-10 7:37 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 15:57 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Roger Pau Monné, Anthony PERARD, Xen-devel
Hi,
On Wed Oct 9, 2024 at 10:40 AM BST, Jan Beulich wrote:
> On 01.10.2024 14:37, Alejandro Vallejo wrote:
> > --- a/xen/lib/x86/policy.c
> > +++ b/xen/lib/x86/policy.c
> > @@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> > #define FAIL_MSR(m) \
> > do { e.msr = (m); goto out; } while ( 0 )
> >
> > - if ( guest->basic.max_leaf > host->basic.max_leaf )
> > + /*
> > + * Old AMD hardware doesn't expose topology information in leaf 0xb. We
> > + * want to emulate that leaf with credible information because it must be
> > + * present on systems in which we emulate the x2APIC.
> > + *
> > + * For that reason, allow the max basic guest leaf to be larger than the
> > + * hosts' up until 0xb.
> > + */
> > + if ( guest->basic.max_leaf > 0xb &&
> > + guest->basic.max_leaf > host->basic.max_leaf )
> > FAIL_CPUID(0, NA);
> >
> > if ( guest->feat.max_subleaf > host->feat.max_subleaf )
>
> I'm concerned by this in multiple ways:
>
> 1) It's pretty ad hoc, and hence doesn't make clear how to deal with similar
> situations in the future.
I agree. I don't have a principled suggestion for how to deal with other cases
where we might have to bump the max leaf. It may be safe (as is here becasue
everything below it is either used or unimplemnted), but AFAIU some leaves
might be problematic to expose, even as zeroes. I suspect that's the problem
you hint at later on about AMX and AVX10?
>
> 2) Why would we permit going up to leaf 0xb when x2APIC is off in the respective
> leaf?
I assume you mean when the x2APIC is not emulated? One reason is to avoid a
migration barrier, as otherwise we can't migrate VMs created in "leaf
0xb"-capable hardware to non-"leaf 0xb"-capable even though the migration is
perfectly safe.
Also, it's benign and simplifies everything. Otherwise we have to find out
during early creation not only whether the host has leaf 0xb, but also whether
we're emulating an x2APIC or not.
Furthermore, not doing this would actively prevent emulating an x2APIC on AMD
Lisbon-like silicon even though it's fine to do so. Note that we have a broken
invariant in existing code where the x2APIC is emulated and leaf 0xb is not
exposed at all; not even to show the x2APIC IDs.
>
> 3) We similarly force a higher extended leaf in order to accommodate the LFENCE-
> is-dispatch-serializing bit. Yet there's no similar extra logic there in the
> function here.
That's done on the host policy though, so there's no clash.
In calculate_host_policy()...
```
/*
* For AMD/Hygon hardware before Zen3, we unilaterally modify LFENCE to be
* dispatch serialising for Spectre mitigations. Extend max_extd_leaf
* beyond what hardware supports, to include the feature leaf containing
* this information.
*/
if ( cpu_has_lfence_dispatch )
max_extd_leaf = max(max_extd_leaf, 0x80000021U);
```
One could imagine doing the same for leaf 0xb and dropping this patch, but then
we'd have to synthesise something on that leaf for hardware that doesn't have
it, which is a lot more annoying.
>
> 4) While there the guest vs host check won't matter, the situation with AMX and
> AVX10 leaves imo still wants considering here right away. IOW (taken together
> with at least 3) above) I think we need to first settle on a model for
> collectively all max (sub)leaf handling. That in particular needs to properly
> spell out who's responsible for what (tool stack vs Xen).
I'm not sure I follow. What's the situation with AMX and AVX10 that you refer
to? I'd assume that making ad-hoc decisions on this is pretty much unavoidable,
but maybe the solution to the problem you mention would highlight a more
general approach.
>
> Jan
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks
2024-10-08 15:41 ` Jan Beulich
@ 2024-10-09 16:11 ` Alejandro Vallejo
0 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 16:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel
On Tue Oct 8, 2024 at 4:41 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:37, Alejandro Vallejo wrote:
> > While doing this, factor out checks common to architectural and hidden
> > state.
> >
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > --
> > Last reviewed in the topology series v3. Fell under the cracks.
> >
> > https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/
>
> It's not the 1st patch in the series, and I can't spot anywhere that it is
> made clear that this one can go in ahead of patch 1. I may have overlooked
> something in the long-ish cover letter.
>
> Jan
Patch 1 is independent of almost everything. It merely needs to go in before
the final patch to avoid turning it into a bugfix. I put it on front under the
expectation that it wouldn't be very contentious. In retrospect, this one ought
to have taken its place, indeed.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area
2024-10-09 13:12 ` Jan Beulich
@ 2024-10-09 16:39 ` Alejandro Vallejo
0 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 16:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel, Andrew Cooper
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create
2024-10-09 13:28 ` Jan Beulich
@ 2024-10-09 16:44 ` Alejandro Vallejo
2024-10-10 7:46 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 16:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel
On Wed Oct 9, 2024 at 2:28 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > If toolstack were to upload LAPIC contexts as part of domain creation it
>
> If it were to - yes. But it doesn't, an peeking ahead in the series I also
> couldn't spot this changing. Hence I don#t think I see why this change
> would be needed, and why ...
Patch 10 does. It's the means by which (in a rather roundabout way)
toolstack overrides vlapic->hw.x2apic_id.
>
> > would encounter a problem were the architectural state does not reflect
> > the APIC ID in the hidden state. This patch ensures updates to the
> > hidden state trigger an update in the architectural registers so the
> > APIC ID in both is consistent.
> >
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> > ---
> > xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 02570f9dd63a..a8183c3023da 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> >
> > s->loaded.hw = 1;
> > if ( s->loaded.regs )
> > + {
> > + /*
> > + * We already processed architectural regs in lapic_load_regs(), so
> > + * this must be a migration. Fix up inconsistencies from any older Xen.
> > + */
> > lapic_load_fixup(s);
> > + }
> > + else
> > + {
> > + /*
> > + * We haven't seen architectural regs so this could be a migration or a
> > + * plain domain create. In the domain create case it's fine to modify
> > + * the architectural state to align it to the APIC ID that was just
> > + * uploaded and in the migrate case it doesn't matter because the
> > + * architectural state will be replaced by the LAPIC_REGS ctx later on.
> > + */
>
> ... a comment would need to mention a case that never really happens, thus
> only risking to cause confusion.
>
> Jan
I assume the "never really happens" is about the same as the previous
paragraph? If so, the same answer applies.
About the lack of ordering in the migrate stream the code already makes no
assumptions as to which HVM context blob might appear first in the vLAPIC area.
I'm not sure why, but I assumed it may be different on older Xen.
>
> > + if ( vlapic_x2apic_mode(s) )
> > + set_x2apic_id(s);
> > + else
> > + vlapic_set_reg(s, APIC_ID, SET_xAPIC_ID(s->hw.x2apic_id));
> > + }
> >
> > hvm_update_vlapic_mode(v);
> >
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
2024-10-09 14:03 ` Jan Beulich
@ 2024-10-09 17:19 ` Alejandro Vallejo
2024-10-10 7:49 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 17:19 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
Hi,
On Wed Oct 9, 2024 at 3:03 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > Make it so the APs expose their own APIC IDs in a LUT. We can use that
> > LUT to populate the MADT, decoupling the algorithm that relates CPU IDs
> > and APIC IDs from hvmloader.
> >
> > While at this also remove ap_callin, as writing the APIC ID may serve
> > the same purpose.
>
> ... on the assumption that no AP will have an APIC ID of zero.
>
> > @@ -341,11 +341,11 @@ int main(void)
> >
> > printf("CPU speed is %u MHz\n", get_cpu_mhz());
> >
> > + smp_initialise();
> > +
> > apic_setup();
> > pci_setup();
> >
> > - smp_initialise();
>
> I can see that you may want cpu_setup(0) to run ahead of apic_setup().
Not only that. This hunk ensures CPU_TO_X2APICID is populated ASAP for every
CPU. Reading zeroes where a non-zero APIC ID should be is fatal and tricky to
debug later. I tripped on enough "used the LUT before being set up" bugs to
really prefer initialising it before anyone has a chance to misuse it.
> Yet is it really appropriate to run boot_cpu() ahead of apic_setup() as well?
I would've agreed before the patches that went in to replace INIT-SIPI-SIPI
with hypercalls, but now that hvmloader is enlightened it has no real need for
the APIC to be configured. If feels weird because you wouldn't use this order
on bare metal. But it's fine under virt.
> At the very least it feels logically wrong, even if at the moment there
> may not be any direct dependency (which might change, however, down the
> road).
I suspect it feels wrong because you can't boot CPUs ahead of configuring your
APIC in real hardware. But hvmloader is always virtualized so that point is
moot. If anything, I'd be scared of adding code ahead of smp_initialise() that
relies on CPU_TO_X2APICID being set when it hasn't yet.
If you have a strong view on the matter I can remove this hunk and call
read_apic_id() from apic_setup(). But it wouldn't be my preference to do so.
>
> > --- a/tools/firmware/hvmloader/mp_tables.c
> > +++ b/tools/firmware/hvmloader/mp_tables.c
> > @@ -198,8 +198,10 @@ static void fill_mp_config_table(struct mp_config_table *mpct, int length)
> > /* fills in an MP processor entry for VCPU 'vcpu_id' */
> > static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
> > {
> > + ASSERT(CPU_TO_X2APICID[vcpu_id] < 0xFF );
>
> Nit: Excess blank before closing paren.
Oops, right.
>
> And of course this will need doing differently anyway once we get to
> support for more than 128 vCPU-s.
This is just a paranoia-driven assert to give quick feedback on the overflow of
the APIC ID later on. The entry in the MP-Table is a single octet long, so in
those cases we'd want to skip the table to begin with.
>
> > --- a/tools/firmware/hvmloader/smp.c
> > +++ b/tools/firmware/hvmloader/smp.c
> > @@ -29,7 +29,34 @@
> >
> > #include <xen/vcpu.h>
> >
> > -static int ap_callin;
> > +/**
> > + * Lookup table of x2APIC IDs.
> > + *
> > + * Each entry is populated its respective CPU as they come online. This is required
> > + * for generating the MADT with minimal assumptions about ID relationships.
> > + */
> > +uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
>
> I can kind of accept keeping it simple in the name (albeit - why all caps?),
> but at least the comment should mention that it may also be an xAPIC ID
> that's stored here.
I'll add that in the comment. I do want it to be x2apic in name though, so as
to make it obvious that it's LUT of 32bit items.
As for the caps, bad reasons. It used to be a macro and over time I kept
interpreting it as an indexed constant. Should be lowercase.
>
> > @@ -104,6 +132,12 @@ static void boot_cpu(unsigned int cpu)
> > void smp_initialise(void)
> > {
> > unsigned int i, nr_cpus = hvm_info->nr_vcpus;
> > + uint32_t ecx;
> > +
> > + cpuid(1, NULL, NULL, &ecx, NULL);
> > + has_x2apic = (ecx >> 21) & 1;
>
> Would be really nice to avoid the literal 21 here by including
> xen/arch-x86/cpufeatureset.h. Can this be arranged for?
I'll give that a go. hvmloader has given no shortage of headaches with its
quirky environment, so we'll see...
>
> Jan
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer
2024-10-09 14:25 ` Jan Beulich
@ 2024-10-09 17:20 ` Alejandro Vallejo
2024-10-11 16:17 ` Alejandro Vallejo
1 sibling, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 17:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Juergen Gross, Xen-devel
On Wed Oct 9, 2024 at 3:25 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > @@ -148,7 +148,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
> > lapic->length = sizeof(*lapic);
> > /* Processor ID must match processor-object IDs in the DSDT. */
> > lapic->acpi_processor_id = i;
> > - lapic->apic_id = config->lapic_id(i);
> > + lapic->apic_id = config->cpu_to_apicid[i];
>
> Perhaps assert (like you do in an earlier patch) that the ID is small
> enough?
>
> > --- a/tools/libacpi/libacpi.h
> > +++ b/tools/libacpi/libacpi.h
> > @@ -84,7 +84,7 @@ struct acpi_config {
> > unsigned long rsdp;
> >
> > /* x86-specific parameters */
> > - uint32_t (*lapic_id)(unsigned cpu);
> > + uint32_t *cpu_to_apicid; /* LUT mapping cpu id to (x2)APIC ID */
>
> const uint32_t *?
>
> > --- a/tools/libs/light/libxl_dom.c
> > +++ b/tools/libs/light/libxl_dom.c
> > @@ -1082,6 +1082,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> >
> > dom->container_type = XC_DOM_HVM_CONTAINER;
> >
> > +#if defined(__i386__) || defined(__x86_64__)
> > + for ( uint32_t i = 0; i < info->max_vcpus; i++ )
>
> Plain unsigned int?
>
> Jan
Sure to all three.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy
2024-10-09 14:53 ` Jan Beulich
@ 2024-10-09 17:29 ` Alejandro Vallejo
2024-10-10 7:55 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 17:29 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On Wed Oct 9, 2024 at 3:53 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > Implements the helper for mapping vcpu_id to x2apic_id given a valid
> > topology in a policy. The algo is written with the intention of
> > extending it to leaves 0x1f and extended 0x26 in the future.
> >
> > Toolstack doesn't set leaf 0xb and the HVM default policy has it
> > cleared, so the leaf is not implemented. In that case, the new helper
> > just returns the legacy mapping.
>
> Is the first sentence of this latter paragraph missing an "If" or "When"
> at the beginning? As written I'm afraid I can't really make sense of it.
>
> Jan
It's a statement of current affairs. Could be rewritten as...
The helper returns the legacy mapping when leaf 0xb is not implemented (as
is the case at the moment).
Does that look better?
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86
2024-10-09 14:45 ` Jan Beulich
@ 2024-10-09 17:57 ` Alejandro Vallejo
2024-10-10 7:54 ` Jan Beulich
0 siblings, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-09 17:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On Wed Oct 9, 2024 at 3:45 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > --- a/xen/include/xen/lib/x86/cpu-policy.h
> > +++ b/xen/include/xen/lib/x86/cpu-policy.h
> > @@ -542,6 +542,22 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> > const struct cpu_policy *guest,
> > struct cpu_policy_errors *err);
> >
> > +/**
> > + * Synthesise topology information in `p` given high-level constraints
> > + *
> > + * Topology is given in various fields accross several leaves, some of
> > + * which are vendor-specific. This function uses the policy itself to
> > + * derive such leaves from threads/core and cores/package.
>
> Isn't it more like s/uses/fills/ (and the rest of the sentence then
> possibly adjust some to match)? The policy looks to be purely an output
> here (except for the vendor field).
Sure.
>
> > --- a/xen/lib/x86/policy.c
> > +++ b/xen/lib/x86/policy.c
> > @@ -2,6 +2,94 @@
> >
> > #include <xen/lib/x86/cpu-policy.h>
> >
> > +static unsigned int order(unsigned int n)
> > +{
> > + ASSERT(n); /* clz(0) is UB */
> > +
> > + return 8 * sizeof(n) - __builtin_clz(n);
> > +}
> > +
> > +int x86_topo_from_parts(struct cpu_policy *p,
> > + unsigned int threads_per_core,
> > + unsigned int cores_per_pkg)
> > +{
> > + unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
>
> What about the (admittedly absurd) case of this overflowing?
Each of them individually could overflow the fields in which they are used.
Does returning EINVAL if either threads_per_core or cores_per_pkg overflow the
INTEL structure j
>
> > + unsigned int apic_id_size;
> > +
> > + if ( !p || !threads_per_core || !cores_per_pkg )
> > + return -EINVAL;
> > +
> > + p->basic.max_leaf = MAX(0xb, p->basic.max_leaf);
>
> Better use the type-safe max() (and min() further down)?
Sure
>
> > + memset(p->topo.raw, 0, sizeof(p->topo.raw));
> > +
> > + /* thread level */
> > + p->topo.subleaf[0].nr_logical = threads_per_core;
> > + p->topo.subleaf[0].id_shift = 0;
> > + p->topo.subleaf[0].level = 0;
> > + p->topo.subleaf[0].type = 1;
> > + if ( threads_per_core > 1 )
> > + p->topo.subleaf[0].id_shift = order(threads_per_core - 1);
> > +
> > + /* core level */
> > + p->topo.subleaf[1].nr_logical = cores_per_pkg;
> > + if ( p->x86_vendor == X86_VENDOR_INTEL )
> > + p->topo.subleaf[1].nr_logical = threads_per_pkg;
> > + p->topo.subleaf[1].id_shift = p->topo.subleaf[0].id_shift;
> > + p->topo.subleaf[1].level = 1;
> > + p->topo.subleaf[1].type = 2;
> > + if ( cores_per_pkg > 1 )
> > + p->topo.subleaf[1].id_shift += order(cores_per_pkg - 1);
> > +
> > + apic_id_size = p->topo.subleaf[1].id_shift;
> > +
> > + /*
> > + * Contrary to what the name might seem to imply. HTT is an enabler for
> > + * SMP and there's no harm in setting it even with a single vCPU.
> > + */
> > + p->basic.htt = true;
> > + p->basic.lppp = MIN(0xff, threads_per_pkg);
> > +
> > + switch ( p->x86_vendor )
> > + {
> > + case X86_VENDOR_INTEL: {
> > + struct cpuid_cache_leaf *sl = p->cache.subleaf;
> > +
> > + for ( size_t i = 0; sl->type &&
> > + i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
> > + {
> > + sl->cores_per_package = cores_per_pkg - 1;
> > + sl->threads_per_cache = threads_per_core - 1;
> > + if ( sl->type == 3 /* unified cache */ )
> > + sl->threads_per_cache = threads_per_pkg - 1;
>
> I wasn't able to find documentation for this, well, anomaly. Can you please
> point me at where this is spelled out?
That's showing all unified caches as caches covering the whole package. We
could do it the other way around (but I don't want to reverse engineer what the
host policy says because that's irrelevant). There's nothing in the SDM (AFAIK)
forcing L2 or L3 to behave one way or another, so we get to choose. I thought
it more helpful to make all unified caches unified across the package. to give
more information in the leaf.
My own system exposes 2 unified caches (data trimmed for space):
``` cpuid
deterministic cache parameters (4):
--- cache 0 ---
cache type = data cache (1)
cache level = 0x1 (1)
maximum IDs for CPUs sharing cache = 0x1 (1)
maximum IDs for cores in pkg = 0xf (15)
--- cache 1 ---
cache type = instruction cache (2)
cache level = 0x1 (1)
maximum IDs for CPUs sharing cache = 0x1 (1)
maximum IDs for cores in pkg = 0xf (15)
--- cache 2 ---
cache type = unified cache (3)
cache level = 0x2 (2)
maximum IDs for CPUs sharing cache = 0x1 (1)
maximum IDs for cores in pkg = 0xf (15)
--- cache 3 ---
cache type = unified cache (3)
cache level = 0x3 (3)
maximum IDs for CPUs sharing cache = 0x1f (31)
maximum IDs for cores in pkg = 0xf (15)
--- cache 4 ---
cache type = no more caches (0)
```
>
> Jan
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
2024-10-09 9:40 ` Jan Beulich
@ 2024-10-09 21:58 ` Andrew Cooper
1 sibling, 0 replies; 37+ messages in thread
From: Andrew Cooper @ 2024-10-09 21:58 UTC (permalink / raw)
To: Alejandro Vallejo, Xen-devel
Cc: Jan Beulich, Roger Pau Monné, Anthony PERARD
On 01/10/2024 1:37 pm, Alejandro Vallejo wrote:
> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
> index f033d22785be..63bc96451d2c 100644
> --- a/xen/lib/x86/policy.c
> +++ b/xen/lib/x86/policy.c
> @@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
> #define FAIL_MSR(m) \
> do { e.msr = (m); goto out; } while ( 0 )
>
> - if ( guest->basic.max_leaf > host->basic.max_leaf )
> + /*
> + * Old AMD hardware doesn't expose topology information in leaf 0xb. We
> + * want to emulate that leaf with credible information because it must be
> + * present on systems in which we emulate the x2APIC.
> + *
> + * For that reason, allow the max basic guest leaf to be larger than the
> + * hosts' up until 0xb.
> + */
> + if ( guest->basic.max_leaf > 0xb &&
> + guest->basic.max_leaf > host->basic.max_leaf )
> FAIL_CPUID(0, NA);
>
> if ( guest->feat.max_subleaf > host->feat.max_subleaf )
This isn't appropriate. You're violating the property that a guest
policy should be bounded by the max (called host here).
Instead, the max policies basic.max_leaf should be at least 0xb.
This is another case where the max wants to be higher with default
inheriting from host.
But it occurs to me that with max being an not-sane-in-isolation policy
anyway, we probably want to end up setting max->$foo.max_leaf to the
appropriate ARRAY_SIZE()'s.
For any leaves within the bounds that Xen knows, it's fine for the
toolstack to set values there even beyond host max leaf, as long as it
doesn't violate other consistency checks.
So, please fix this by adjusting
calculate_{pv,hvm}_{max,default}_policy() in Xen.
Sadly I think it will end up a tad ugly, but don't worry too much.
We've got a number of host==default but max special fields, and I'm
wanting to get a few more examples before choosing how best to
rationalise it to reduce opencoding.
~Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility
2024-10-09 15:57 ` Alejandro Vallejo
@ 2024-10-10 7:37 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-10 7:37 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Roger Pau Monné, Anthony PERARD, Xen-devel, Andrew Cooper
On 09.10.2024 17:57, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 10:40 AM BST, Jan Beulich wrote:
>> On 01.10.2024 14:37, Alejandro Vallejo wrote:
>>> --- a/xen/lib/x86/policy.c
>>> +++ b/xen/lib/x86/policy.c
>>> @@ -15,7 +15,16 @@ int x86_cpu_policies_are_compatible(const struct cpu_policy *host,
>>> #define FAIL_MSR(m) \
>>> do { e.msr = (m); goto out; } while ( 0 )
>>>
>>> - if ( guest->basic.max_leaf > host->basic.max_leaf )
>>> + /*
>>> + * Old AMD hardware doesn't expose topology information in leaf 0xb. We
>>> + * want to emulate that leaf with credible information because it must be
>>> + * present on systems in which we emulate the x2APIC.
>>> + *
>>> + * For that reason, allow the max basic guest leaf to be larger than the
>>> + * hosts' up until 0xb.
>>> + */
>>> + if ( guest->basic.max_leaf > 0xb &&
>>> + guest->basic.max_leaf > host->basic.max_leaf )
>>> FAIL_CPUID(0, NA);
>>>
>>> if ( guest->feat.max_subleaf > host->feat.max_subleaf )
>>
>> I'm concerned by this in multiple ways:
>>
>> 1) It's pretty ad hoc, and hence doesn't make clear how to deal with similar
>> situations in the future.
>
> I agree. I don't have a principled suggestion for how to deal with other cases
> where we might have to bump the max leaf. It may be safe (as is here becasue
> everything below it is either used or unimplemnted), but AFAIU some leaves
> might be problematic to expose, even as zeroes. I suspect that's the problem
> you hint at later on about AMX and AVX10?
Not exactly, but perhaps somewhat related (see below).
>> 2) Why would we permit going up to leaf 0xb when x2APIC is off in the respective
>> leaf?
>
> I assume you mean when the x2APIC is not emulated? One reason is to avoid a
> migration barrier, as otherwise we can't migrate VMs created in "leaf
> 0xb"-capable hardware to non-"leaf 0xb"-capable even though the migration is
> perfectly safe.
Leaf 0xb ought to be synthesized anyway (to match the guest's topology);
hardware capabilities hence don't matter here.
> Also, it's benign and simplifies everything. Otherwise we have to find out
> during early creation not only whether the host has leaf 0xb, but also whether
> we're emulating an x2APIC or not.
The policy passed by the tool stack will tell you what the choice there was.
> Furthermore, not doing this would actively prevent emulating an x2APIC on AMD
> Lisbon-like silicon even though it's fine to do so.
I'm afraid I don't understand this. If the tool stack cleared the x2APIC bit,
x2APIC ought to not be emulated. If it sets it (as permitted by the max
policy), x2APIC would be emulated.
> Note that we have a broken
> invariant in existing code where the x2APIC is emulated and leaf 0xb is not
> exposed at all; not even to show the x2APIC IDs.
Well, fixing this is what this series is about, isn't it?
>> 3) We similarly force a higher extended leaf in order to accommodate the LFENCE-
>> is-dispatch-serializing bit. Yet there's no similar extra logic there in the
>> function here.
>
> That's done on the host policy though, so there's no clash.
There's no clash, sure, but ...
> In calculate_host_policy()...
>
> ```
> /*
> * For AMD/Hygon hardware before Zen3, we unilaterally modify LFENCE to be
> * dispatch serialising for Spectre mitigations. Extend max_extd_leaf
> * beyond what hardware supports, to include the feature leaf containing
> * this information.
> */
> if ( cpu_has_lfence_dispatch )
> max_extd_leaf = max(max_extd_leaf, 0x80000021U);
> ```
>
> One could imagine doing the same for leaf 0xb and dropping this patch, but then
> we'd have to synthesise something on that leaf for hardware that doesn't have
> it, which is a lot more annoying.
... we're doing things one way there and another way here. Which is generally
undesirable imo.
>> 4) While there the guest vs host check won't matter, the situation with AMX and
>> AVX10 leaves imo still wants considering here right away. IOW (taken together
>> with at least 3) above) I think we need to first settle on a model for
>> collectively all max (sub)leaf handling. That in particular needs to properly
>> spell out who's responsible for what (tool stack vs Xen).
>
> I'm not sure I follow. What's the situation with AMX and AVX10 that you refer
> to?
See the prereq series to both, most recently posted at
https://lists.xen.org/archives/html/xen-devel/2024-08/msg00591.html
That's hacky; Andrew has indicated that he'd like to take care of this (mostly)
in the tool stack instead. Yet so far nothing has surfaced, hence I'm keeping
to have this dependency for both series.
Jan
> I'd assume that making ad-hoc decisions on this is pretty much unavoidable,
> but maybe the solution to the problem you mention would highlight a more
> general approach.
>
> Cheers,
> Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create
2024-10-09 16:44 ` Alejandro Vallejo
@ 2024-10-10 7:46 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-10 7:46 UTC (permalink / raw)
To: Alejandro Vallejo; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel
On 09.10.2024 18:44, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 2:28 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> If toolstack were to upload LAPIC contexts as part of domain creation it
>>
>> If it were to - yes. But it doesn't, an peeking ahead in the series I also
>> couldn't spot this changing. Hence I don#t think I see why this change
>> would be needed, and why ...
>
> Patch 10 does. It's the means by which (in a rather roundabout way)
> toolstack overrides vlapic->hw.x2apic_id.
Oh, indeed - I managed to not spot this. I think you want to either re-word
the description here to make clear there's actually a plan to do what is
being said as purely hypothetical, or simply fold the patches.
>>> would encounter a problem were the architectural state does not reflect
>>> the APIC ID in the hidden state. This patch ensures updates to the
>>> hidden state trigger an update in the architectural registers so the
>>> APIC ID in both is consistent.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>> xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>>> index 02570f9dd63a..a8183c3023da 100644
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
>>>
>>> s->loaded.hw = 1;
>>> if ( s->loaded.regs )
>>> + {
>>> + /*
>>> + * We already processed architectural regs in lapic_load_regs(), so
>>> + * this must be a migration. Fix up inconsistencies from any older Xen.
>>> + */
>>> lapic_load_fixup(s);
>>> + }
>>> + else
>>> + {
>>> + /*
>>> + * We haven't seen architectural regs so this could be a migration or a
>>> + * plain domain create. In the domain create case it's fine to modify
>>> + * the architectural state to align it to the APIC ID that was just
>>> + * uploaded and in the migrate case it doesn't matter because the
>>> + * architectural state will be replaced by the LAPIC_REGS ctx later on.
>>> + */
>>
>> ... a comment would need to mention a case that never really happens, thus
>> only risking to cause confusion.
>
> I assume the "never really happens" is about the same as the previous
> paragraph? If so, the same answer applies.
Yes.
> About the lack of ordering in the migrate stream the code already makes no
> assumptions as to which HVM context blob might appear first in the vLAPIC area.
>
> I'm not sure why, but I assumed it may be different on older Xen.
I agree with being flexible here; I'm not aware of anything in the migration spec
(and certainly not in the unwritten v1 one) mandating particular ordering.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
2024-10-09 17:19 ` Alejandro Vallejo
@ 2024-10-10 7:49 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-10 7:49 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 09.10.2024 19:19, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 3:03 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> Make it so the APs expose their own APIC IDs in a LUT. We can use that
>>> LUT to populate the MADT, decoupling the algorithm that relates CPU IDs
>>> and APIC IDs from hvmloader.
>>>
>>> While at this also remove ap_callin, as writing the APIC ID may serve
>>> the same purpose.
>>
>> ... on the assumption that no AP will have an APIC ID of zero.
>>
>>> @@ -341,11 +341,11 @@ int main(void)
>>>
>>> printf("CPU speed is %u MHz\n", get_cpu_mhz());
>>>
>>> + smp_initialise();
>>> +
>>> apic_setup();
>>> pci_setup();
>>>
>>> - smp_initialise();
>>
>> I can see that you may want cpu_setup(0) to run ahead of apic_setup().
>
> Not only that. This hunk ensures CPU_TO_X2APICID is populated ASAP for every
> CPU. Reading zeroes where a non-zero APIC ID should be is fatal and tricky to
> debug later. I tripped on enough "used the LUT before being set up" bugs to
> really prefer initialising it before anyone has a chance to misuse it.
>
>> Yet is it really appropriate to run boot_cpu() ahead of apic_setup() as well?
>
> I would've agreed before the patches that went in to replace INIT-SIPI-SIPI
> with hypercalls, but now that hvmloader is enlightened it has no real need for
> the APIC to be configured. If feels weird because you wouldn't use this order
> on bare metal. But it's fine under virt.
>
>> At the very least it feels logically wrong, even if at the moment there
>> may not be any direct dependency (which might change, however, down the
>> road).
>
> I suspect it feels wrong because you can't boot CPUs ahead of configuring your
> APIC in real hardware. But hvmloader is always virtualized so that point is
> moot. If anything, I'd be scared of adding code ahead of smp_initialise() that
> relies on CPU_TO_X2APICID being set when it hasn't yet.
>
> If you have a strong view on the matter I can remove this hunk and call
> read_apic_id() from apic_setup(). But it wouldn't be my preference to do so.
No, with the explanations above it's fine to stay as is. Just that some of
what you say above needs to move into the patch description.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86
2024-10-09 17:57 ` Alejandro Vallejo
@ 2024-10-10 7:54 ` Jan Beulich
2024-10-15 13:08 ` Alejandro Vallejo
0 siblings, 1 reply; 37+ messages in thread
From: Jan Beulich @ 2024-10-10 7:54 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 09.10.2024 19:57, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 3:45 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> --- a/xen/lib/x86/policy.c
>>> +++ b/xen/lib/x86/policy.c
>>> @@ -2,6 +2,94 @@
>>>
>>> #include <xen/lib/x86/cpu-policy.h>
>>>
>>> +static unsigned int order(unsigned int n)
>>> +{
>>> + ASSERT(n); /* clz(0) is UB */
>>> +
>>> + return 8 * sizeof(n) - __builtin_clz(n);
>>> +}
>>> +
>>> +int x86_topo_from_parts(struct cpu_policy *p,
>>> + unsigned int threads_per_core,
>>> + unsigned int cores_per_pkg)
>>> +{
>>> + unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
>>
>> What about the (admittedly absurd) case of this overflowing?
>
> Each of them individually could overflow the fields in which they are used.
>
> Does returning EINVAL if either threads_per_core or cores_per_pkg overflow the
> INTEL structure j
The sentence looks unfinished, so I can only vaguely say that my answer to
the question would likely be "yes".
>>> + switch ( p->x86_vendor )
>>> + {
>>> + case X86_VENDOR_INTEL: {
>>> + struct cpuid_cache_leaf *sl = p->cache.subleaf;
>>> +
>>> + for ( size_t i = 0; sl->type &&
>>> + i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
>>> + {
>>> + sl->cores_per_package = cores_per_pkg - 1;
>>> + sl->threads_per_cache = threads_per_core - 1;
>>> + if ( sl->type == 3 /* unified cache */ )
>>> + sl->threads_per_cache = threads_per_pkg - 1;
>>
>> I wasn't able to find documentation for this, well, anomaly. Can you please
>> point me at where this is spelled out?
>
> That's showing all unified caches as caches covering the whole package. We
> could do it the other way around (but I don't want to reverse engineer what the
> host policy says because that's irrelevant). There's nothing in the SDM (AFAIK)
> forcing L2 or L3 to behave one way or another, so we get to choose. I thought
> it more helpful to make all unified caches unified across the package. to give
> more information in the leaf.
>
> My own system exposes 2 unified caches (data trimmed for space):
>
> ``` cpuid
>
> deterministic cache parameters (4):
> --- cache 0 ---
> cache type = data cache (1)
> cache level = 0x1 (1)
> maximum IDs for CPUs sharing cache = 0x1 (1)
> maximum IDs for cores in pkg = 0xf (15)
> --- cache 1 ---
> cache type = instruction cache (2)
> cache level = 0x1 (1)
> maximum IDs for CPUs sharing cache = 0x1 (1)
> maximum IDs for cores in pkg = 0xf (15)
> --- cache 2 ---
> cache type = unified cache (3)
> cache level = 0x2 (2)
> maximum IDs for CPUs sharing cache = 0x1 (1)
Note how this is different ...
> maximum IDs for cores in pkg = 0xf (15)
> --- cache 3 ---
> cache type = unified cache (3)
> cache level = 0x3 (3)
> maximum IDs for CPUs sharing cache = 0x1f (31)
... from this, whereas your code would make it the same.
Especially if this is something you do beyond / outside the spec, it imo
needs reasoning about in fair detail in the description.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy
2024-10-09 17:29 ` Alejandro Vallejo
@ 2024-10-10 7:55 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-10 7:55 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On 09.10.2024 19:29, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 3:53 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> Implements the helper for mapping vcpu_id to x2apic_id given a valid
>>> topology in a policy. The algo is written with the intention of
>>> extending it to leaves 0x1f and extended 0x26 in the future.
>>>
>>> Toolstack doesn't set leaf 0xb and the HVM default policy has it
>>> cleared, so the leaf is not implemented. In that case, the new helper
>>> just returns the legacy mapping.
>>
>> Is the first sentence of this latter paragraph missing an "If" or "When"
>> at the beginning? As written I'm afraid I can't really make sense of it.
>
> It's a statement of current affairs. Could be rewritten as...
>
> The helper returns the legacy mapping when leaf 0xb is not implemented (as
> is the case at the moment).
>
> Does that look better?
Yes.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer
2024-10-09 14:25 ` Jan Beulich
2024-10-09 17:20 ` Alejandro Vallejo
@ 2024-10-11 16:17 ` Alejandro Vallejo
2024-10-14 6:26 ` Jan Beulich
1 sibling, 1 reply; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-11 16:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Juergen Gross, Xen-devel
On Wed Oct 9, 2024 at 3:25 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> > @@ -148,7 +148,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
> > lapic->length = sizeof(*lapic);
> > /* Processor ID must match processor-object IDs in the DSDT. */
> > lapic->acpi_processor_id = i;
> > - lapic->apic_id = config->lapic_id(i);
> > + lapic->apic_id = config->cpu_to_apicid[i];
>
> Perhaps assert (like you do in an earlier patch) that the ID is small
> enough?
Actually, I just remembered why I didn't. libacpi is pulled into libxl, which
is integrated into libvirt. A failed assert here would kill the application,
which is not very nice.
HVM is already protected by the mp tables assert, so I'm not terribly worried
about it and, while PVH is not, it would crash pretty quickly due to the
corruption.
I'd rather have the domain crashing rather than virt-manager.
>
> > --- a/tools/libacpi/libacpi.h
> > +++ b/tools/libacpi/libacpi.h
> > @@ -84,7 +84,7 @@ struct acpi_config {
> > unsigned long rsdp;
> >
> > /* x86-specific parameters */
> > - uint32_t (*lapic_id)(unsigned cpu);
> > + uint32_t *cpu_to_apicid; /* LUT mapping cpu id to (x2)APIC ID */
>
> const uint32_t *?
>
> > --- a/tools/libs/light/libxl_dom.c
> > +++ b/tools/libs/light/libxl_dom.c
> > @@ -1082,6 +1082,11 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> >
> > dom->container_type = XC_DOM_HVM_CONTAINER;
> >
> > +#if defined(__i386__) || defined(__x86_64__)
> > + for ( uint32_t i = 0; i < info->max_vcpus; i++ )
>
> Plain unsigned int?
>
> Jan
Sigh... and this didn't have the libxl style either.
I really hate this style mix we have :/
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer
2024-10-11 16:17 ` Alejandro Vallejo
@ 2024-10-14 6:26 ` Jan Beulich
0 siblings, 0 replies; 37+ messages in thread
From: Jan Beulich @ 2024-10-14 6:26 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Juergen Gross, Xen-devel
On 11.10.2024 18:17, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 3:25 PM BST, Jan Beulich wrote:
>> On 01.10.2024 14:38, Alejandro Vallejo wrote:
>>> @@ -148,7 +148,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt *ctxt,
>>> lapic->length = sizeof(*lapic);
>>> /* Processor ID must match processor-object IDs in the DSDT. */
>>> lapic->acpi_processor_id = i;
>>> - lapic->apic_id = config->lapic_id(i);
>>> + lapic->apic_id = config->cpu_to_apicid[i];
>>
>> Perhaps assert (like you do in an earlier patch) that the ID is small
>> enough?
>
> Actually, I just remembered why I didn't. libacpi is pulled into libxl, which
> is integrated into libvirt. A failed assert here would kill the application,
> which is not very nice.
>
> HVM is already protected by the mp tables assert, so I'm not terribly worried
> about it and, while PVH is not, it would crash pretty quickly due to the
> corruption.
>
> I'd rather have the domain crashing rather than virt-manager.
Fair enough.
Jan
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v6 08/11] xen/lib: Add topology generator for x86
2024-10-10 7:54 ` Jan Beulich
@ 2024-10-15 13:08 ` Alejandro Vallejo
0 siblings, 0 replies; 37+ messages in thread
From: Alejandro Vallejo @ 2024-10-15 13:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Xen-devel
On Thu Oct 10, 2024 at 8:54 AM BST, Jan Beulich wrote:
> On 09.10.2024 19:57, Alejandro Vallejo wrote:
> > On Wed Oct 9, 2024 at 3:45 PM BST, Jan Beulich wrote:
> >> On 01.10.2024 14:38, Alejandro Vallejo wrote:
> >>> --- a/xen/lib/x86/policy.c
> >>> +++ b/xen/lib/x86/policy.c
> >>> @@ -2,6 +2,94 @@
> >>>
> >>> #include <xen/lib/x86/cpu-policy.h>
> >>>
> >>> +static unsigned int order(unsigned int n)
> >>> +{
> >>> + ASSERT(n); /* clz(0) is UB */
> >>> +
> >>> + return 8 * sizeof(n) - __builtin_clz(n);
> >>> +}
> >>> +
> >>> +int x86_topo_from_parts(struct cpu_policy *p,
> >>> + unsigned int threads_per_core,
> >>> + unsigned int cores_per_pkg)
> >>> +{
> >>> + unsigned int threads_per_pkg = threads_per_core * cores_per_pkg;
> >>
> >> What about the (admittedly absurd) case of this overflowing?
> >
> > Each of them individually could overflow the fields in which they are used.
> >
> > Does returning EINVAL if either threads_per_core or cores_per_pkg overflow the
> > INTEL structure j
>
> The sentence looks unfinished, so I can only vaguely say that my answer to
> the question would likely be "yes".
It was indeed. Regardless, the number of bits available in Intel's cache
subleaves is rather limited, so I'll be clipping those to the maximum on
overflow and...
>
> >>> + switch ( p->x86_vendor )
> >>> + {
> >>> + case X86_VENDOR_INTEL: {
> >>> + struct cpuid_cache_leaf *sl = p->cache.subleaf;
> >>> +
> >>> + for ( size_t i = 0; sl->type &&
> >>> + i < ARRAY_SIZE(p->cache.raw); i++, sl++ )
> >>> + {
> >>> + sl->cores_per_package = cores_per_pkg - 1;
> >>> + sl->threads_per_cache = threads_per_core - 1;
> >>> + if ( sl->type == 3 /* unified cache */ )
> >>> + sl->threads_per_cache = threads_per_pkg - 1;
> >>
> >> I wasn't able to find documentation for this, well, anomaly. Can you please
> >> point me at where this is spelled out?
> >
> > That's showing all unified caches as caches covering the whole package. We
> > could do it the other way around (but I don't want to reverse engineer what the
> > host policy says because that's irrelevant). There's nothing in the SDM (AFAIK)
> > forcing L2 or L3 to behave one way or another, so we get to choose. I thought
> > it more helpful to make all unified caches unified across the package. to give
> > more information in the leaf.
> >
> > My own system exposes 2 unified caches (data trimmed for space):
> >
> > ``` cpuid
> >
> > deterministic cache parameters (4):
> > --- cache 0 ---
> > cache type = data cache (1)
> > cache level = 0x1 (1)
> > maximum IDs for CPUs sharing cache = 0x1 (1)
> > maximum IDs for cores in pkg = 0xf (15)
> > --- cache 1 ---
> > cache type = instruction cache (2)
> > cache level = 0x1 (1)
> > maximum IDs for CPUs sharing cache = 0x1 (1)
> > maximum IDs for cores in pkg = 0xf (15)
> > --- cache 2 ---
> > cache type = unified cache (3)
> > cache level = 0x2 (2)
> > maximum IDs for CPUs sharing cache = 0x1 (1)
>
> Note how this is different ...
>
> > maximum IDs for cores in pkg = 0xf (15)
> > --- cache 3 ---
> > cache type = unified cache (3)
> > cache level = 0x3 (3)
> > maximum IDs for CPUs sharing cache = 0x1f (31)
>
> ... from this, whereas your code would make it the same.
>
> Especially if this is something you do beyond / outside the spec, it imo
> needs reasoning about in fair detail in the description.
... given the risk of clipping, I'll get rid of that conditional too to make it
easier for a non-clipped number to be reported.
I'll write in the commit message the behaviour on overflow for these leaves.
>
> Jan
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2024-10-15 13:09 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-01 12:37 [PATCH v6 00/11] x86: Expose consistent topology to guests Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 01/11] lib/x86: Relax checks about policy compatibility Alejandro Vallejo
2024-10-09 9:40 ` Jan Beulich
2024-10-09 15:57 ` Alejandro Vallejo
2024-10-10 7:37 ` Jan Beulich
2024-10-09 21:58 ` Andrew Cooper
2024-10-01 12:37 ` [PATCH v6 02/11] x86/vlapic: Move lapic migration checks to the check hooks Alejandro Vallejo
2024-10-08 15:41 ` Jan Beulich
2024-10-09 16:11 ` Alejandro Vallejo
2024-10-01 12:37 ` [PATCH v6 03/11] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area Alejandro Vallejo
2024-10-09 13:12 ` Jan Beulich
2024-10-09 16:39 ` Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create Alejandro Vallejo
2024-10-09 13:28 ` Jan Beulich
2024-10-09 16:44 ` Alejandro Vallejo
2024-10-10 7:46 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves Alejandro Vallejo
2024-10-09 14:03 ` Jan Beulich
2024-10-09 17:19 ` Alejandro Vallejo
2024-10-10 7:49 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 06/11] tools/libacpi: Use LUT of APIC IDs rather than function pointer Alejandro Vallejo
2024-10-09 14:25 ` Jan Beulich
2024-10-09 17:20 ` Alejandro Vallejo
2024-10-11 16:17 ` Alejandro Vallejo
2024-10-14 6:26 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 07/11] tools/libguest: Always set vCPU context in vcpu_hvm() Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 08/11] xen/lib: Add topology generator for x86 Alejandro Vallejo
2024-10-09 14:45 ` Jan Beulich
2024-10-09 17:57 ` Alejandro Vallejo
2024-10-10 7:54 ` Jan Beulich
2024-10-15 13:08 ` Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 09/11] xen/x86: Derive topologically correct x2APIC IDs from the policy Alejandro Vallejo
2024-10-09 14:53 ` Jan Beulich
2024-10-09 17:29 ` Alejandro Vallejo
2024-10-10 7:55 ` Jan Beulich
2024-10-01 12:38 ` [PATCH v6 10/11] tools/libguest: Set distinct x2APIC IDs for each vCPU Alejandro Vallejo
2024-10-01 12:38 ` [PATCH v6 11/11] tools/x86: Synthesise domain topologies Alejandro Vallejo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.