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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 23/05/2024 17:13, Roger Pau Monné wrote:
> 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?

I was pretty sure that was the behaviour of real HW, but clearly I was
wrong. Just checked on a beefy machine and that's indeed the low 8 bits,
just as Xen emulates. Got confused with the core count, that does clip
to 255.

Will adjust by explicitly checking for the x2apic_id bit.

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

Not a bad idea. Sure

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

I thought about it, but then we'd need locking for the prints as well,
or refactor things so only the BSP prints on success.

The time taken is truly negligible, so I reckon it's better left for
another patch.

> 
>>  
>>      /* 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?

Sure.

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

Should've been part of the vlapic_policy_update change. That line got
lost in the v1->v2 conversion. I just moved to where it logically
belongs now.

Cheers,
Alejandro