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
