[PULL 0/5] Misc HW patches & fixes for 2024-05-17

[PULL 0/5] Misc HW patches & fixes for 2024-05-17

[Philippe Mathieu-Daudé]

WARNING & ERROR from checkpatch.pl in tests/unit/test-smp-parse.c
deliberately ignored.

The following changes since commit 85ef20f1673feaa083f4acab8cf054df77b0dbed:

  Merge tag 'pull-maintainer-may24-160524-2' of https://gitlab.com/stsquad/qemu into staging (2024-05-16 10:02:56 +0200)

are available in the Git repository at:

  https://github.com/philmd/qemu.git tags/hw-misc-20240517

for you to fetch changes up to 93a3048dcf4565c73f2aa1d751f7197e296f1f1f:

  tests: Gently exit from GDB when tests complete (2024-05-17 16:49:04 +0200)

----------------------------------------------------------------
Misc HW patches queue

- Fix build when GBM buffer management library is detected (Cédric)
- Fix PFlash block write (Gerd)
- Allow 'parameter=1' for SMP topology on any machine (Daniel)
- Allow guest-debug tests to run with recent GDB (Gustavo)

----------------------------------------------------------------

Cédric Le Goater (1):
  ui/console: Only declare variable fence_fd when CONFIG_GBM is defined

Daniel P. Berrangé (2):
  hw/core: allow parameter=1 for SMP topology on any machine
  tests: add testing of parameter=1 for SMP topology

Gerd Hoffmann (1):
  hw/pflash: fix block write start

Gustavo Romero (1):
  tests: Gently exit from GDB when tests complete

 hw/block/pflash_cfi01.c           |  8 ++-
 hw/core/machine-smp.c             | 84 ++++++++++---------------------
 tests/unit/test-smp-parse.c       | 16 ++++--
 ui/gtk-egl.c                      |  2 +-
 tests/guest-debug/test_gdbstub.py |  2 +-
 5 files changed, 44 insertions(+), 68 deletions(-)

-- 
2.41.0

[PULL 1/5] ui/console: Only declare variable fence_fd when CONFIG_GBM is defined

[Philippe Mathieu-Daudé]

From: Cédric Le Goater <clg@redhat.com>

This to avoid a build breakage :

../ui/gtk-egl.c: In function ‘gd_egl_draw’:
../ui/gtk-egl.c:73:9: error: unused variable ‘fence_fd’ [-Werror=unused-variable]
   73 |     int fence_fd;
      |         ^~~~~~~~

Fixes: fa6426805b12 ("ui/console: Use qemu_dmabuf_set_..() helpers instead")
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20240515100520.574383-1-clg@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 ui/gtk-egl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 0473f689c9..9831c10e1b 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -68,9 +68,9 @@ void gd_egl_draw(VirtualConsole *vc)
     GdkWindow *window;
 #ifdef CONFIG_GBM
     QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
+    int fence_fd;
 #endif
     int ww, wh, ws;
-    int fence_fd;
 
     if (!vc->gfx.gls) {
         return;
-- 
2.41.0

[PULL 2/5] hw/pflash: fix block write start

[Philippe Mathieu-Daudé]

From: Gerd Hoffmann <kraxel@redhat.com>

Move the pflash_blk_write_start() call.  We need the offset of the
first data write, not the offset for the setup (number-of-bytes)
write.  Without this fix u-boot can do block writes to the first
flash block only.

While being at it drop a leftover FIXME.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2343
Fixes: 284a7ee2e290 ("hw/pflash: implement update buffer for block writes")
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20240516121237.534875-1-kraxel@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 1bda8424b9..c8f1cf5a87 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -518,10 +518,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             break;
         case 0xe8: /* Write to buffer */
             trace_pflash_write(pfl->name, "write to buffer");
-            /* FIXME should save @offset, @width for case 1+ */
-            qemu_log_mask(LOG_UNIMP,
-                          "%s: Write to buffer emulation is flawed\n",
-                          __func__);
             pfl->status |= 0x80; /* Ready! */
             break;
         case 0xf0: /* Probe for AMD flash */
@@ -574,7 +570,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             }
             pfl->counter = value;
             pfl->wcycle++;
-            pflash_blk_write_start(pfl, offset);
             break;
         case 0x60:
             if (cmd == 0xd0) {
@@ -605,6 +600,9 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         switch (pfl->cmd) {
         case 0xe8: /* Block write */
             /* FIXME check @offset, @width */
+            if (pfl->blk_offset == -1 && pfl->counter) {
+                pflash_blk_write_start(pfl, offset);
+            }
             if (!pfl->ro && (pfl->blk_offset != -1)) {
                 pflash_data_write(pfl, offset, value, width, be);
             } else {
-- 
2.41.0

[PULL 3/5] hw/core: allow parameter=1 for SMP topology on any machine

[Philippe Mathieu-Daudé]

From: Daniel P. Berrangé <berrange@redhat.com>

This effectively reverts

  commit 54c4ea8f3ae614054079395842128a856a73dbf9
  Author: Zhao Liu <zhao1.liu@intel.com>
  Date:   Sat Mar 9 00:01:37 2024 +0800

    hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

but is not done as a 'git revert' since the part of the changes to the
file hw/core/machine-smp.c which add 'has_XXX' checks remain desirable.
Furthermore, we have to tweak the subsequently added unit test to
account for differing warning message.

The rationale for the original deprecation was:

  "Currently, it was allowed for users to specify the unsupported
   topology parameter as "1". For example, x86 PC machine doesn't
   support drawer/book/cluster topology levels, but user could specify
   "-smp drawers=1,books=1,clusters=1".

   This is meaningless and confusing, so that the support for this kind
   of configurations is marked deprecated since 9.0."

There are varying POVs on the topic of 'unsupported' topology levels.

It is common to say that on a system without hyperthreading, that there
is always 1 thread. Likewise when new CPUs introduced a concept of
multiple "dies', it was reasonable to say that all historical CPUs
before that implicitly had 1 'die'. Likewise for the more recently
introduced 'modules' and 'clusters' parameter'. From this POV, it is
valid to set 'parameter=1' on the -smp command line for any machine,
only a value > 1 is strictly an error condition.

It doesn't cause any functional difficulty for QEMU, because internally
the QEMU code is itself assuming that all "unsupported" parameters
implicitly have a value of '1'.

At the libvirt level, we've allowed applications to set 'parameter=1'
when configuring a guest, and pass that through to QEMU.

Deprecating this creates extra difficulty for because there's no info
exposed from QEMU about which machine types "support" which parameters.
Thus, libvirt can't know whether it is valid to pass 'parameter=1' for
a given machine type, or whether it will trigger deprecation messages.

Since there's no apparent functional benefit to deleting this deprecated
behaviour from QEMU, and it creates problems for consumers of QEMU,
remove this deprecation.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Message-ID: <20240513123358.612355-2-berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/machine-smp.c       | 84 ++++++++++++-------------------------
 tests/unit/test-smp-parse.c |  8 ++--
 2 files changed, 31 insertions(+), 61 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 2b93fa99c9..5d8d7edcbd 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -118,76 +118,46 @@ void machine_parse_smp_config(MachineState *ms,
     }
 
     /*
-     * If not supported by the machine, a topology parameter must be
-     * omitted.
+     * If not supported by the machine, a topology parameter must
+     * not be set to a value greater than 1.
      */
-    if (!mc->smp_props.modules_supported && config->has_modules) {
-        if (config->modules > 1) {
-            error_setg(errp, "modules not supported by this "
-                       "machine's CPU topology");
-            return;
-        } else {
-            /* Here modules only equals 1 since we've checked zero case. */
-            warn_report("Deprecated CPU topology (considered invalid): "
-                        "Unsupported modules parameter mustn't be "
-                        "specified as 1");
-        }
+    if (!mc->smp_props.modules_supported &&
+        config->has_modules && config->modules > 1) {
+        error_setg(errp,
+                   "modules > 1 not supported by this machine's CPU topology");
+        return;
     }
     modules = modules > 0 ? modules : 1;
 
-    if (!mc->smp_props.clusters_supported && config->has_clusters) {
-        if (config->clusters > 1) {
-            error_setg(errp, "clusters not supported by this "
-                       "machine's CPU topology");
-            return;
-        } else {
-            /* Here clusters only equals 1 since we've checked zero case. */
-            warn_report("Deprecated CPU topology (considered invalid): "
-                        "Unsupported clusters parameter mustn't be "
-                        "specified as 1");
-        }
+    if (!mc->smp_props.clusters_supported &&
+        config->has_clusters && config->clusters > 1) {
+        error_setg(errp,
+                   "clusters > 1 not supported by this machine's CPU topology");
+        return;
     }
     clusters = clusters > 0 ? clusters : 1;
 
-    if (!mc->smp_props.dies_supported && config->has_dies) {
-        if (config->dies > 1) {
-            error_setg(errp, "dies not supported by this "
-                       "machine's CPU topology");
-            return;
-        } else {
-            /* Here dies only equals 1 since we've checked zero case. */
-            warn_report("Deprecated CPU topology (considered invalid): "
-                        "Unsupported dies parameter mustn't be "
-                        "specified as 1");
-        }
+    if (!mc->smp_props.dies_supported &&
+        config->has_dies && config->dies > 1) {
+        error_setg(errp,
+                   "dies > 1 not supported by this machine's CPU topology");
+        return;
     }
     dies = dies > 0 ? dies : 1;
 
-    if (!mc->smp_props.books_supported && config->has_books) {
-        if (config->books > 1) {
-            error_setg(errp, "books not supported by this "
-                       "machine's CPU topology");
-            return;
-        } else {
-            /* Here books only equals 1 since we've checked zero case. */
-            warn_report("Deprecated CPU topology (considered invalid): "
-                        "Unsupported books parameter mustn't be "
-                        "specified as 1");
-        }
+    if (!mc->smp_props.books_supported &&
+        config->has_books && config->books > 1) {
+        error_setg(errp,
+                   "books > 1 not supported by this machine's CPU topology");
+        return;
     }
     books = books > 0 ? books : 1;
 
-    if (!mc->smp_props.drawers_supported && config->has_drawers) {
-        if (config->drawers > 1) {
-            error_setg(errp, "drawers not supported by this "
-                       "machine's CPU topology");
-            return;
-        } else {
-            /* Here drawers only equals 1 since we've checked zero case. */
-            warn_report("Deprecated CPU topology (considered invalid): "
-                        "Unsupported drawers parameter mustn't be "
-                        "specified as 1");
-        }
+    if (!mc->smp_props.drawers_supported &&
+        config->has_drawers && config->drawers > 1) {
+        error_setg(errp,
+                   "drawers > 1 not supported by this machine's CPU topology");
+        return;
     }
     drawers = drawers > 0 ? drawers : 1;
 
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 8994337e12..56165e6644 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -337,21 +337,21 @@ static const struct SMPTestData data_generic_invalid[] = {
     {
         /* config: -smp 2,dies=2 */
         .config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
-        .expect_error = "dies not supported by this machine's CPU topology",
+        .expect_error = "dies > 1 not supported by this machine's CPU topology",
     }, {
         /* config: -smp 2,clusters=2 */
         .config = SMP_CONFIG_WITH_CLUSTERS(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
-        .expect_error = "clusters not supported by this machine's CPU topology",
+        .expect_error = "clusters > 1 not supported by this machine's CPU topology",
     }, {
         /* config: -smp 2,books=2 */
         .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, F, 0, T, 2, F,
                                                 0, F, 0, F, 0, F, 0),
-        .expect_error = "books not supported by this machine's CPU topology",
+        .expect_error = "books > 1 not supported by this machine's CPU topology",
     }, {
         /* config: -smp 2,drawers=2 */
         .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, T, 2, F, 0, F,
                                                 0, F, 0, F, 0, F, 0),
-        .expect_error = "drawers not supported by this machine's CPU topology",
+        .expect_error = "drawers > 1 not supported by this machine's CPU topology",
     }, {
         /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=8 */
         .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 8),
-- 
2.41.0

[PULL 4/5] tests: add testing of parameter=1 for SMP topology

[Philippe Mathieu-Daudé]

From: Daniel P. Berrangé <berrange@redhat.com>

Validate that it is possible to pass 'parameter=1' for any SMP topology
parameter, since unsupported parameters are implicitly considered to
always have a value of 1.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Message-ID: <20240513123358.612355-3-berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/unit/test-smp-parse.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 56165e6644..9fdba24fce 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -330,6 +330,14 @@ static const struct SMPTestData data_generic_valid[] = {
         .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 16),
         .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
         .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+    }, {
+        /*
+         * Unsupported parameters are always allowed to be set to '1'
+         * config: -smp 8,books=1,drawers=1,sockets=2,modules=1,dies=1,cores=2,threads=2,maxcpus=8
+         * expect: cpus=8,sockets=2,cores=2,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(8, 1, 1, 2, 1, 1, 2, 2, 8),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
     },
 };
 
-- 
2.41.0

[PULL 5/5] tests: Gently exit from GDB when tests complete

[Philippe Mathieu-Daudé]

From: Gustavo Romero <gustavo.romero@linaro.org>

GDB commit a207f6b3a38 ('Rewrite "python" command exception handling')
changed how exit() called from Python scripts loaded by GDB behave,
turning it into an exception instead of a generic error code that is
returned. This change caused several QEMU tests to crash with the
following exception:

Python Exception <class 'SystemExit'>: 0
Error occurred in Python: 0

This happens because in tests/guest-debug/test_gdbstub.py exit is
called after the tests have completed.

This commit fixes it by politely asking GDB to exit via gdb.execute,
passing the proper fail_count to be reported to 'make', instead of
abruptly calling exit() from the Python script.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20240515173132.2462201-4-gustavo.romero@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/guest-debug/test_gdbstub.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/guest-debug/test_gdbstub.py b/tests/guest-debug/test_gdbstub.py
index 7f71d34da1..46fbf98f0c 100644
--- a/tests/guest-debug/test_gdbstub.py
+++ b/tests/guest-debug/test_gdbstub.py
@@ -57,4 +57,4 @@ def main(test, expected_arch=None):
         pass
 
     print("All tests complete: {} failures".format(fail_count))
-    exit(fail_count)
+    gdb.execute(f"exit {fail_count}")
-- 
2.41.0

RE: [PULL 1/5] ui/console: Only declare variable fence_fd when CONFIG_GBM is defined

[Kim, Dongwon]

Thanks and sorry for missing this in the original commit.

Acked-by: Dongwon Kim <dongwon.kim@intel.com>

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Friday, May 17, 2024 8:02 AM
> To: qemu-devel@nongnu.org
> Cc: Cédric Le Goater <clg@redhat.com>; Kim, Dongwon
> <dongwon.kim@intel.com>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Philippe Mathieu-Daudé
> <philmd@linaro.org>
> Subject: [PULL 1/5] ui/console: Only declare variable fence_fd when
> CONFIG_GBM is defined
> 
> From: Cédric Le Goater <clg@redhat.com>
> 
> This to avoid a build breakage :
> 
> ../ui/gtk-egl.c: In function ‘gd_egl_draw’:
> ../ui/gtk-egl.c:73:9: error: unused variable ‘fence_fd’ [-Werror=unused-variable]
>    73 |     int fence_fd;
>       |         ^~~~~~~~
> 
> Fixes: fa6426805b12 ("ui/console: Use qemu_dmabuf_set_..() helpers instead")
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20240515100520.574383-1-clg@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  ui/gtk-egl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 0473f689c9..9831c10e1b 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -68,9 +68,9 @@ void gd_egl_draw(VirtualConsole *vc)
>      GdkWindow *window;
>  #ifdef CONFIG_GBM
>      QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> +    int fence_fd;
>  #endif
>      int ww, wh, ws;
> -    int fence_fd;
> 
>      if (!vc->gfx.gls) {
>          return;
> --
> 2.41.0

Re: [PULL 0/5] Misc HW patches & fixes for 2024-05-17

[Richard Henderson]

On 5/17/24 17:02, Philippe Mathieu-Daudé wrote:
> WARNING & ERROR from checkpatch.pl in tests/unit/test-smp-parse.c
> deliberately ignored.
> 
> The following changes since commit 85ef20f1673feaa083f4acab8cf054df77b0dbed:
> 
>    Merge tag 'pull-maintainer-may24-160524-2' ofhttps://gitlab.com/stsquad/qemu  into staging (2024-05-16 10:02:56 +0200)
> 
> are available in the Git repository at:
> 
>    https://github.com/philmd/qemu.git  tags/hw-misc-20240517
> 
> for you to fetch changes up to 93a3048dcf4565c73f2aa1d751f7197e296f1f1f:
> 
>    tests: Gently exit from GDB when tests complete (2024-05-17 16:49:04 +0200)
> 
> ----------------------------------------------------------------
> Misc HW patches queue
> 
> - Fix build when GBM buffer management library is detected (Cédric)
> - Fix PFlash block write (Gerd)
> - Allow 'parameter=1' for SMP topology on any machine (Daniel)
> - Allow guest-debug tests to run with recent GDB (Gustavo)

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate.


r~

Re: [PULL 3/5] hw/core: allow parameter=1 for SMP topology on any machine

[Daniel P. Berrangé]

Hi Michael,

This patch fixes a regression that was introduced in QEMU 9.0,
reported by yet another user at:

  https://gitlab.com/qemu-project/qemu/-/issues/2420

Could you pull this patch into stable-9.0.  If you think testing
is important for stable, the following patch adds further unit
testing coverage too.

Daniel

On Fri, May 17, 2024 at 05:02:25PM +0200, Philippe Mathieu-Daudé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> This effectively reverts
> 
>   commit 54c4ea8f3ae614054079395842128a856a73dbf9
>   Author: Zhao Liu <zhao1.liu@intel.com>
>   Date:   Sat Mar 9 00:01:37 2024 +0800
> 
>     hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations
> 
> but is not done as a 'git revert' since the part of the changes to the
> file hw/core/machine-smp.c which add 'has_XXX' checks remain desirable.
> Furthermore, we have to tweak the subsequently added unit test to
> account for differing warning message.
> 
> The rationale for the original deprecation was:
> 
>   "Currently, it was allowed for users to specify the unsupported
>    topology parameter as "1". For example, x86 PC machine doesn't
>    support drawer/book/cluster topology levels, but user could specify
>    "-smp drawers=1,books=1,clusters=1".
> 
>    This is meaningless and confusing, so that the support for this kind
>    of configurations is marked deprecated since 9.0."
> 
> There are varying POVs on the topic of 'unsupported' topology levels.
> 
> It is common to say that on a system without hyperthreading, that there
> is always 1 thread. Likewise when new CPUs introduced a concept of
> multiple "dies', it was reasonable to say that all historical CPUs
> before that implicitly had 1 'die'. Likewise for the more recently
> introduced 'modules' and 'clusters' parameter'. From this POV, it is
> valid to set 'parameter=1' on the -smp command line for any machine,
> only a value > 1 is strictly an error condition.
> 
> It doesn't cause any functional difficulty for QEMU, because internally
> the QEMU code is itself assuming that all "unsupported" parameters
> implicitly have a value of '1'.
> 
> At the libvirt level, we've allowed applications to set 'parameter=1'
> when configuring a guest, and pass that through to QEMU.
> 
> Deprecating this creates extra difficulty for because there's no info
> exposed from QEMU about which machine types "support" which parameters.
> Thus, libvirt can't know whether it is valid to pass 'parameter=1' for
> a given machine type, or whether it will trigger deprecation messages.
> 
> Since there's no apparent functional benefit to deleting this deprecated
> behaviour from QEMU, and it creates problems for consumers of QEMU,
> remove this deprecation.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> Message-ID: <20240513123358.612355-2-berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/core/machine-smp.c       | 84 ++++++++++++-------------------------
>  tests/unit/test-smp-parse.c |  8 ++--
>  2 files changed, 31 insertions(+), 61 deletions(-)
> 
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 2b93fa99c9..5d8d7edcbd 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -118,76 +118,46 @@ void machine_parse_smp_config(MachineState *ms,
>      }
>  
>      /*
> -     * If not supported by the machine, a topology parameter must be
> -     * omitted.
> +     * If not supported by the machine, a topology parameter must
> +     * not be set to a value greater than 1.
>       */
> -    if (!mc->smp_props.modules_supported && config->has_modules) {
> -        if (config->modules > 1) {
> -            error_setg(errp, "modules not supported by this "
> -                       "machine's CPU topology");
> -            return;
> -        } else {
> -            /* Here modules only equals 1 since we've checked zero case. */
> -            warn_report("Deprecated CPU topology (considered invalid): "
> -                        "Unsupported modules parameter mustn't be "
> -                        "specified as 1");
> -        }
> +    if (!mc->smp_props.modules_supported &&
> +        config->has_modules && config->modules > 1) {
> +        error_setg(errp,
> +                   "modules > 1 not supported by this machine's CPU topology");
> +        return;
>      }
>      modules = modules > 0 ? modules : 1;
>  
> -    if (!mc->smp_props.clusters_supported && config->has_clusters) {
> -        if (config->clusters > 1) {
> -            error_setg(errp, "clusters not supported by this "
> -                       "machine's CPU topology");
> -            return;
> -        } else {
> -            /* Here clusters only equals 1 since we've checked zero case. */
> -            warn_report("Deprecated CPU topology (considered invalid): "
> -                        "Unsupported clusters parameter mustn't be "
> -                        "specified as 1");
> -        }
> +    if (!mc->smp_props.clusters_supported &&
> +        config->has_clusters && config->clusters > 1) {
> +        error_setg(errp,
> +                   "clusters > 1 not supported by this machine's CPU topology");
> +        return;
>      }
>      clusters = clusters > 0 ? clusters : 1;
>  
> -    if (!mc->smp_props.dies_supported && config->has_dies) {
> -        if (config->dies > 1) {
> -            error_setg(errp, "dies not supported by this "
> -                       "machine's CPU topology");
> -            return;
> -        } else {
> -            /* Here dies only equals 1 since we've checked zero case. */
> -            warn_report("Deprecated CPU topology (considered invalid): "
> -                        "Unsupported dies parameter mustn't be "
> -                        "specified as 1");
> -        }
> +    if (!mc->smp_props.dies_supported &&
> +        config->has_dies && config->dies > 1) {
> +        error_setg(errp,
> +                   "dies > 1 not supported by this machine's CPU topology");
> +        return;
>      }
>      dies = dies > 0 ? dies : 1;
>  
> -    if (!mc->smp_props.books_supported && config->has_books) {
> -        if (config->books > 1) {
> -            error_setg(errp, "books not supported by this "
> -                       "machine's CPU topology");
> -            return;
> -        } else {
> -            /* Here books only equals 1 since we've checked zero case. */
> -            warn_report("Deprecated CPU topology (considered invalid): "
> -                        "Unsupported books parameter mustn't be "
> -                        "specified as 1");
> -        }
> +    if (!mc->smp_props.books_supported &&
> +        config->has_books && config->books > 1) {
> +        error_setg(errp,
> +                   "books > 1 not supported by this machine's CPU topology");
> +        return;
>      }
>      books = books > 0 ? books : 1;
>  
> -    if (!mc->smp_props.drawers_supported && config->has_drawers) {
> -        if (config->drawers > 1) {
> -            error_setg(errp, "drawers not supported by this "
> -                       "machine's CPU topology");
> -            return;
> -        } else {
> -            /* Here drawers only equals 1 since we've checked zero case. */
> -            warn_report("Deprecated CPU topology (considered invalid): "
> -                        "Unsupported drawers parameter mustn't be "
> -                        "specified as 1");
> -        }
> +    if (!mc->smp_props.drawers_supported &&
> +        config->has_drawers && config->drawers > 1) {
> +        error_setg(errp,
> +                   "drawers > 1 not supported by this machine's CPU topology");
> +        return;
>      }
>      drawers = drawers > 0 ? drawers : 1;
>  
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 8994337e12..56165e6644 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -337,21 +337,21 @@ static const struct SMPTestData data_generic_invalid[] = {
>      {
>          /* config: -smp 2,dies=2 */
>          .config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
> -        .expect_error = "dies not supported by this machine's CPU topology",
> +        .expect_error = "dies > 1 not supported by this machine's CPU topology",
>      }, {
>          /* config: -smp 2,clusters=2 */
>          .config = SMP_CONFIG_WITH_CLUSTERS(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
> -        .expect_error = "clusters not supported by this machine's CPU topology",
> +        .expect_error = "clusters > 1 not supported by this machine's CPU topology",
>      }, {
>          /* config: -smp 2,books=2 */
>          .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, F, 0, T, 2, F,
>                                                  0, F, 0, F, 0, F, 0),
> -        .expect_error = "books not supported by this machine's CPU topology",
> +        .expect_error = "books > 1 not supported by this machine's CPU topology",
>      }, {
>          /* config: -smp 2,drawers=2 */
>          .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, T, 2, F, 0, F,
>                                                  0, F, 0, F, 0, F, 0),
> -        .expect_error = "drawers not supported by this machine's CPU topology",
> +        .expect_error = "drawers > 1 not supported by this machine's CPU topology",
>      }, {
>          /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=8 */
>          .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 8),
> -- 
> 2.41.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 3/5] hw/core: allow parameter=1 for SMP topology on any machine

[Michael Tokarev]

03.07.2024 11:21, Daniel P. Berrangé wrote:
> Hi Michael,
> 
> This patch fixes a regression that was introduced in QEMU 9.0,
> reported by yet another user at:
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/2420

Aha.

> Could you pull this patch into stable-9.0.  If you think testing
> is important for stable, the following patch adds further unit
> testing coverage too.

Sure.  I think I considered applying it, but for some reason (which
I don't recall anymore) rejected that thought.

This change sort of clashes with 8ec0a46347987c7 "hw/core/machine:
Support modules in -smp" though, -- I'm fixing the context.

Yes, I'll pick up the test as well.

Thanks,

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt