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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 24/05/2024 08:21, 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
>> +     */
>> +    if ( apic_id == 255 )
>> +        cpuid(0xb, NULL, NULL, NULL, &apic_id);
>> +
>> +    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();
> 
> Further thinking about this: do we really need the wmb(), given the
> usage of ACCESS_ONCE()?
> 
> wmb() is a compiler barrier, and the usage of volatile in
> ACCESS_ONCE() should already prevent any compiler re-ordering.
> 
> Thanks, Roger.

volatile reads/writes cannot be reordered with other volatile
reads/writes, but volatile reads/writes can be reordered with
non-volatile reads/writes, AFAIR.

My intent here was to prevent the compiler omitting the write (though in
practice it didn't). I think the barrier is still required to prevent
reordering according to the spec.

Cheers,
Alejandro