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

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

[Alejandro Vallejo]

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.

The last patch prevents writing an MP Tables should we have vCPUs that can not
be represented there. That's at the moment dead code because all vCPUs are
currently representable in 8 bits. This will inavitably stop being true in the
future after we increase the maximum number of guest vCPUs.

This short series is extracted from v7 of the much longer "Expose consistent
topology to guests".

  https://lore.kernel.org/xen-devel/20241021154600.11745-5-alejandro.vallejo@cloud.com/

Changes with respect to the original patch on each individual patch.

Alejandro Vallejo (3):
  tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves
  tools/hvmloader: Replace LAPIC_ID() with cpu_to_apicid[]
  tools/hvmloader: Skip writing MP tables if any CPU has an APIC ID >=
    255

 tools/firmware/hvmloader/config.h    |  4 ++-
 tools/firmware/hvmloader/hvmloader.c | 12 ++++---
 tools/firmware/hvmloader/mp_tables.c |  2 +-
 tools/firmware/hvmloader/smp.c       | 47 +++++++++++++++++++++++++++-
 tools/firmware/hvmloader/util.c      |  2 +-
 5 files changed, 59 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH 1/3] tools/hvmloader: Retrieve (x2)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>
---
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..c61ed524f947 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 (x2)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[%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 2/3] 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>
---
Changes from v7 of the longer topology series:
  * Removed ASSERT() for the MP tables and merely skipped writing them
    if any vCPU has an APIC ID >=255.
---
 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

[PATCH 3/3] tools/hvmloader: Skip writing MP tables if any CPU has an APIC ID >= 255

[Alejandro Vallejo]

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

Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Roger Pau Monné]

On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
> 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.
> 
> The last patch prevents writing an MP Tables should we have vCPUs that can not
> be represented there. That's at the moment dead code because all vCPUs are
> currently representable in 8 bits. This will inavitably stop being true in the
> future after we increase the maximum number of guest vCPUs.

While I'm fine with the MP Table change, should it also come together
with a patch that introduces the code to create x2APIC entries in
libacpi construct_madt() helper? (and bumping the MADT revision, as
I'm quite sure version 2 didn't have x2APIC entries in the
specification).

Otherwise the MP Table change seems like a red herring, because the
MADT created by libacpi will also be incorrect and APIC IDs will wrap in
local APIC entries, just like it would on MP Tables.

Thanks, Roger.

Re: [PATCH 1/3] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Roger Pau Monné]

On Tue, Jan 28, 2025 at 04:33:40PM +0000, Alejandro Vallejo wrote:
> 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>
> ---
> 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..c61ed524f947 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 (x2)APIC IDs.

Not sure you need to explicitly mention (x2) in the comment?  It will
either be xAPIC or x2APIC IDs, but just using "APIC IDs" should cover
both unambiguously?

> + *
> + * 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[%u] ... ", cpu, apicid);

I would explicitly name the field in the print:

" - CPU%u APIC ID %u ... "

As otherwise it's not obvious what the value in the braces is (and you
have to go look at the code).

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

Noticed you could narrow the scope of ecx, but at the price of
introducing the definition line inside of the if condition.  I don't
have a strong opinion, and I assume you prefer it this way, which is
fine IMO.

Thanks, Roger.

Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Alejandro Vallejo]

On Tue Jan 28, 2025 at 5:45 PM GMT, Roger Pau Monné wrote:
> On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
> > 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.
> > 
> > The last patch prevents writing an MP Tables should we have vCPUs that can not
> > be represented there. That's at the moment dead code because all vCPUs are
> > currently representable in 8 bits. This will inavitably stop being true in the
> > future after we increase the maximum number of guest vCPUs.
>
> While I'm fine with the MP Table change, should it also come together
> with a patch that introduces the code to create x2APIC entries in
> libacpi construct_madt() helper? (and bumping the MADT revision, as
> I'm quite sure version 2 didn't have x2APIC entries in the
> specification).

That's a lot more involved though. Matt started something in that direction
last year, but testing it was (and still is) effectively impossible until
HVM_MAX_VCPUS increases.

  https://lore.kernel.org/xen-devel/cd1a3ce14790af8c1bb4372ef0be5a6cbbb50b1c.1710338145.git.matthew.barnes@cloud.com/

The rest of the topo series can be used to test that (with a hack to
artificially bump the width of thread_id space), I'd rather not test a patch
with a long and still uncommitted series.

>
> Otherwise the MP Table change seems like a red herring, because the
> MADT created by libacpi will also be incorrect and APIC IDs will wrap in
> local APIC entries, just like it would on MP Tables.
>
> Thanks, Roger.

My take is that this is strictly better than what we have today by virtue of
going down from 2 latent bugs to just 1. That said, I don't strictly need it
for the topo series to advance, so it is (in a sense) optional.

A second approach is to gate things differently by preventing legacy BIOS
domains from having APs with APIC IDs >= 255 at all. OVMF already has MP and
$PIR tables disabled, from what I can see in hvmloader.

Cheers,
Alejandro

Re: [PATCH 1/3] tools/hvmloader: Retrieve (x2)APIC IDs from the APs themselves

[Alejandro Vallejo]

On Tue Jan 28, 2025 at 5:59 PM GMT, Roger Pau Monné wrote:
> On Tue, Jan 28, 2025 at 04:33:40PM +0000, Alejandro Vallejo wrote:
> > 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>
> > ---
> > 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..c61ed524f947 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 (x2)APIC IDs.
>
> Not sure you need to explicitly mention (x2) in the comment?  It will
> either be xAPIC or x2APIC IDs, but just using "APIC IDs" should cover
> both unambiguously?

Sure.

>
> > + *
> > + * 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[%u] ... ", cpu, apicid);
>
> I would explicitly name the field in the print:
>
> " - CPU%u APIC ID %u ... "
>
> As otherwise it's not obvious what the value in the braces is (and you
> have to go look at the code).

Sure.

>
> >      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;
>
> Noticed you could narrow the scope of ecx, but at the price of
> introducing the definition line inside of the if condition.  I don't
> have a strong opinion, and I assume you prefer it this way, which is
> fine IMO.
>
> Thanks, Roger.

I marginally prefer it this way, but I don't particularly care. I wanted to
avoid one more line in a hunk that's already quite high for a single CPUID
check. 

Cheers,
Alejandro

Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Roger Pau Monné]

On Tue, Jan 28, 2025 at 06:42:38PM +0000, Alejandro Vallejo wrote:
> On Tue Jan 28, 2025 at 5:45 PM GMT, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
> > > 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.
> > > 
> > > The last patch prevents writing an MP Tables should we have vCPUs that can not
> > > be represented there. That's at the moment dead code because all vCPUs are
> > > currently representable in 8 bits. This will inavitably stop being true in the
> > > future after we increase the maximum number of guest vCPUs.
> >
> > While I'm fine with the MP Table change, should it also come together
> > with a patch that introduces the code to create x2APIC entries in
> > libacpi construct_madt() helper? (and bumping the MADT revision, as
> > I'm quite sure version 2 didn't have x2APIC entries in the
> > specification).
> 
> That's a lot more involved though. Matt started something in that direction
> last year, but testing it was (and still is) effectively impossible until
> HVM_MAX_VCPUS increases.
> 
>   https://lore.kernel.org/xen-devel/cd1a3ce14790af8c1bb4372ef0be5a6cbbb50b1c.1710338145.git.matthew.barnes@cloud.com/
> 
> The rest of the topo series can be used to test that (with a hack to
> artificially bump the width of thread_id space), I'd rather not test a patch
> with a long and still uncommitted series.
> 
> >
> > Otherwise the MP Table change seems like a red herring, because the
> > MADT created by libacpi will also be incorrect and APIC IDs will wrap in
> > local APIC entries, just like it would on MP Tables.
> >
> > Thanks, Roger.
> 
> My take is that this is strictly better than what we have today by virtue of
> going down from 2 latent bugs to just 1. That said, I don't strictly need it
> for the topo series to advance, so it is (in a sense) optional.

I'm fine with the patch, but it probably wants to mention in the
commit message that MADT tables will still wrap when using APIC IDs >
255, as otherwise it seems MADT is not taken into consideration.

Thanks, Roger.

Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Jan Beulich]

On 29.01.2025 17:25, Roger Pau Monné wrote:
> On Tue, Jan 28, 2025 at 06:42:38PM +0000, Alejandro Vallejo wrote:
>> On Tue Jan 28, 2025 at 5:45 PM GMT, Roger Pau Monné wrote:
>>> On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
>>>> 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.
>>>>
>>>> The last patch prevents writing an MP Tables should we have vCPUs that can not
>>>> be represented there. That's at the moment dead code because all vCPUs are
>>>> currently representable in 8 bits. This will inavitably stop being true in the
>>>> future after we increase the maximum number of guest vCPUs.
>>>
>>> While I'm fine with the MP Table change, should it also come together
>>> with a patch that introduces the code to create x2APIC entries in
>>> libacpi construct_madt() helper? (and bumping the MADT revision, as
>>> I'm quite sure version 2 didn't have x2APIC entries in the
>>> specification).
>>
>> That's a lot more involved though. Matt started something in that direction
>> last year, but testing it was (and still is) effectively impossible until
>> HVM_MAX_VCPUS increases.
>>
>>   https://lore.kernel.org/xen-devel/cd1a3ce14790af8c1bb4372ef0be5a6cbbb50b1c.1710338145.git.matthew.barnes@cloud.com/
>>
>> The rest of the topo series can be used to test that (with a hack to
>> artificially bump the width of thread_id space), I'd rather not test a patch
>> with a long and still uncommitted series.
>>
>>>
>>> Otherwise the MP Table change seems like a red herring, because the
>>> MADT created by libacpi will also be incorrect and APIC IDs will wrap in
>>> local APIC entries, just like it would on MP Tables.
>>>
>>> Thanks, Roger.
>>
>> My take is that this is strictly better than what we have today by virtue of
>> going down from 2 latent bugs to just 1. That said, I don't strictly need it
>> for the topo series to advance, so it is (in a sense) optional.
> 
> I'm fine with the patch, but it probably wants to mention in the
> commit message that MADT tables will still wrap when using APIC IDs >
> 255, as otherwise it seems MADT is not taken into consideration.

I think we simply should not add MADT entries with wrapped (truncated)
APIC IDs. Which can be done when they truly are at risk of wrapping, or
right here.

Jan

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

[Jan Beulich]

On 28.01.2025 17:33, Alejandro Vallejo wrote:
> 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>
> ---
> Changes from v7 of the longer topology series:
>   * Removed ASSERT() for the MP tables and merely skipped writing them
>     if any vCPU has an APIC ID >=255.

Throughout the patch I can't anything matching this; ...

> --- 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 */
> --- 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 )
> --- 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 )
> --- 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,

... no conditional is being added anywhere. What am I missing?

Thing is - this way I'm uncertain whether the wording above is
imprecise, or whether I should really ask that we continue to write all
entries with at most 8-bit-wide APIC IDs, omitting just those with
wider ones.

Jan

Re: [PATCH 3/3] tools/hvmloader: Skip writing MP tables if any CPU has an APIC ID >= 255

[Jan Beulich]

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

Re: [PATCH 0/3] tools/hvmloader: Decouple APIC IDs from vCPU IDs

[Alejandro Vallejo]

On Thu Jan 30, 2025 at 9:17 AM GMT, Jan Beulich wrote:
> On 29.01.2025 17:25, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2025 at 06:42:38PM +0000, Alejandro Vallejo wrote:
> >> On Tue Jan 28, 2025 at 5:45 PM GMT, Roger Pau Monné wrote:
> >>> On Tue, Jan 28, 2025 at 04:33:39PM +0000, Alejandro Vallejo wrote:
> >>>> 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.
> >>>>
> >>>> The last patch prevents writing an MP Tables should we have vCPUs that can not
> >>>> be represented there. That's at the moment dead code because all vCPUs are
> >>>> currently representable in 8 bits. This will inavitably stop being true in the
> >>>> future after we increase the maximum number of guest vCPUs.
> >>>
> >>> While I'm fine with the MP Table change, should it also come together
> >>> with a patch that introduces the code to create x2APIC entries in
> >>> libacpi construct_madt() helper? (and bumping the MADT revision, as
> >>> I'm quite sure version 2 didn't have x2APIC entries in the
> >>> specification).
> >>
> >> That's a lot more involved though. Matt started something in that direction
> >> last year, but testing it was (and still is) effectively impossible until
> >> HVM_MAX_VCPUS increases.
> >>
> >>   https://lore.kernel.org/xen-devel/cd1a3ce14790af8c1bb4372ef0be5a6cbbb50b1c.1710338145.git.matthew.barnes@cloud.com/
> >>
> >> The rest of the topo series can be used to test that (with a hack to
> >> artificially bump the width of thread_id space), I'd rather not test a patch
> >> with a long and still uncommitted series.
> >>
> >>>
> >>> Otherwise the MP Table change seems like a red herring, because the
> >>> MADT created by libacpi will also be incorrect and APIC IDs will wrap in
> >>> local APIC entries, just like it would on MP Tables.
> >>>
> >>> Thanks, Roger.
> >>
> >> My take is that this is strictly better than what we have today by virtue of
> >> going down from 2 latent bugs to just 1. That said, I don't strictly need it
> >> for the topo series to advance, so it is (in a sense) optional.
> > 
> > I'm fine with the patch, but it probably wants to mention in the
> > commit message that MADT tables will still wrap when using APIC IDs >
> > 255, as otherwise it seems MADT is not taken into consideration.
>
> I think we simply should not add MADT entries with wrapped (truncated)
> APIC IDs. Which can be done when they truly are at risk of wrapping, or
> right here.
>
> Jan

I'm unsure that's the best approach, but I'll just drop the patch on the next
version. It's all gated on getting extended APIC IDs on the IOAPIC and MSIs
working anyway.

Cheers,
Alejandro