Re: [PATCH v2 5/8] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 08, 2024 at 01:39:24PM +0100, 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.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
> v2:
>   * New patch. Replaces adding cpu policy to hvmloader in v1.
> ---
>  tools/firmware/hvmloader/config.h    |  6 ++++-
>  tools/firmware/hvmloader/hvmloader.c |  4 +--
>  tools/firmware/hvmloader/smp.c       | 40 +++++++++++++++++++++++-----
>  tools/firmware/hvmloader/util.h      |  5 ++++
>  xen/arch/x86/include/asm/hvm/hvm.h   |  1 +
>  5 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h 
> b/tools/firmware/hvmloader/config.h
> index c82adf6dc508..edf6fa9c908c 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -4,6 +4,8 @@
>  #include <stdint.h>
>  #include <stdbool.h>
>  
> +#include <xen/hvm/hvm_info_table.h>
> +
>  enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
>  extern enum virtual_vga virtual_vga;
>  
> @@ -49,8 +51,10 @@ extern uint8_t ioapic_version;
>  
>  #define IOAPIC_ID           0x01
>  
> +extern uint32_t CPU_TO_X2APICID[HVM_MAX_VCPUS];
> +
>  #define LAPIC_BASE_ADDRESS  0xfee00000
> -#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
> +#define LAPIC_ID(vcpu_id)   (CPU_TO_X2APICID[(vcpu_id)])
>  
>  #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
>  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
> diff --git a/tools/firmware/hvmloader/hvmloader.c 
> b/tools/firmware/hvmloader/hvmloader.c
> index c58841e5b556..1eba92229925 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -342,11 +342,11 @@ int main(void)
>  
>      printf("CPU speed is %u MHz\n", get_cpu_mhz());
>  
> +    smp_initialise();
> +
>      apic_setup();
>      pci_setup();
>  
> -    smp_initialise();
> -
>      perform_tests();
>  
>      if ( bios->bios_info_setup )
> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
> index a668f15d7e1f..4d75f239c2f5 100644
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -29,7 +29,34 @@
>  
>  #include <xen/vcpu.h>
>  
> -static int ap_callin, ap_cpuid;
> +static int ap_cpuid;
> +
> +/**
> + * 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];
> +
> +static uint32_t read_apic_id(void)
> +{
> +    uint32_t apic_id;
> +
> +    cpuid(1, NULL, &apic_id, NULL, NULL);
> +    apic_id >>= 24;
> +
> +    /*
> +     * APIC IDs over 255 are represented by 255 in leaf 1 and are meant to be
> +     * read from topology leaves instead. Xen exposes x2APIC IDs in leaf 0xb,
> +     * but only if the x2APIC feature is present. If there are that many CPUs
> +     * it's guaranteed to be there so we can avoid checking for it 
> specifically
> +     */

Maybe I'm missing something, but given the current code won't Xen just
return the low 8 bits from the x2APIC ID?  I don't see any code in
guest_cpuid() that adjusts the IDs to be 255 when > 255.

> +    if ( apic_id == 255 )
> +        cpuid(0xb, NULL, NULL, NULL, &apic_id);

Won't the correct logic be to check if x2APIC is set in CPUID, and
then fetch the APIC ID from leaf 0xb, otherwise fallback to fetching
the APID ID from leaf 1?

> +
> +    return apic_id;
> +}
>  
>  static void ap_start(void)
>  {
> @@ -37,12 +64,12 @@ static void ap_start(void)
>      cacheattr_init();
>      printf("done.\n");
>  
> +    wmb();
> +    ACCESS_ONCE(CPU_TO_X2APICID[ap_cpuid]) = read_apic_id();

A comment would be helpful here, that CPU_TO_X2APICID[ap_cpuid] is
used as synchronization that the AP has started.

You probably want to assert that read_apic_id() doesn't return 0,
otherwise we are skewed.

> +
>      if ( !ap_cpuid )
>          return;
>  
> -    wmb();
> -    ap_callin = 1;
> -
>      while ( 1 )
>          asm volatile ( "hlt" );
>  }
> @@ -86,10 +113,11 @@ static void boot_cpu(unsigned int cpu)
>          BUG();
>  
>      /*
> -     * Wait for the secondary processor to complete initialisation.
> +     * Wait for the secondary processor to complete initialisation,
> +     * which is signaled by its x2APIC ID being writted to the LUT.
>       * Do not touch shared resources meanwhile.
>       */
> -    while ( !ap_callin )
> +    while ( !ACCESS_ONCE(CPU_TO_X2APICID[cpu]) )
>          cpu_relax();

As a further improvement, we could launch all APs in pararell, and use
a for loop to wait until all positions of the CPU_TO_X2APICID array
are set.

>  
>      /* Take the secondary processor offline. */
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 14078bde1e30..51e9003bc615 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -23,6 +23,11 @@ enum {
>  #define __STR(...) #__VA_ARGS__
>  #define STR(...) __STR(__VA_ARGS__)
>  
> +#define __ACCESS_ONCE(x) ({                             \
> +            (void)(typeof(x))0; /* Scalar typecheck. */ \
> +            (volatile typeof(x) *)&(x); })
> +#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))

Might be better for this to be placed in common-macros.h?

> +
>  /* GDT selector values. */
>  #define SEL_CODE16          0x0008
>  #define SEL_DATA16          0x0010
> diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
> b/xen/arch/x86/include/asm/hvm/hvm.h
> index 84911f3ebcb4..6c005f0b0b38 100644
> --- a/xen/arch/x86/include/asm/hvm/hvm.h
> +++ b/xen/arch/x86/include/asm/hvm/hvm.h
> @@ -16,6 +16,7 @@
>  #include <asm/current.h>
>  #include <asm/x86_emulate.h>
>  #include <asm/hvm/asid.h>
> +#include <asm/hvm/vlapic.h>

Is this a stray change?  Otherwise I don't see why this need to be
part of this patch when the rest of the changes are exclusively to
hvmloader.

Thanks, Roger.