Re: [PATCH 1/3] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jan 28, 2025 at 04:33:40PM +0000, Alejandro Vallejo wrote:
> Make it so the APs expose their own APIC IDs in a lookup table (LUT). We
> can use that LUT to populate the MADT, decoupling the algorithm that
> relates CPU IDs and APIC IDs from hvmloader.
> 
> Modified the printf to also print the APIC ID of each CPU, as well as
> fixing a (benign) wrong specifier being used for the vcpu id.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> Changes from the v7 version of this patch in the longer topology series:
>   * s/cpu_to_x2apicid/cpu_to_apicid/
>     * Though, as I already stated, I don't think this is a good idea.
>   * Dynamically size cpu_to_apicid rather than using HVM_MAX_VCPUS.
>   * Got rid of the ap_callin removal. It's not as trivial if we don't
>     want to assume cpu0 always has apicid=0. Part of the complaints on
>     the previous versions involved the inability to do that.
>   * For debugging sanity, I've added the apicid to the CPU boot printf.
>       * Later on, toolstack will choose the APIC IDs and it's helpful
>         to know the relationship in the logs.
>       * While at it, fix the vcpu specifier s/%d/%u/
>   * Check for leaf 0xb while probing for x2apic support.
> ---
>  tools/firmware/hvmloader/smp.c | 43 +++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
> index 1b940cefd071..c61ed524f947 100644
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -31,9 +31,38 @@
>  
>  static int ap_callin;
>  
> +/** True if x2apic support is exposed to the guest. */
> +static bool has_x2apic;
> +
> +/**
> + * Lookup table of (x2)APIC IDs.

Not sure you need to explicitly mention (x2) in the comment?  It will
either be xAPIC or x2APIC IDs, but just using "APIC IDs" should cover
both unambiguously?

> + *
> + * Each entry is populated for its respective CPU as they come online. This is
> + * required for generating the MADT with minimal assumptions about ID
> + * relationships.
> + */
> +uint32_t *cpu_to_apicid;
> +
> +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;
> +    }
> +
> +    return apic_id;
> +}
> +
>  static void cpu_setup(unsigned int cpu)
>  {
> -    printf(" - CPU%d ... ", cpu);
> +    uint32_t apicid = cpu_to_apicid[cpu] = read_apic_id();
> +
> +    printf(" - CPU%u[%u] ... ", cpu, apicid);

I would explicitly name the field in the print:

" - CPU%u APIC ID %u ... "

As otherwise it's not obvious what the value in the braces is (and you
have to go look at the code).

>      cacheattr_init();
>      printf("done.\n");
>  
> @@ -104,8 +133,20 @@ static void boot_cpu(unsigned int cpu)
>  void smp_initialise(void)
>  {
>      unsigned int i, nr_cpus = hvm_info->nr_vcpus;
> +    uint32_t ecx, max_leaf;

Noticed you could narrow the scope of ecx, but at the price of
introducing the definition line inside of the if condition.  I don't
have a strong opinion, and I assume you prefer it this way, which is
fine IMO.

Thanks, Roger.