| Message ID | 20241001123807.605-7-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
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
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
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
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
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;
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(-)