[PATCH v2 0/2] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[PATCH v2 0/2] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Alejandro Vallejo]

v1->v2:
  * Dropped patch to skip writing the MP Tables if apicid >= 255

v1: https://lore.kernel.org/xen-devel/20250128163342.1491-1-alejandro.vallejo@cloud.com/
source series: https://lore.kernel.org/xen-devel/20241021154600.11745-5-alejandro.vallejo@cloud.com/

The hypervisor, hvmloader and the toolstack currently engage in a shared
assumption that for every vCPU apicid == 2 * vcpuid. This series removes such
assumption from hvmloader, by making it read the APIC ID of each vCPU and
storing it for later use.

Alejandro Vallejo (2):
  tools/hvmloader: Retrieve APIC IDs from the APs themselves
  tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

 tools/firmware/hvmloader/config.h    |  3 +-
 tools/firmware/hvmloader/hvmloader.c |  6 ++--
 tools/firmware/hvmloader/mp_tables.c |  2 +-
 tools/firmware/hvmloader/smp.c       | 43 +++++++++++++++++++++++++++-
 tools/firmware/hvmloader/util.c      |  2 +-
 5 files changed, 49 insertions(+), 7 deletions(-)

-- 
2.48.1

[PATCH v2 1/2] tools/hvmloader: Retrieve APIC IDs from the APs themselves

[Alejandro Vallejo]

Make it so the APs expose their own APIC IDs in a lookup table (LUT). We
can use that LUT to populate the MADT, decoupling the algorithm that
relates CPU IDs and APIC IDs from hvmloader.

Modified the printf to also print the APIC ID of each CPU, as well as
fixing a (benign) wrong specifier being used for the vcpu id.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v1->v2:
  * Removed "(x2)" from the comment of cpu_to_apicid.
  * Added "APIC ID" to the printed string on AP boot up.

Changes from the v7 version of this patch in the longer topology series:
  * s/cpu_to_x2apicid/cpu_to_apicid/
    * Though, as I already stated, I don't think this is a good idea.
  * Dynamically size cpu_to_apicid rather than using HVM_MAX_VCPUS.
  * Got rid of the ap_callin removal. It's not as trivial if we don't
    want to assume cpu0 always has apicid=0. Part of the complaints on
    the previous versions involved the inability to do that.
  * For debugging sanity, I've added the apicid to the CPU boot printf.
      * Later on, toolstack will choose the APIC IDs and it's helpful
        to know the relationship in the logs.
      * While at it, fix the vcpu specifier s/%d/%u/
  * Check for leaf 0xb while probing for x2apic support.
---
 tools/firmware/hvmloader/smp.c | 43 +++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
index 1b940cefd071..341fd6e0b61a 100644
--- a/tools/firmware/hvmloader/smp.c
+++ b/tools/firmware/hvmloader/smp.c
@@ -31,9 +31,38 @@
 
 static int ap_callin;
 
+/** True if x2apic support is exposed to the guest. */
+static bool has_x2apic;
+
+/**
+ * Lookup table of APIC IDs.
+ *
+ * Each entry is populated for its respective CPU as they come online. This is
+ * required for generating the MADT with minimal assumptions about ID
+ * relationships.
+ */
+uint32_t *cpu_to_apicid;
+
+static uint32_t read_apic_id(void)
+{
+    uint32_t apic_id;
+
+    if ( has_x2apic )
+        cpuid(0xb, NULL, NULL, NULL, &apic_id);
+    else
+    {
+        cpuid(1, NULL, &apic_id, NULL, NULL);
+        apic_id >>= 24;
+    }
+
+    return apic_id;
+}
+
 static void cpu_setup(unsigned int cpu)
 {
-    printf(" - CPU%d ... ", cpu);
+    uint32_t apicid = cpu_to_apicid[cpu] = read_apic_id();
+
+    printf(" - CPU%u APIC ID %u ... ", cpu, apicid);
     cacheattr_init();
     printf("done.\n");
 
@@ -104,8 +133,20 @@ static void boot_cpu(unsigned int cpu)
 void smp_initialise(void)
 {
     unsigned int i, nr_cpus = hvm_info->nr_vcpus;
+    uint32_t ecx, max_leaf;
+
+    cpuid(0, &max_leaf, NULL, NULL, NULL);
+    if ( max_leaf >= 0xb )
+    {
+        cpuid(1, NULL, NULL, &ecx, NULL);
+        has_x2apic = (ecx >> 21) & 1;
+        if ( has_x2apic )
+            printf("x2APIC supported\n");
+    }
 
     printf("Multiprocessor initialisation:\n");
+    cpu_to_apicid = scratch_alloc(sizeof(*cpu_to_apicid) * nr_cpus,
+                                  sizeof(*cpu_to_apicid));
     cpu_setup(0);
     for ( i = 1; i < nr_cpus; i++ )
         boot_cpu(i);
-- 
2.48.1

[PATCH v2 2/2] tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

[Alejandro Vallejo]

Replace uses of the LAPIC_ID() macro with accesses to the
cpu_to_apicid[] lookup table. This table contains the APIC IDs of each
vCPU as probed at runtime rather than assuming a predefined relation.

Moved smp_initialise() ahead of apic_setup() in order to initialise
cpu_to_apicid ASAP and avoid using it uninitialised. Note that bringing
up the APs doesn't need the APIC in hvmloader becasue it always runs
virtualized and uses the PV interface.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v1->v2:
  * No changes
Changes wrt original series
  * No changes (it was wrongly stated in v1 that something did. That was part
    of the following patch)

---
 tools/firmware/hvmloader/config.h    | 3 ++-
 tools/firmware/hvmloader/hvmloader.c | 6 +++---
 tools/firmware/hvmloader/mp_tables.c | 2 +-
 tools/firmware/hvmloader/util.c      | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index cd716bf39245..6e1da137d779 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -48,8 +48,9 @@ extern uint8_t ioapic_version;
 
 #define IOAPIC_ID           0x01
 
+extern uint32_t *cpu_to_apicid;
+
 #define LAPIC_BASE_ADDRESS  0xfee00000
-#define LAPIC_ID(vcpu_id)   ((vcpu_id) * 2)
 
 #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 f8af88fabf24..4e330fc1e241 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -224,7 +224,7 @@ static void apic_setup(void)
 
     /* 8259A ExtInts are delivered through IOAPIC pin 0 (Virtual Wire Mode). */
     ioapic_write(0x10, APIC_DM_EXTINT);
-    ioapic_write(0x11, SET_APIC_ID(LAPIC_ID(0)));
+    ioapic_write(0x11, SET_APIC_ID(cpu_to_apicid[0]));
 }
 
 struct bios_info {
@@ -341,11 +341,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/mp_tables.c b/tools/firmware/hvmloader/mp_tables.c
index 77d3010406d0..3c93a5c947d9 100644
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -199,7 +199,7 @@ static void fill_mp_config_table(struct mp_config_table *mpct, int length)
 static void fill_mp_proc_entry(struct mp_proc_entry *mppe, int vcpu_id)
 {
     mppe->type = ENTRY_TYPE_PROCESSOR;
-    mppe->lapic_id = LAPIC_ID(vcpu_id);
+    mppe->lapic_id = cpu_to_apicid[vcpu_id];
     mppe->lapic_version = 0x11;
     mppe->cpu_flags = CPU_FLAG_ENABLED;
     if ( vcpu_id == 0 )
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index d3b3f9038e64..2d07ce129013 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -827,7 +827,7 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
 
 static uint32_t acpi_lapic_id(unsigned cpu)
 {
-    return LAPIC_ID(cpu);
+    return cpu_to_apicid[cpu];
 }
 
 void hvmloader_acpi_build_tables(struct acpi_config *config,
-- 
2.48.1

Re: [PATCH v2 2/2] tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

[Jan Beulich]

On 04.02.2025 15:45, Alejandro Vallejo wrote:
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -48,8 +48,9 @@ extern uint8_t ioapic_version;
>  
>  #define IOAPIC_ID           0x01
>  
> +extern uint32_t *cpu_to_apicid;

Strictly speaking this ought to be part of the earlier patch. If hvmloader
was Misra-checked, this would be a (transient) violation.

config.h is also somewhat odd a place to put this declaration, yet then I
can't really suggest anything better.

Jan

Re: [PATCH v2 2/2] tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

[Alejandro Vallejo]

On Tue Feb 4, 2025 at 3:07 PM GMT, Jan Beulich wrote:
> On 04.02.2025 15:45, Alejandro Vallejo wrote:
> > --- a/tools/firmware/hvmloader/config.h
> > +++ b/tools/firmware/hvmloader/config.h
> > @@ -48,8 +48,9 @@ extern uint8_t ioapic_version;
> >  
> >  #define IOAPIC_ID           0x01
> >  
> > +extern uint32_t *cpu_to_apicid;
>
> Strictly speaking this ought to be part of the earlier patch. If hvmloader
> was Misra-checked, this would be a (transient) violation.

Hmmm. I don't see it. The previous patch is fully contained in smp.c and this
extern isn't required until now. Does MISRA have mandates on non-static symbols
not present in headers?

The global could be static in patch1, but seems silly seeing how it'd be undone
here.

>
> config.h is also somewhat odd a place to put this declaration, yet then I
> can't really suggest anything better.
>
> Jan

Any header will do but there's no better one I could find, and I'd rather not
create a new one just for this.

Cheers,
Alejandro

Re: [PATCH v2 2/2] tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

[Jan Beulich]

On 04.02.2025 16:25, Alejandro Vallejo wrote:
> On Tue Feb 4, 2025 at 3:07 PM GMT, Jan Beulich wrote:
>> On 04.02.2025 15:45, Alejandro Vallejo wrote:
>>> --- a/tools/firmware/hvmloader/config.h
>>> +++ b/tools/firmware/hvmloader/config.h
>>> @@ -48,8 +48,9 @@ extern uint8_t ioapic_version;
>>>  
>>>  #define IOAPIC_ID           0x01
>>>  
>>> +extern uint32_t *cpu_to_apicid;
>>
>> Strictly speaking this ought to be part of the earlier patch. If hvmloader
>> was Misra-checked, this would be a (transient) violation.
> 
> Hmmm. I don't see it. The previous patch is fully contained in smp.c and this
> extern isn't required until now. Does MISRA have mandates on non-static symbols
> not present in headers?

Every non-static definition is expected to have exactly one earlier
declaration.

Jan

Re: [PATCH v2 2/2] tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]

[Alejandro Vallejo]

On Tue Feb 4, 2025 at 3:46 PM GMT, Jan Beulich wrote:
> On 04.02.2025 16:25, Alejandro Vallejo wrote:
> > On Tue Feb 4, 2025 at 3:07 PM GMT, Jan Beulich wrote:
> >> On 04.02.2025 15:45, Alejandro Vallejo wrote:
> >>> --- a/tools/firmware/hvmloader/config.h
> >>> +++ b/tools/firmware/hvmloader/config.h
> >>> @@ -48,8 +48,9 @@ extern uint8_t ioapic_version;
> >>>  
> >>>  #define IOAPIC_ID           0x01
> >>>  
> >>> +extern uint32_t *cpu_to_apicid;
> >>
> >> Strictly speaking this ought to be part of the earlier patch. If hvmloader
> >> was Misra-checked, this would be a (transient) violation.
> > 
> > Hmmm. I don't see it. The previous patch is fully contained in smp.c and this
> > extern isn't required until now. Does MISRA have mandates on non-static symbols
> > not present in headers?
>
> Every non-static definition is expected to have exactly one earlier
> declaration.
>
> Jan

I had no idea. Fair enough then, I'll adjust...

Cheers,
Alejandro