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