| Message ID | 20241001123807.605-6-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: > 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
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
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
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__ */
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(-)