Re: MP tables do not report multiple CPUs in Qemu 6.2.0 on x86 when given -smp cpus=n flag

[Ani Sinha <ani@anisinha.ca>]

[-- Attachment #1: Type: text/plain, Size: 4550 bytes --]

+qemu-devel

On Thu, 20 Jan 2022, Ani Sinha wrote:

>
>
> On Wed, 19 Jan 2022, Peter Maydell wrote:
>
> > On Wed, 19 Jan 2022 at 14:44, Godmar Back <gback@cs.vt.edu> wrote:
> > > after upgrading to 6.2.0, I observed that code such as MIT's xv6 (see
> > > [1]) is no longer able to detect multiple CPUs.  Their code works in
> > > 6.1.1, however.
> >
> > Hi; this isn't a great place for reporting QEMU bugs, because
> > it's more of a user-to-user discussion list. Not all QEMU
> > developers read it. I've cc'd the ACPI maintainers, who
> > hopefully may have an idea about what's happening here.
> > You could also file a bug at
> > https://gitlab.com/qemu-project/qemu/-/issues
> >
> > > I built 6.1.1 from source and 6.2.0 from source and I have also tested
> > > with CentOS stream's 6.1.1 qemu-kvm and was able to pinpoint this
> > > change to these 2 versions of qemu. I am using qemu-system-i386
> > > specifically.
> > >
> > > I tried to go through the ChangeLog to see if the `-smp` option was
> > > deprecated or changed.  I found this note [2] about invalid topologies
> > > having been removed in 5.2. Here's what I found after long
> > > experimentation:
> > >
> > > The legacy MP tables appear to work only if you specify the longform
> > > `-smp cpus=4,cores=1,threads=1,sockets=4` in 6.2.0.  If you specify
> > > `-smp 4` or `-smp cpus=4` it will not work in 6.2.0 (it worked in
> > > 6.1.1). I am guessing that perhaps the MP tables add entries for each
> > > socket, but that perhaps the behavior of the shortcuts `-smp n` and
> > > `-smp cpus=n` was changed to influence the number of cores rather than
> > > sockets.
> > >
> > > In other words, `-smp cpus=n` now means `-smp
> > > cpus=n,cores=n,threads=1,sockets=1` whereas in 6.1.1 and before it
> > > meant `-smp cpus=n,cores=1,threads=1,sockets=n`. I note that
> > > specifying `-smp cpus=4,cores=4,threads=1,sockets=1` in 6.1.1 also
> > > does not create 4 entries in the legacy MP tables.
> > >
>
> My suspicion is that the following commit might have introduced a
> regression:
>
> commit 86ce2d28fa09d15547b5cabdc264cadfb53a848c
> Author: Yanan Wang <wangyanan55@huawei.com>
> Date:   Tue Oct 26 11:46:58 2021 +0800
>
>     hw/core/machine: Split out the smp parsing code
>
>     We are going to introduce an unit test for the parser smp_parse()
>     in hw/core/machine.c, but now machine.c is only built in softmmu.
>
>     In order to solve the build dependency on the smp parsing code and
>     avoid building unrelated stuff for the unit tests, move the tested
>     code from machine.c into a separate file, i.e., machine-smp.c and
>     build it in common field.
>
>     Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>     Reviewed-by: Andrew Jones <drjones@redhat.com>
>     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>     Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>     Message-Id: <20211026034659.22040-2-wangyanan55@huawei.com>
>     Acked-by: Eduardo Habkost <ehabkost@redhat.com>
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> I am still checking.

Yes this patch introduced the behavior of preferring cores over sockets
post 6.2. I think this is intentional. See the following hunk:

+        if (mc->smp_props.prefer_sockets) {
+            /* prefer sockets over cores before 6.2 */
+            if (sockets == 0) {
+                cores = cores > 0 ? cores : 1;
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            } else if (cores == 0) {
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            }
+        } else {
+            /* prefer cores over sockets since 6.2 */
+            if (cores == 0) {
+                sockets = sockets > 0 ? sockets : 1;
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            } else if (sockets == 0) {
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            }
+        }


prefer_sockets is set to true for 6.1 machines:

static void pc_q35_6_1_machine_options(MachineClass *m)
{
    pc_q35_6_2_machine_options(m);
    m->alias = NULL;
    compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
    compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
    m->smp_props.prefer_sockets = true;
}

So this behavior change is intended.