[SeaBIOS] Re: [PATCH] fw/mptable: fix smp core initialization per cpu package

[Anthony Iliopoulos]

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