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

["Alejandro Vallejo" <alejandro.vallejo@cloud.com>]

Hi,

On Wed Oct 9, 2024 at 3:03 PM BST, Jan Beulich wrote:
> 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().

Not only that. This hunk ensures CPU_TO_X2APICID is populated ASAP for every
CPU. Reading zeroes where a non-zero APIC ID should be is fatal and tricky to
debug later. I tripped on enough "used the LUT before being set up" bugs to
really prefer initialising it before anyone has a chance to misuse it.

> Yet is it really appropriate to run boot_cpu() ahead of apic_setup() as well?

I would've agreed before the patches that went in to replace INIT-SIPI-SIPI
with hypercalls, but now that hvmloader is enlightened it has no real need for
the APIC to be configured. If feels weird because you wouldn't use this order
on bare metal. But it's fine under virt.

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

I suspect it feels wrong because you can't boot CPUs ahead of configuring your
APIC in real hardware. But hvmloader is always virtualized so that point is
moot. If anything, I'd be scared of adding code ahead of smp_initialise() that
relies on CPU_TO_X2APICID being set when it hasn't yet.

If you have a strong view on the matter I can remove this hunk and call
read_apic_id() from apic_setup(). But it wouldn't be my preference to do so.

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

Oops, right.

>
> And of course this will need doing differently anyway once we get to
> support for more than 128 vCPU-s.

This is just a paranoia-driven assert to give quick feedback on the overflow of
the APIC ID later on. The entry in the MP-Table is a single octet long, so in
those cases we'd want to skip the table to begin with.

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

I'll add that in the comment. I do want it to be x2apic in name though, so as
to make it obvious that it's LUT of 32bit items.

As for the caps, bad reasons. It used to be a macro and over time I kept
interpreting it as an indexed constant. Should be lowercase.

>
> > @@ -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?

I'll give that a go. hvmloader has given no shortage of headaches with its
quirky environment, so we'll see...

>
> Jan

Cheers,
Alejandro