MP Tables have no means of representing APIC IDs wider than 255. At the
moment the effect is wrapping around which would give a corrupted table
with duplicate APIC IDs (some of them possibly the broadcast 255).

Skip writing it altogether. While it would be possible to restrict the
APs shown it's just not worth the work. Any OS that needs such
adjustments should not have been booted with that many vCPUs to begin
with.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
Changes with respect to v7 in the longer topology series:
  * This patch replaces the previous assert in hvmloader/mp_tables.c
---
 tools/firmware/hvmloader/config.h    | 1 +
 tools/firmware/hvmloader/hvmloader.c | 6 +++++-
 tools/firmware/hvmloader/smp.c       | 4 ++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 6e1da137d779..53be34e48a02 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -49,6 +49,7 @@ extern uint8_t ioapic_version;
 #define IOAPIC_ID           0x01
 
 extern uint32_t *cpu_to_apicid;
+extern uint32_t max_apicid;
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
 
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 4e330fc1e241..54299e27364d 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -389,7 +389,11 @@ int main(void)
 
     if ( (hvm_info->nr_vcpus > 1) || hvm_info->apic_mode )
     {
-        if ( bios->create_mp_tables )
+        /*
+         * Legacy MP tables hold strictly xAPIC IDs. Skip writing
+         * the tables altogether if we have IDs wider than 8bits.
+         */
+        if ( max_apicid < 0xFF && bios->create_mp_tables )
             bios->create_mp_tables();
         if ( bios->create_pir_tables )
             bios->create_pir_tables();
diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
index c61ed524f947..0a01cdc18caa 100644
--- a/tools/firmware/hvmloader/smp.c
+++ b/tools/firmware/hvmloader/smp.c
@@ -34,6 +34,9 @@ static int ap_callin;
 /** True if x2apic support is exposed to the guest. */
 static bool has_x2apic;
 
+/** Highest entry in `cpu_to_apicid`. */
+uint32_t max_apicid;
+
 /**
  * Lookup table of (x2)APIC IDs.
  *
@@ -61,6 +64,7 @@ static uint32_t read_apic_id(void)
 static void cpu_setup(unsigned int cpu)
 {
     uint32_t apicid = cpu_to_apicid[cpu] = read_apic_id();
+    max_apicid = max(max_apicid, apicid);
 
     printf(" - CPU%u[%u] ... ", cpu, apicid);
     cacheattr_init();
-- 
2.48.1

On 28.01.2025 17:33, Alejandro Vallejo wrote:
> MP Tables have no means of representing APIC IDs wider than 255. At the
> moment the effect is wrapping around which would give a corrupted table
> with duplicate APIC IDs (some of them possibly the broadcast 255).
> 
> Skip writing it altogether. While it would be possible to restrict the
> APs shown it's just not worth the work. Any OS that needs such
> adjustments should not have been booted with that many vCPUs to begin
> with.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Oh, here we go. Yet then indeed ...

> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -49,6 +49,7 @@ extern uint8_t ioapic_version;
>  #define IOAPIC_ID           0x01
>  
>  extern uint32_t *cpu_to_apicid;
> +extern uint32_t max_apicid;

.. I don't think we need this, as ...

> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -389,7 +389,11 @@ int main(void)
>  
>      if ( (hvm_info->nr_vcpus > 1) || hvm_info->apic_mode )
>      {
> -        if ( bios->create_mp_tables )
> +        /*
> +         * Legacy MP tables hold strictly xAPIC IDs. Skip writing
> +         * the tables altogether if we have IDs wider than 8bits.
> +         */
> +        if ( max_apicid < 0xFF && bios->create_mp_tables )
>              bios->create_mp_tables();

I think we ought to continue to write MP tables in this case, merely
omitting processor entries with APIC IDs that don't fit.

Jan