[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |