Currently the mptable setup code only considers one core per cpu package
for populating the cpu tables. The detection logic goes back to machines
where the cpuid would signify the presence of multiple logical cores
(HT) and presumably this change was made to prevent creating vcpus for
each hyperthread.

In practice this restriction is not required any longer especially since
current processors feature many physical cores per socket, and it is
preventing QEMU guests that don't enable ACPI from seeing more than one
cores, unless those cores are explicitly placed across multiple sockets
(one core per socket, specifically).

During v6.2 QEMU changed the default cpu topology arrangement preference
from arranging multiple cores spreaded across multiple sockets to
placing them within the same socket [1]. In practice this means that
without specifing explicitly the cpu topology and assuming QEMU
defaults, a guest without ACPI will not be able to instantiate more than
one core.

Fix this by lifting the restriction of only populating the mptable with
the first logical processor per package.

[1] commit 4a0af2930a4e ("machine: Prefer cores over sockets in smp
parsing since 6.2")

Fixes: c0ad0e8febe5 ("Only add the first logical CPU in each physical
CPU to the MPS tables.")

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 src/fw/mptable.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/fw/mptable.c b/src/fw/mptable.c
index 47385cc5d32d..a939743691c9 100644
--- a/src/fw/mptable.c
+++ b/src/fw/mptable.c
@@ -47,19 +47,13 @@ mptable_setup(void)
         cpuid_signature = 0x600;
         cpuid_features = 0x201;
     }
-    int pkgcpus = 1;
-    if (cpuid_features & (1 << 28)) {
-        /* Only populate the MPS tables with the first logical CPU in
-           each package */
-        pkgcpus = (ebx >> 16) & 0xff;
-        pkgcpus = 1 << (__fls(pkgcpus - 1) + 1); /* round up to power of 2 */
-    }
+
     u8 apic_version = readl((u8*)BUILD_APIC_ADDR + 0x30) & 0xff;
 
     // CPU definitions.
     struct mpt_cpu *cpus = (void*)&config[1], *cpu = cpus;
     int i;
-    for (i = 0; i < MaxCountCPUs; i+=pkgcpus) {
+    for (i = 0; i < MaxCountCPUs; i++) {
         memset(cpu, 0, sizeof(*cpu));
         cpu->type = MPT_TYPE_CPU;
         cpu->apicid = i;
-- 
2.47.0

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Wed, Dec 11, 2024 at 11:36:42AM +0100, Anthony Iliopoulos via SeaBIOS wrote:
> Currently the mptable setup code only considers one core per cpu package
> for populating the cpu tables. The detection logic goes back to machines
> where the cpuid would signify the presence of multiple logical cores
> (HT) and presumably this change was made to prevent creating vcpus for
> each hyperthread.
> 
> In practice this restriction is not required any longer especially since
> current processors feature many physical cores per socket, and it is
> preventing QEMU guests that don't enable ACPI from seeing more than one
> cores, unless those cores are explicitly placed across multiple sockets
> (one core per socket, specifically).
> 
> During v6.2 QEMU changed the default cpu topology arrangement preference
> from arranging multiple cores spreaded across multiple sockets to
> placing them within the same socket [1]. In practice this means that
> without specifing explicitly the cpu topology and assuming QEMU
> defaults, a guest without ACPI will not be able to instantiate more than
> one core.
> 
> Fix this by lifting the restriction of only populating the mptable with
> the first logical processor per package.
> 
> [1] commit 4a0af2930a4e ("machine: Prefer cores over sockets in smp
> parsing since 6.2")
> 
> Fixes: c0ad0e8febe5 ("Only add the first logical CPU in each physical
> CPU to the MPS tables.")
> 
> Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>

Alas, we found that we can't maintain these files - anytime we make
any change (for better or worse) it causes backwards compatibility
problems with clients.  If you look at the top of mptable.c you'll
find:

// DO NOT ADD NEW FEATURES HERE.  (See paravirt.c / biostables.c instead.)

So, if a change is needed to the mptable then code will be needed to
generate the desired table in qemu and pass it through to SeaBIOS.

Cheers,
-Kevin


> ---
>  src/fw/mptable.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/src/fw/mptable.c b/src/fw/mptable.c
> index 47385cc5d32d..a939743691c9 100644
> --- a/src/fw/mptable.c
> +++ b/src/fw/mptable.c
> @@ -47,19 +47,13 @@ mptable_setup(void)
>          cpuid_signature = 0x600;
>          cpuid_features = 0x201;
>      }
> -    int pkgcpus = 1;
> -    if (cpuid_features & (1 << 28)) {
> -        /* Only populate the MPS tables with the first logical CPU in
> -           each package */
> -        pkgcpus = (ebx >> 16) & 0xff;
> -        pkgcpus = 1 << (__fls(pkgcpus - 1) + 1); /* round up to power of 2 */
> -    }
> +
>      u8 apic_version = readl((u8*)BUILD_APIC_ADDR + 0x30) & 0xff;
>  
>      // CPU definitions.
>      struct mpt_cpu *cpus = (void*)&config[1], *cpu = cpus;
>      int i;
> -    for (i = 0; i < MaxCountCPUs; i+=pkgcpus) {
> +    for (i = 0; i < MaxCountCPUs; i++) {
>          memset(cpu, 0, sizeof(*cpu));
>          cpu->type = MPT_TYPE_CPU;
>          cpu->apicid = i;
> -- 
> 2.47.0
> 
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Tue, Dec 24, 2024 at 06:55:19PM -0500, Kevin O'Connor wrote:
> On Wed, Dec 11, 2024 at 11:36:42AM +0100, Anthony Iliopoulos via SeaBIOS wrote:
> > Currently the mptable setup code only considers one core per cpu package
> > for populating the cpu tables. The detection logic goes back to machines
> > where the cpuid would signify the presence of multiple logical cores
> > (HT) and presumably this change was made to prevent creating vcpus for
> > each hyperthread.
> > 
> > In practice this restriction is not required any longer especially since
> > current processors feature many physical cores per socket, and it is
> > preventing QEMU guests that don't enable ACPI from seeing more than one
> > cores, unless those cores are explicitly placed across multiple sockets
> > (one core per socket, specifically).
> > 
> > During v6.2 QEMU changed the default cpu topology arrangement preference
> > from arranging multiple cores spreaded across multiple sockets to
> > placing them within the same socket [1]. In practice this means that
> > without specifing explicitly the cpu topology and assuming QEMU
> > defaults, a guest without ACPI will not be able to instantiate more than
> > one core.
> > 
> > Fix this by lifting the restriction of only populating the mptable with
> > the first logical processor per package.
> > 
> > [1] commit 4a0af2930a4e ("machine: Prefer cores over sockets in smp
> > parsing since 6.2")
> > 
> > Fixes: c0ad0e8febe5 ("Only add the first logical CPU in each physical
> > CPU to the MPS tables.")
> > 
> > Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
> 
> Alas, we found that we can't maintain these files - anytime we make
> any change (for better or worse) it causes backwards compatibility
> problems with clients.  If you look at the top of mptable.c you'll
> find:

What are those other clients, out of curiosity? AFAICT other than qemu it is
only bochs that may be leveraging it (although not by default).

> // DO NOT ADD NEW FEATURES HERE.  (See paravirt.c / biostables.c instead.)
> 
> So, if a change is needed to the mptable then code will be needed to
> generate the desired table in qemu and pass it through to SeaBIOS.

I assume you mean via the fw_cfg interface entries, but in paravirt.c
there's the other comment:

// List of QEMU fw_cfg entries.  DO NOT ADD MORE.  (All new content
// should be passed via the fw_cfg "file" interface.)

Also since qemu is already passing all the necessary info (CFG_MAX_CPUS), there
shouldn't be any need to construct and pass any new entries, what about
something simpler along the lines of the following diff:

diff --git a/src/fw/mptable.c b/src/fw/mptable.c
index 47385cc5d32d..76d2213d4b30 100644
--- a/src/fw/mptable.c
+++ b/src/fw/mptable.c
@@ -12,6 +12,7 @@
 #include "hw/pci_regs.h" // PCI_INTERRUPT_PIN
 #include "malloc.h" // free
 #include "output.h" // dprintf
+#include "paravirt.h" // runningOnQEMU
 #include "romfile.h" // romfile_loadint
 #include "std/mptable.h" // MPTABLE_SIGNATURE
 #include "string.h" // memset
@@ -48,7 +49,7 @@ mptable_setup(void)
         cpuid_features = 0x201;
     }
     int pkgcpus = 1;
-    if (cpuid_features & (1 << 28)) {
+    if (cpuid_features & (1 << 28) && !runningOnQEMU()) {
         /* Only populate the MPS tables with the first logical CPU in
            each package */
         pkgcpus = (ebx >> 16) & 0xff;

Regards,
Anthony
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Thu, Dec 26, 2024 at 12:22:43PM +0100, Anthony Iliopoulos wrote:
> On Tue, Dec 24, 2024 at 06:55:19PM -0500, Kevin O'Connor wrote:
> > On Wed, Dec 11, 2024 at 11:36:42AM +0100, Anthony Iliopoulos via SeaBIOS wrote:
> > > Currently the mptable setup code only considers one core per cpu package
> > > for populating the cpu tables. The detection logic goes back to machines
> > > where the cpuid would signify the presence of multiple logical cores
> > > (HT) and presumably this change was made to prevent creating vcpus for
> > > each hyperthread.
> > > 
> > > In practice this restriction is not required any longer especially since
> > > current processors feature many physical cores per socket, and it is
> > > preventing QEMU guests that don't enable ACPI from seeing more than one
> > > cores, unless those cores are explicitly placed across multiple sockets
> > > (one core per socket, specifically).
> > > 
> > > During v6.2 QEMU changed the default cpu topology arrangement preference
> > > from arranging multiple cores spreaded across multiple sockets to
> > > placing them within the same socket [1]. In practice this means that
> > > without specifing explicitly the cpu topology and assuming QEMU
> > > defaults, a guest without ACPI will not be able to instantiate more than
> > > one core.
> > > 
> > > Fix this by lifting the restriction of only populating the mptable with
> > > the first logical processor per package.
> > > 
> > > [1] commit 4a0af2930a4e ("machine: Prefer cores over sockets in smp
> > > parsing since 6.2")
> > > 
> > > Fixes: c0ad0e8febe5 ("Only add the first logical CPU in each physical
> > > CPU to the MPS tables.")
> > > 
> > > Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
> > 
> > Alas, we found that we can't maintain these files - anytime we make
> > any change (for better or worse) it causes backwards compatibility
> > problems with clients.  If you look at the top of mptable.c you'll
> > find:
> 
> What are those other clients, out of curiosity? AFAICT other than qemu it is
> only bochs that may be leveraging it (although not by default).

By "clients" I meant "guests" - for example various versions of linux,
windows, etc. that are running in the guest VM alongside SeaBIOS.

> > // DO NOT ADD NEW FEATURES HERE.  (See paravirt.c / biostables.c instead.)
> > 
> > So, if a change is needed to the mptable then code will be needed to
> > generate the desired table in qemu and pass it through to SeaBIOS.
> 
> I assume you mean via the fw_cfg interface entries, but in paravirt.c
> there's the other comment:

I was referring to how qemu_platform_setup() calls
romfile_loader_execute() to populate acpi (and possibly other) tables
generated by qemu.  Or, similarly, how smbios_build_tables() copies
the smbios table generated by qemu.

> 
> // List of QEMU fw_cfg entries.  DO NOT ADD MORE.  (All new content
> // should be passed via the fw_cfg "file" interface.)
> 
> Also since qemu is already passing all the necessary info (CFG_MAX_CPUS), there
> shouldn't be any need to construct and pass any new entries, what about
> something simpler along the lines of the following diff:

As before, we've found that any change to these guest visible tables
causes unexpected compatibility and rollout issues.

Cheers,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org