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