Re: [PATCH v6 05/11] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Jan Beulich <jbeulich@xxxxxxxx>]

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