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.vall...@cloud.com>
> >> ---
> >> 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.
