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é <berra...@redhat.com> > > This effectively reverts > > commit 54c4ea8f3ae614054079395842128a856a73dbf9 > Author: Zhao Liu <zhao1....@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é <berra...@redhat.com> > Reviewed-by: Zhao Liu <zhao1....@intel.com> > Reviewed-by: Ján Tomko <jto...@redhat.com> > Message-ID: <20240513123358.612355-2-berra...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <phi...@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 :|
