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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Tue Jan 28, 2025 at 5:59 PM GMT, Roger Pau Monné wrote:
> 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@xxxxxxxxx>
> > ---
> > 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?

Sure.

>
> > + *
> > + * 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).

Sure.

>
> >      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.

I marginally prefer it this way, but I don't particularly care. I wanted to
avoid one more line in a hunk that's already quite high for a single CPUID
check. 

Cheers,
Alejandro