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

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

On Fri, May 24, 2024 at 04:15:34PM +0100, Alejandro Vallejo wrote:
> 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.

Right, I've read the C99 spec and it does indeed only guarantee the
state of volatile objects to be as expected at sequence points.  I
think however this specific code would still be fine since the
function calls are considered to have side-effects, and those must all
be completed before the volatile access.

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

Yes, my understating is that you added the ACCESS_ONCE() to possibly
prevent the compiler from re-ordering the CPU_TO_X2APICID[ap_cpuid]) =
read_apic_id() after the 'hlt' loop?

AFAICT the expressions in the `for` statement are considered sequence
points, and the calling `read_apic_id()` could have side-effects, so
the compiler won't be able to elide or move the setting of
CPU_TO_X2APICID[] due to all side-effects of previous evaluations must
be complete at sequence points.

I'm fine with leaving the wmb() + ACCESS_ONCE().

Thanks, Roger.