Re: [PATCH v3 20/22] hw/i386/pc: Remove deprecated pc-i440fx-2.3 machine

2024-04-17 Thread Zhao Liu
On Tue, Apr 16, 2024 at 03:52:49PM +0200, Philippe Mathieu-Daudé wrote:
> Date: Tue, 16 Apr 2024 15:52:49 +0200
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH v3 20/22] hw/i386/pc: Remove deprecated pc-i440fx-2.3
>  machine
> X-Mailer: git-send-email 2.41.0
> 
> The pc-i440fx-2.3 machine was deprecated for the 8.2
> release (see commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated"),
> time to remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/about/deprecated.rst   |  4 ++--
>  docs/about/removed-features.rst |  2 +-
>  hw/i386/pc.c| 25 -
>  hw/i386/pc_piix.c   | 19 ---
>  4 files changed, 3 insertions(+), 47 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 01/22] hw/i386/pc: Deprecate 2.4 to 2.7 pc-i440fx machines

2024-04-17 Thread Zhao Liu
On Tue, Apr 16, 2024 at 03:52:30PM +0200, Philippe Mathieu-Daudé wrote:
> Date: Tue, 16 Apr 2024 15:52:30 +0200
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH v3 01/22] hw/i386/pc: Deprecate 2.4 to 2.7 pc-i440fx
>  machines
> X-Mailer: git-send-email 2.41.0
> 
> Similarly to the commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated",
> deprecate the 2.4 to 2.7 machines.
> 
> Suggested-by: Thomas Huth 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/about/deprecated.rst | 4 ++--
>  hw/i386/pc_piix.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

LGTM,

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any machine

2024-05-13 Thread Zhao Liu
Cc Paolo for x86 topology part

Hi Daniel,

On Mon, May 13, 2024 at 01:33:57PM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 13 May 2024 13:33:57 +0100
> From: "Daniel P. Berrangé" 
> Subject: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any
>  machine
> 
> This effectively reverts
> 
>   commit 54c4ea8f3ae614054079395842128a856a73dbf9
>   Author: Zhao Liu 
>   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.

Currently QEMU has become more and more difficult to maintain a general
topology hierarchy, there are two recent examples:

1. as you mentioned "module" v.s. "cluster", one reason for introducing
"module" is because it is difficult to define what "cluster" is for x86,
the cluster in the device tree can be nested, then it can correspond to
an x86 die, or it can correspond to an x86 module. Therefore, specifying
"clusters=1" for x86 is ambiguous.

2. s390 introduces book and drawer, which are above socket/package
level, but for x86, the level above the package names "cluster" (yeah,
"cluster" again :-(). So if user sets "books=1" or "drawers=1" for x86,
then it's meaningless. Similarly, "clusters=1" is also very confusing for
x86 machine.

I think that only thread/core/socket are architecturally general, the
other topology levels are hard to define across architectures, then
allowing unsupported topology=1 is always confusing...

Moreover, QEMU currently requires a clear topology containment
relationship when defining a topology, after which it will become
increasingly difficult to define a generic topology containment
relationship when new topology levels are introduced in the future...

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

I understand that libvirt is having trouble because there is no interface
to expose which topology levels the current machine supports. As a
workaround to eliminate the difficulties at the libvirt level, it's
ok for me.

But I believe deprecating the unsupported topology is necessary, so do
you think it's acceptable to include an interface to expose the supported
topology if it's going to be deprecated again later?

Regards,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any machine

2024-05-14 Thread Zhao Liu
> I'm failing to see what real world technical problems QEMU faces
> with a parameter being set to '1' by a mgmt app, when QEMU itself
> treats all omitted values as being '1' anyway.
> 
> If we're trying to faithfully model the real world, then restricting
> the topology against machine types though still looks inherantly wrong.
> The valid topology ought to be constrained based on the named CPU model.
> eg it doesn't make sense to allow 'dies=4' with a Skylake CPU model,
> only an EPYC CPU model, especially if we want to model cache info in
> a way that matches the real world silicon better.

Thanks for figuring out this. This issue is related with Intel CPU
cache model: currently Intel code defaults L3 shared at die level.
This could be resolved by defining the accurate default cache topology
level for CPU model and make Intel CPU models share L3 at package level
except only Cascadelake.

Then user could define any other topology levels (die/module) for
Icelake and this won't change the cache topology, unless the user adds
more sockets or further customizes the cache topology in another way [1].
Do you agree with this solution?

[1]: 
https://lore.kernel.org/qemu-devel/20240220092504.726064-1-zhao1@linux.intel.com/

[snip]

> As above, I think that restrictions based on machine type, while nice and
> simple, are incorrect long term. If we did impose restrictions based on
> CPU model, then we could trivially expose this info to mgmt apps via the
> existing mechanism for querying supported CPU models. Limiting based on
> CPU model, however, has potentially greater back compat issues, though
> it would be strictly more faithful to hardware.

I think as long as the default cache topology model is clearly defined,
users can further customize the CPU topology and adjust the cache
topology based on it. After all, topology is architectural, not CPU
model-specific (linux support for topology does not take into account
specific CPU models).

For example, x86, for simplicity, can we assume that all x86 CPU models
support all x86 topology levels (thread/core/module/die/package) without
making distinctions based on specific CPU models?

That way as long as the user doesn't change the default topology, then
Guest's cache and other topology information won't be "corrupted".

And there's one more question, does this rollback mean that smp's
parameters must have compatible default values for all architectures?

This is related with my SMP cache proposal above [1], should I provide
default entries (e.g. default) to be compatible with all architectures,
even if they don't support custom cache topology? Like the following:

-smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
 l1d-cache=default,l1i-cache=default,l2-cache=default,l3-cache=default

Thanks,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 02/18] hw/usb/hcd-xhci: Enumerate xhci_flags setting values

2024-03-07 Thread Zhao Liu
Hi Philippe,

On Tue, Mar 05, 2024 at 02:42:04PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Tue,  5 Mar 2024 14:42:04 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 02/18] hw/usb/hcd-xhci: Enumerate xhci_flags
>  setting values
> X-Mailer: git-send-email 2.41.0
> 
> xhci_flags are used as bits for QOM properties,
> expected to be somehow stable (external interface).
> 
> Explicit their values so removing any enum doesn't
> modify the other ones.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/hcd-xhci.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> index 98f598382a..37f0d2e43b 100644
> --- a/hw/usb/hcd-xhci.h
> +++ b/hw/usb/hcd-xhci.h
> @@ -37,8 +37,8 @@ typedef struct XHCIEPContext XHCIEPContext;
>  
>  enum xhci_flags {
>  XHCI_FLAG_SS_FIRST = 1,
> -XHCI_FLAG_FORCE_PCIE_ENDCAP,
> -XHCI_FLAG_ENABLE_STREAMS,
> +XHCI_FLAG_FORCE_PCIE_ENDCAP = 2,
> +XHCI_FLAG_ENABLE_STREAMS = 3,
>  };
>

From the commit 290fd20db6e0 ("usb xhci: change msi/msix property
type"), the enum values were modified directly.

So it seems not necessary to bind enum type with specific value,
right?

Thanks,
Zhao

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

2024-03-07 Thread Zhao Liu
On Thu, Mar 07, 2024 at 07:22:10AM +0100, Thomas Huth wrote:
> Date: Thu, 7 Mar 2024 07:22:10 +0100
> From: Thomas Huth 
> Subject: Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported
>  "parameter=1" SMP configurations
> 
> On 06/03/2024 10.53, Zhao Liu wrote:
> > From: Zhao Liu 
> > 
> > Currentlt, it was allowed for users to specify the unsupported
> 
> s/Currentlt/Currently/
> 
> > 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 depresated since 9.0. And report warning
> 
> s/depresated/deprecated/
> 
> > message for such case like:
> > 
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >  Unsupported clusters parameter mustn't be specified as 
> > 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >  Unsupported books parameter mustn't be specified as 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >  Unsupported drawers parameter mustn't be specified as 1
> > 
> > Users have to ensure that all the topology members described with -smp
> > are supported by the target machine.
> > 
> > Cc: devel@lists.libvirt.org
> > Signed-off-by: Zhao Liu 
> > ---
> >   docs/about/deprecated.rst | 14 +
> >   hw/core/machine-smp.c | 63 +--
> >   2 files changed, 61 insertions(+), 16 deletions(-)
> > 
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 872974640252..2e782e83e952 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as 
> > ``arg_name=on``.
> >   However, short-form booleans are deprecated and full explicit 
> > ``arg_name=on``
> >   form is preferred.
> > +``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)
> 
> s/Unsopported/Unsupported/
> 
> > +'''
> > +
> > +Specified CPU topology parameters must be supported by the machine.
> > +
> > +In the SMP configuration, users should provide the CPU topology parameters 
> > that
> > +are supported by the target machine.
> > +
> > +However, historically it was allowed for users to specify the unsupported
> > +topology parameter as "1", which is meaningless. So support for this kind 
> > of
> > +configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) 
> > is
> > +marked depresated since 9.0, users have to ensure that all the topology 
> > members
> 
> s/depresated/deprecated/
> 
> > +described with -smp are supported by the target machine.
> > +
> >   QEMU Machine Protocol (QMP) commands
> >   
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 96533886b14e..50a5a40dbc3d 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
> >   /*
> >* If not supported by the machine, a topology parameter must be
> > - * omitted or specified equal to 1.
> > + * omitted.
> >*/
> > -if (!mc->smp_props.dies_supported && dies > 1) {
> > -error_setg(errp, "dies not supported by this machine's CPU 
> > topology");
> > -return;
> > -}
> > -if (!mc->smp_props.clusters_supported && clusters > 1) {
> > -error_setg(errp, "clusters not supported by this machine's CPU 
> > topology");
> > -return;
> > +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): "
> > +

Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-06 Thread Zhao Liu
Hi Prasad,

> On Tue, 5 Mar 2024 at 12:59, Zhao Liu  wrote:
> > After simple test, if user sets maxcpus as 0, the has_maxcpus will be
> > true as well...I think it's related with QAPI code generation logic.
> 
> * Right.
> 
> [Maybe we digressed a bit in the discussion, so I snipped much of the
> details here. Sorry about that.]
> 
> * "if user sets maxcpus as 0, the has_maxcpus will be true as well",
> ie if 'has_*' fields are always set
> 
> unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> 
> then checking 'config->has_maxcpus ?' above is probably not required I
> think. It could just be
> 
>maxcpus = config->maxcpus

Yes.

> If a user does not specify config->maxcpus with -smp option, then it
> could default to zero(0) in 'config' parameter? (same for other config
> fields)
 
Yes. I could post another series for this cleanup soon.

> * If such change requires API changes (I'm not sure how), then
> probably it is outside the scope of this patch.
> 
> ...wdyt?
>

The above change you suggested doesn't require API changes ;-).

Thanks,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-06 Thread Zhao Liu
On Wed, Mar 06, 2024 at 10:19:41AM +0530, Prasad Pandit wrote:
> Date: Wed, 6 Mar 2024 10:19:41 +0530
> From: Prasad Pandit 
> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0"
>  SMP configurations
> 
> Hello Zhao,
> 
> On Wed, 6 Mar 2024 at 08:49, Zhao Liu  wrote:
> >> then checking 'config->has_maxcpus ?' above is probably not required I
> >> think. It could just be
> >>
> >>maxcpus = config->maxcpus
> >
> > Yes.
> >
> > > If a user does not specify config->maxcpus with -smp option, then it
> > > could default to zero(0) in 'config' parameter? (same for other config
> > > fields)
> >
> > Yes. I could post another series for this cleanup soon.
> > The above change you suggested doesn't require API changes ;-).
> 
> * Great!  (Communication is the most difficult skill to master. :))
> 
> * If you plan to send a separate patch for above refactoring, then I'd
> add Reviewed-by for this one.
 
Yeah, I will send a series, which will also include this patch, to avoid
trivial smp cleanup fragmentation.

> Reviewed-by: Prasad Pandit 

Thanks!

-Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 01/14] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-06 Thread Zhao Liu
From: Zhao Liu 

The "parameter=0" SMP configurations have been marked as deprecated
since v6.2.

For these cases, -smp currently returns the warning and adjusts the
zeroed parameters to 1 by default.

Remove the above compatibility logic in v9.0, and return error directly
if any -smp parameter is set as 0.

Cc: devel@lists.libvirt.org
Signed-off-by: Zhao Liu 
Reviewed-by: Thomas Huth 
Reviewed-by: Prasad Pandit 
---
 docs/about/deprecated.rst   | 16 
 docs/about/removed-features.rst | 15 +++
 hw/core/machine-smp.c   |  5 +++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 36bd3e15ef06..872974640252 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -36,22 +36,6 @@ and will cause a warning.
 The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on``
 rather than ``delay=off``.
 
-``-smp`` ("parameter=0" SMP configurations) (since 6.2)
-'''
-
-Specified CPU topology parameters must be greater than zero.
-
-In the SMP configuration, users should either provide a CPU topology
-parameter with a reasonable value (greater than zero) or just omit it
-and QEMU will compute the missing value.
-
-However, historically it was implicitly allowed for users to provide
-a parameter with zero value, which is meaningless and could also possibly
-cause unexpected results in the -smp parsing. So support for this kind of
-configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will
-be removed in the near future, users have to ensure that all the topology
-members described with -smp are greater than zero.
-
 Plugin argument passing through ``arg=`` (since 6.1)
 
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 417a0e4fa1d9..f9cf874f7b1f 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an 
accelerator property,
 and given a name that better reflects what it actually does.
 Use ``-accel tcg,one-insn-per-tb=on`` instead.
 
+``-smp`` ("parameter=0" SMP configurations) (removed in 9.0)
+
+
+Specified CPU topology parameters must be greater than zero.
+
+In the SMP configuration, users should either provide a CPU topology
+parameter with a reasonable value (greater than zero) or just omit it
+and QEMU will compute the missing value.
+
+However, historically it was implicitly allowed for users to provide
+a parameter with zero value, which is meaningless and could also possibly
+cause unexpected results in the -smp parsing. So support for this kind of
+configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have
+to ensure that all the topology members described with -smp are greater
+than zero.
 
 User-mode emulator command line arguments
 -
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 25019c91ee36..96533886b14e 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
 (config->has_cores && config->cores == 0) ||
 (config->has_threads && config->threads == 0) ||
 (config->has_maxcpus && config->maxcpus == 0)) {
-warn_report("Deprecated CPU topology (considered invalid): "
-"CPU topology parameters must be greater than zero");
+error_setg(errp, "Invalid CPU topology: "
+   "CPU topology parameters must be greater than zero");
+return;
 }
 
 /*
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

2024-03-06 Thread Zhao Liu
From: Zhao Liu 

Currentlt, 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 depresated since 9.0. And report warning
message for such case like:

qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
Unsupported clusters parameter mustn't be specified as 1
qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
Unsupported books parameter mustn't be specified as 1
qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
Unsupported drawers parameter mustn't be specified as 1

Users have to ensure that all the topology members described with -smp
are supported by the target machine.

Cc: devel@lists.libvirt.org
Signed-off-by: Zhao Liu 
---
 docs/about/deprecated.rst | 14 +
 hw/core/machine-smp.c | 63 +--
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 872974640252..2e782e83e952 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as 
``arg_name=on``.
 However, short-form booleans are deprecated and full explicit ``arg_name=on``
 form is preferred.
 
+``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)
+'''
+
+Specified CPU topology parameters must be supported by the machine.
+
+In the SMP configuration, users should provide the CPU topology parameters that
+are supported by the target machine.
+
+However, historically it was allowed for users to specify the unsupported
+topology parameter as "1", which is meaningless. So support for this kind of
+configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
+marked depresated since 9.0, users have to ensure that all the topology members
+described with -smp are supported by the target machine.
+
 QEMU Machine Protocol (QMP) commands
 
 
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 96533886b14e..50a5a40dbc3d 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
 
 /*
  * If not supported by the machine, a topology parameter must be
- * omitted or specified equal to 1.
+ * omitted.
  */
-if (!mc->smp_props.dies_supported && dies > 1) {
-error_setg(errp, "dies not supported by this machine's CPU topology");
-return;
-}
-if (!mc->smp_props.clusters_supported && clusters > 1) {
-error_setg(errp, "clusters not supported by this machine's CPU 
topology");
-return;
+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");
+}
 }
+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");
+}
+}
 dies = dies > 0 ? dies : 1;
-clusters = clusters > 0 ? clusters : 1;
 
-if (!mc->smp_props.books_supported && books > 1) {
-error_setg(errp, "books not supported by this machine's CPU topology");
-return;
+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 che

Re: [PATCH-for-9.1 v2 12/21] hw/i386/pc: Remove PCMachineClass::enforce_aligned_dimm

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:14AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:14 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 12/21] hw/i386/pc: Remove
>  PCMachineClass::enforce_aligned_dimm
> X-Mailer: git-send-email 2.41.0
> 
> PCMachineClass::enforce_aligned_dimm was only used by the
> pc-i440fx-2.1 machine, which got removed. It is now always
> true. Remove it, simplifying pc_get_device_memory_range().
> Update the comment in Avocado test_phybits_low_pse36().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/i386/pc.h  |  3 ---
>  hw/i386/pc.c  | 14 +++---
>  tests/avocado/mem-addr-space-check.py |  3 +--
>  3 files changed, 4 insertions(+), 16 deletions(-)

[snip]

> diff --git a/tests/avocado/mem-addr-space-check.py 
> b/tests/avocado/mem-addr-space-check.py
> index af019969c0..ad75170d52 100644
> --- a/tests/avocado/mem-addr-space-check.py
> +++ b/tests/avocado/mem-addr-space-check.py
> @@ -31,8 +31,7 @@ def test_phybits_low_pse36(self):
>  at 4 GiB boundary when "above_4g_mem_size" is 0 (this would be true 
> when
>  we have 0.5 GiB of VM memory, see pc_q35_init()). This means total
>  hotpluggable memory size is 60 GiB. Per slot, we reserve 1 GiB of 
> memory
> -for dimm alignment for all newer machines (see enforce_aligned_dimm
> -property for pc machines and pc_get_device_memory_range()). That 
> leaves
> +        for dimm alignment for all machines. That leaves

Just nit, better align it here.

Reviewed-by: Zhao Liu 

>  total hotpluggable actual memory size of 59 GiB. If the VM is started
>  with 0.5 GiB of memory, maxmem should be set to a maximum value of
>  59.5 GiB to ensure that the processor can address all memory 
> directly.
> -- 
> 2.41.0
> 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 04/21] hw/i386/acpi: Remove PCMachineClass::legacy_acpi_table_size

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:06AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:06 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 04/21] hw/i386/acpi: Remove
>  PCMachineClass::legacy_acpi_table_size
> X-Mailer: git-send-email 2.41.0
> 
> PCMachineClass::legacy_acpi_table_size was only used by the
> pc-i440fx-2.0 machine, which got removed. Remove it and simplify
> acpi_build().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/i386/pc.h |  1 -
>  hw/i386/acpi-build.c | 62 +---
>  2 files changed, 12 insertions(+), 51 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 16/21] hw/i386/pc: Remove PCMachineClass::resizable_acpi_blob

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:18AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:18 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 16/21] hw/i386/pc: Remove
>  PCMachineClass::resizable_acpi_blob
> X-Mailer: git-send-email 2.41.0
> 
> PCMachineClass::resizable_acpi_blob was only used by the
> pc-i440fx-2.2 machine, which got removed. It is now always
> true. Remove it, simplifying acpi_build().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/i386/pc.h |  3 ---
>  hw/i386/acpi-build.c | 10 --
>  hw/i386/pc.c |  1 -
>  3 files changed, 14 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 15/21] hw/i386/pc: Remove deprecated pc-i440fx-2.2 machine

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:17AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:17 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 15/21] hw/i386/pc: Remove deprecated
>  pc-i440fx-2.2 machine
> X-Mailer: git-send-email 2.41.0
> 
> The pc-i440fx-2.2 machine was deprecated for the 8.2
> release (see commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated"),
> time to remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-13-phi...@linaro.org>
> ---
>  docs/about/deprecated.rst   |  6 +++---
>  docs/about/removed-features.rst |  2 +-
>  include/hw/i386/pc.h|  3 ---
>  hw/i386/pc.c| 23 ---
>  hw/i386/pc_piix.c   | 21 -----
>  5 files changed, 4 insertions(+), 51 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 11/21] hw/smbios: Remove 'smbios_uuid_encoded', simplify smbios_encode_uuid()

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:13AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:13 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 11/21] hw/smbios: Remove 'smbios_uuid_encoded',
>  simplify smbios_encode_uuid()
> X-Mailer: git-send-email 2.41.0
> 
> 'smbios_encode_uuid' is always true, remove it,
> simplifying smbios_encode_uuid().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/smbios/smbios.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 05/21] hw/acpi/ich9: Remove 'memory-hotplug-support' property

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:07AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:07 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 05/21] hw/acpi/ich9: Remove
>  'memory-hotplug-support' property
> X-Mailer: git-send-email 2.41.0
> 
> No external code sets the 'memory-hotplug-support'
> property, remove it.
> 
> Suggested-by: Thomas Huth 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/acpi/ich9.c | 18 --
>  1 file changed, 18 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 08/21] target/i386/kvm: Remove x86_cpu_change_kvm_default() and 'kvm-cpu.h'

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:10AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:10 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 08/21] target/i386/kvm: Remove
>  x86_cpu_change_kvm_default() and 'kvm-cpu.h'
> X-Mailer: git-send-email 2.41.0
> 
> x86_cpu_change_kvm_default() was only used out of kvm-cpu.c by
> the pc-i440fx-2.1 machine, which got removed. Make it static,
> and remove its declaration. "kvm-cpu.h" is now empty, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-8-phi...@linaro.org>
> ---
>  target/i386/kvm/kvm-cpu.h | 41 ---
>  target/i386/kvm/kvm-cpu.c |  3 +--
>  2 files changed, 1 insertion(+), 43 deletions(-)
>  delete mode 100644 target/i386/kvm/kvm-cpu.h
> 
> diff --git a/target/i386/kvm/kvm-cpu.h b/target/i386/kvm/kvm-cpu.h
> deleted file mode 100644
> index e858ca21e5..00
> --- a/target/i386/kvm/kvm-cpu.h
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -/*
> - * i386 KVM CPU type and functions
> - *
> - *  Copyright (c) 2003 Fabrice Bellard
> - *
> - * This library is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2 of the License, or (at your option) any later version.
> - *
> - * This library is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> - */
> -
> -#ifndef KVM_CPU_H
> -#define KVM_CPU_H
> -
> -#ifdef CONFIG_KVM
> -/*
> - * Change the value of a KVM-specific default
> - *
> - * If value is NULL, no default will be set and the original
> - * value from the CPU model table will be kept.
> - *
> - * It is valid to call this function only for properties that
> - * are already present in the kvm_default_props table.
> - */
> -void x86_cpu_change_kvm_default(const char *prop, const char *value);

Features in kvm_default_props[] are supposed to be supported on the
oldest kernal version (v4.5, from docs/system/target-i386.rst).

So future PC machines will not use this interface to adjust
compatibility with the oldest v4.5 kernel. And it makes sense to stop
exposing this interface in the header. Thus,

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 06/21] hw/acpi/ich9: Remove dead code related to 'acpi_memory_hotplug'

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:08AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:08 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 06/21] hw/acpi/ich9: Remove dead code related to
>  'acpi_memory_hotplug'
> X-Mailer: git-send-email 2.41.0
> 
> acpi_memory_hotplug::is_enabled is set to %true once via
> ich9_lpc_initfn() -> ich9_pm_add_properties(). No need to
> check it, so remove now dead code.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/acpi/ich9.c | 28 ++--
>  1 file changed, 6 insertions(+), 22 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 03/21] hw/usb/hcd-xhci: Remove XHCI_FLAG_SS_FIRST flag

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:05AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:05 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 03/21] hw/usb/hcd-xhci: Remove
>  XHCI_FLAG_SS_FIRST flag
> X-Mailer: git-send-email 2.41.0
> 
> XHCI_FLAG_SS_FIRST was only used by the pc-i440fx-2.0 machine,
> which got removed. Remove it and simplify various functions in
> hcd-xhci.c.
> 
> Reviewed-by: Thomas Huth 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/hcd-xhci.h |  3 +--
>  hw/usb/hcd-xhci-nec.c |  2 --
>  hw/usb/hcd-xhci-pci.c |  1 -
>  hw/usb/hcd-xhci.c | 42 --
>  4 files changed, 9 insertions(+), 39 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 13/21] hw/mem/pc-dimm: Remove legacy_align argument from pc_dimm_pre_plug()

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:15AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:15 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 13/21] hw/mem/pc-dimm: Remove legacy_align
>  argument from pc_dimm_pre_plug()
> X-Mailer: git-send-email 2.41.0
> 
> 'legacy_align' is always NULL, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-11-phi...@linaro.org>
> ---
>  include/hw/mem/pc-dimm.h | 3 +--
>  hw/arm/virt.c| 2 +-
>  hw/i386/pc.c | 2 +-
>  hw/loongarch/virt.c  | 2 +-
>  hw/mem/pc-dimm.c | 6 ++
>  hw/ppc/spapr.c   | 2 +-
>  6 files changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 14/21] hw/mem/memory-device: Remove legacy_align from memory_device_pre_plug()

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:16AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:16 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 14/21] hw/mem/memory-device: Remove legacy_align
>  from memory_device_pre_plug()
> X-Mailer: git-send-email 2.41.0
> 
> 'legacy_align' is always NULL, remove it, simplifying
> memory_device_pre_plug().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-12-phi...@linaro.org>
> ---
>  include/hw/mem/memory-device.h |  2 +-
>  hw/i386/pc.c   |  3 +--
>  hw/mem/memory-device.c | 12 
>  hw/mem/pc-dimm.c   |  2 +-
>  hw/virtio/virtio-md-pci.c  |  2 +-
>  5 files changed, 8 insertions(+), 13 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 21/21] hw/i386/pc: Replace PCMachineClass::acpi_data_size by PC_ACPI_DATA_SIZE

2024-03-28 Thread Zhao Liu
Hi Philippe,

On Wed, Mar 27, 2024 at 10:51:23AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:23 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 21/21] hw/i386/pc: Replace
>  PCMachineClass::acpi_data_size by PC_ACPI_DATA_SIZE
> X-Mailer: git-send-email 2.41.0

[snip]

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 27e04c52f6..f01a30d3d9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -256,6 +256,16 @@ GlobalProperty pc_compat_2_4[] = {
>  };
>  const size_t pc_compat_2_4_len = G_N_ELEMENTS(pc_compat_2_4);
>  
> +/*
> + * @PC_ACPI_DATA_SIZE:
> + * Size of the chunk of memory at the top of RAM for the BIOS ACPI tables
> + * and other BIOS datastructures.
> + *
> + * BIOS ACPI tables: 128K. Other BIOS datastructures: less than 4K
> + * reported to be used at the moment, 32K should be enough for a while.
> + */
> +#define PC_ACPI_DATA_SIZE (0x2 + 0x8000)
> +

What about putting this in pc.h? Because it is a general definition for
PC.

Others are good for me,

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 09/21] hw/i386/pc: Remove PCMachineClass::smbios_uuid_encoded

2024-03-28 Thread Zhao Liu
Hi Philippe,

On Wed, Mar 27, 2024 at 10:51:11AM +0100, Philippe Mathieu-Daudé wrote:

[snip]

> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index d802d2787f..f7c2501161 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -63,8 +63,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState 
> *fw_cfg,
>  
>  if (pcmc->smbios_defaults) {
>  /* These values are guest ABI, do not change */
> -smbios_set_defaults("QEMU", mc->desc, mc->name,
> -pcmc->smbios_uuid_encoded);
> +smbios_set_defaults("QEMU", mc->desc, mc->name, true);

Since this parameter is always ture, then we can drop it and further
clean up the static flag "smbios_uuid_encoded" in hw/smbios/smbios.c.

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 07/21] hw/i386/pc: Remove deprecated pc-i440fx-2.1 machine

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:09AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:09 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 07/21] hw/i386/pc: Remove deprecated
>  pc-i440fx-2.1 machine
> X-Mailer: git-send-email 2.41.0
> 
> The pc-i440fx-2.1 machine was deprecated for the 8.2
> release (see commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated"),
> time to remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-7-phi...@linaro.org>
> ---
>  docs/about/deprecated.rst   |  2 +-
>  docs/about/removed-features.rst |  2 +-
>  include/hw/i386/pc.h|  3 ---
>  hw/i386/pc.c|  7 ---
>  hw/i386/pc_piix.c   | 23 -------
>  5 files changed, 2 insertions(+), 35 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 17/21] hw/i386/pc: Remove PCMachineClass::rsdp_in_ram

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:19AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:19 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 17/21] hw/i386/pc: Remove
>  PCMachineClass::rsdp_in_ram
> X-Mailer: git-send-email 2.41.0
> 
> PCMachineClass::rsdp_in_ram was only used by the
> pc-i440fx-2.2 machine, which got removed. It is
> now always true. Remove it, simplifying acpi_setup().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-15-phi...@linaro.org>
> ---
>  include/hw/i386/pc.h |  1 -
>  hw/i386/acpi-build.c | 35 ---
>  hw/i386/pc.c |  1 -
>  3 files changed, 4 insertions(+), 33 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 18/21] hw/i386/acpi: Remove AcpiBuildState::rsdp field

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:20AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:20 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 18/21] hw/i386/acpi: Remove AcpiBuildState::rsdp
>  field
> X-Mailer: git-send-email 2.41.0
> 
> AcpiBuildState::rsdp is always NULL, remove it,
> simplifying acpi_build_update().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-16-phi...@linaro.org>
> ---
>  hw/i386/acpi-build.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 02/21] hw/usb/hcd-xhci: Remove XHCI_FLAG_FORCE_PCIE_ENDCAP flag

2024-03-27 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:04AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:04 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 02/21] hw/usb/hcd-xhci: Remove
>  XHCI_FLAG_FORCE_PCIE_ENDCAP flag
> X-Mailer: git-send-email 2.41.0
> 
> XHCI_FLAG_FORCE_PCIE_ENDCAP was only used by the
> pc-i440fx-2.0 machine, which got removed. Remove it
> and simplify usb_xhci_pci_realize().
> 
> Reviewed-by: Thomas Huth 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/hcd-xhci.h | 1 -
>  hw/usb/hcd-xhci-nec.c | 2 --
>  hw/usb/hcd-xhci-pci.c | 3 +--
>  3 files changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 01/21] hw/i386/pc: Remove deprecated pc-i440fx-2.0 machine

2024-03-27 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:03AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:03 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 01/21] hw/i386/pc: Remove deprecated
>  pc-i440fx-2.0 machine
> X-Mailer: git-send-email 2.41.0
> 
> The pc-i440fx-2.0 machine was deprecated for the 8.2
> release (see commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated"),
> time to remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Message-Id: <20240305134221.30924-2-phi...@linaro.org>
> ---
>  docs/about/deprecated.rst   |  2 +-
>  docs/about/removed-features.rst |  2 +-
>  include/hw/i386/pc.h|  3 ---
>  hw/i386/pc.c| 15 -
>  hw/i386/pc_piix.c   | 37 -----
>  5 files changed, 2 insertions(+), 57 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 10/21] hw/smbios: Remove 'uuid_encoded' argument from smbios_set_defaults()

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:12AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:12 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 10/21] hw/smbios: Remove 'uuid_encoded' argument
>  from smbios_set_defaults()
> X-Mailer: git-send-email 2.41.0
> 
> 'uuid_encoded' is always NULL, remove it.

It's a boolean, so,

s/NULL/true/.


> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/firmware/smbios.h | 3 +--
>  hw/arm/virt.c| 3 +--
>  hw/i386/fw_cfg.c | 2 +-
>  hw/loongarch/virt.c  | 2 +-
>  hw/riscv/virt.c  | 2 +-
>  hw/smbios/smbios.c   | 6 ++
>  6 files changed, 7 insertions(+), 11 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 09/21] hw/i386/pc: Remove PCMachineClass::smbios_uuid_encoded

2024-03-28 Thread Zhao Liu
> Since this parameter is always ture, then we can drop it and further
> clean up the static flag "smbios_uuid_encoded" in hw/smbios/smbios.c.

Oops, my email didn't sync up well, the next two patches were doing
just that.

Thanks,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 20/21] target/i386: Remove X86CPU::kvm_no_smi_migration field

2024-03-28 Thread Zhao Liu
On Wed, Mar 27, 2024 at 10:51:22AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:22 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 20/21] target/i386: Remove
>  X86CPU::kvm_no_smi_migration field
> X-Mailer: git-send-email 2.41.0
> 
> X86CPU::kvm_no_smi_migration was only used by the
> pc-i440fx-2.3 machine, which got removed. Remove it
> and simplify kvm_put_vcpu_events().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/i386/cpu.h | 3 ---
>  target/i386/cpu.c | 2 --
>  target/i386/kvm/kvm.c | 7 +--
>  3 files changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Zhao Liu 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH-for-9.1 v2 19/21] hw/i386/pc: Remove 2.3 and deprecate 2.4 to 2.7 pc-i440fx machines

2024-03-28 Thread Zhao Liu
Hi Philippe,

On Wed, Mar 27, 2024 at 10:51:21AM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 27 Mar 2024 10:51:21 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH-for-9.1 v2 19/21] hw/i386/pc: Remove 2.3 and deprecate 2.4
>  to 2.7 pc-i440fx machines
> X-Mailer: git-send-email 2.41.0
> 
> The pc-i440fx-2.3 machine was deprecated for the 8.2
> release (see commit c7437f0ddb "docs/about: Mark the
> old pc-i440fx-2.0 - 2.3 machine types as deprecated"),
> time to remove it. Similarly to the cited commit,
> deprecate the 2.4 to 2.7 machines.

I suggest split the deprecation of 2.4-2.7 in another patch.

And when a old machine is marked as deprecated, is it necessary to
set "deprecation_reason" as commit c7437f0ddb?

I tend to set that field since boards.h said:

/**
 * MachineClass:
 * @deprecation_reason: If set, the machine is marked as deprecated. The
 *string should provide some clear information about what to use instead.
 *...
 */

And that field would be printed when user boots the machine.

Additionally, could we define rules for deprecating old machines?
For example, if it's more than 8 years old (as commit c7437f0ddb) or
after how many releases, the old machine can be considered for
deprecation.

Thanks,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Zhao Liu
From: Zhao Liu 

The "parameter=0" SMP configurations have been marked as deprecated
since v6.2.

For these cases, -smp currently returns the warning and adjusts the
zeroed parameters to 1 by default.

Remove the above compatibility logic in v9.0, and return error directly
if any -smp parameter is set as 0.

Signed-off-by: Zhao Liu 
---
 docs/about/deprecated.rst   | 16 
 docs/about/removed-features.rst | 15 +++
 hw/core/machine-smp.c   |  5 +++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 36bd3e15ef06..872974640252 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -36,22 +36,6 @@ and will cause a warning.
 The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on``
 rather than ``delay=off``.
 
-``-smp`` ("parameter=0" SMP configurations) (since 6.2)
-'''
-
-Specified CPU topology parameters must be greater than zero.
-
-In the SMP configuration, users should either provide a CPU topology
-parameter with a reasonable value (greater than zero) or just omit it
-and QEMU will compute the missing value.
-
-However, historically it was implicitly allowed for users to provide
-a parameter with zero value, which is meaningless and could also possibly
-cause unexpected results in the -smp parsing. So support for this kind of
-configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will
-be removed in the near future, users have to ensure that all the topology
-members described with -smp are greater than zero.
-
 Plugin argument passing through ``arg=`` (since 6.1)
 
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 417a0e4fa1d9..f9cf874f7b1f 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an 
accelerator property,
 and given a name that better reflects what it actually does.
 Use ``-accel tcg,one-insn-per-tb=on`` instead.
 
+``-smp`` ("parameter=0" SMP configurations) (removed in 9.0)
+
+
+Specified CPU topology parameters must be greater than zero.
+
+In the SMP configuration, users should either provide a CPU topology
+parameter with a reasonable value (greater than zero) or just omit it
+and QEMU will compute the missing value.
+
+However, historically it was implicitly allowed for users to provide
+a parameter with zero value, which is meaningless and could also possibly
+cause unexpected results in the -smp parsing. So support for this kind of
+configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have
+to ensure that all the topology members described with -smp are greater
+than zero.
 
 User-mode emulator command line arguments
 -
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 25019c91ee36..96533886b14e 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
 (config->has_cores && config->cores == 0) ||
 (config->has_threads && config->threads == 0) ||
 (config->has_maxcpus && config->maxcpus == 0)) {
-warn_report("Deprecated CPU topology (considered invalid): "
-"CPU topology parameters must be greater than zero");
+error_setg(errp, "Invalid CPU topology: "
+   "CPU topology parameters must be greater than zero");
+return;
 }
 
 /*
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Zhao Liu
Hi Prasad,

On Mon, Mar 04, 2024 at 11:23:58AM +0530, Prasad Pandit wrote:
> Date: Mon, 4 Mar 2024 11:23:58 +0530
> From: Prasad Pandit 
> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0"
>  SMP configurations
> 
> On Mon, 4 Mar 2024 at 10:02, Zhao Liu  wrote:
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 25019c91ee36..96533886b14e 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
> >  (config->has_cores && config->cores == 0) ||
> >  (config->has_threads && config->threads == 0) ||
> >  (config->has_maxcpus && config->maxcpus == 0)) {
> > -warn_report("Deprecated CPU topology (considered invalid): "
> > -"CPU topology parameters must be greater than zero");
> > +error_setg(errp, "Invalid CPU topology: "
> > +   "CPU topology parameters must be greater than zero");
> > +return;
> >  }
> 
> unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;

This indicates the default maxcpus is initialized as 0 if user doesn't
specifies it.

For this case - no user configuration - maxcpus will be re-calculated
as:

maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies *
  clusters * cores * threads; (*)

>  ...
>  if (config->has_maxcpus && config->maxcpus == 0)

This check only wants to identify the case that user sets the 0.

> 
> * The check (has_maxcpus && maxcpus == 0) seems to be repeating above,
> maybe we could check if (maxcpus == 0) error_setg().

If the default maxcpus is initialized as 0, then (maxcpus == 0) will
fail if user doesn't set maxcpus.

However, we could initialize maxcpus as other default value, e.g., 

maxcpus = config->has_maxcpus ? config->maxcpus : 1.

But it is still necessary to distinguish whether maxcpus is user-set or
auto-initialized.

If it is user-set, -smp should fail is there's invalid maxcpus/invalid
topology.

Otherwise, if it is auto-initialized, its value should be adjusted based
on other topology components as the above calculation in (*).

> And same for
> other topology parameters?

Other parameters also have the similar needs to distinguish if they're
set by user. So the check needs to also cover has_* fields.

> * Also a check to ensure cpus <= maxcpus is required I think.
>

Yes, the valid topology needs this. This code block already covers this
case ;-):

if (maxcpus < cpus) {
g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
error_setg(errp, "Invalid CPU topology: "
   "maxcpus must be equal to or greater than smp: "
   "%s == maxcpus (%u) < smp_cpus (%u)",
   topo_msg, maxcpus, cpus);
return;
}

Thanks,
Zhao

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-05 Thread Zhao Liu
Hi Prasad,

> On Mon, 4 Mar 2024 at 12:19, Zhao Liu  wrote:
> > > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> >
> > This indicates the default maxcpus is initialized as 0 if user doesn't
> > specifies it.
> 
> * 'has_maxcpus' should be set only if maxcpus > 0. If maxcpus == 0,
> then setting 'has_maxcpus=1' seems convoluted.

After simple test, if user sets maxcpus as 0, the has_maxcpus will be
true as well...I think it's related with QAPI code generation logic.

> > However, we could initialize maxcpus as other default value, e.g.,
> >
> > maxcpus = config->has_maxcpus ? config->maxcpus : 1.
> ===
> hw/core/machine.c
>  machine_initfn
> /* default to mc->default_cpus */
> ms->smp.cpus = mc->default_cpus;
> ms->smp.max_cpus = mc->default_cpus;
> 
>static void machine_class_base_init(ObjectClass *oc, void *data)
>{
>MachineClass *mc = MACHINE_CLASS(oc);
>mc->max_cpus = mc->max_cpus ?: 1;
>mc->min_cpus = mc->min_cpus ?: 1;
>mc->default_cpus = mc->default_cpus ?: 1;
>}
> ===
> * Looking at the above bits, it seems smp.cpus & smp.max_cpus are
> initialised to 1 via default_cpus in MachineClass object.

Yes.

The maxcpus I mentioned is a local virable in
machine_parse_smp_config(), whihc is used to do sanity-check check.

In machine_parse_smp_config(), when we can confirm the topology is
valid, then ms->smp.cpus and ms->smp.max_cpus are set with the valid
virables (cpus and maxcpus).

> >>  if (config->has_maxcpus && config->maxcpus == 0)
> > This check only wants to identify the case that user sets the 0.
> > If the default maxcpus is initialized as 0, then (maxcpus == 0) will
> > fail if user doesn't set maxcpus.
> >
> > But it is still necessary to distinguish whether maxcpus is user-set or
> > auto-initialized.
> 
> * If it is set to zero(0) either by user or by auto-initialise, it is
> still invalid, right?

The latter, "auto-initialise", means user could omit "cpus" and "maxcpus"
parameters in -smp.

Even though the local variable "cpus" and "maxcpus" are initialized as
0, eventually ms->smp.cpus and ms->smp.max_cpus will still have the
valid values.

> > If it is user-set, -smp should fail is there's invalid maxcpus/invalid
> > topology.
> >
> > Otherwise, if it is auto-initialized, its value should be adjusted based
> > on other topology components as the above calculation in (*).
> 
> * Why have such diverging ways?
> * Could we simplify it as
>- If cpus/maxcpus==0, it is invalid, show an error and exit.

Hmm, the origial behavior means if user doesn't set cpus=*/maxcpus=* in
-smp, then QEMU will auto-complete these 2 fields.

If we also return error for the above case that user omits cpus and
maxcpus parameters, then this change the QEMU's API and we need to mark
feature that the cpus/maxcpus parameter can be omitted as deprecated and
remove it out. Just like what I did in this patch for zeroed-parameter
case.

I feel if there's no issue then it's not necessary to change the API. Do
you agree?

>- If cpus/maxcpus > 0, but incorrect for topology, then
> re-calculate the correct value based on topology parameters. If the
> re-calculated value is still incorrect or unsatisfactory, then show an
> error and exit.

Yes, this case is right.

> * Saying that user setting cpu/maxcpus=0 is invalid and
> auto-initialising it to zero(0) is valid, is not consistent.
>

I think "auto-initialising it to zero(0)" doesn't means we re-initialize
ms->smp.cpus and ms->smp.max_cpus as 0 (these 2 fields store actual basic
topology information and they're defult as 1 as you said above).

Does my explaination address your concern? ;-)

Thanks,
Zhao
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any machine

2024-05-20 Thread Zhao Liu
On Wed, May 15, 2024 at 06:06:56PM +0100, Daniel P. Berrangé wrote:
> Date: Wed, 15 May 2024 18:06:56 +0100
> From: "Daniel P. Berrangé" 
> Subject: Re: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any
>  machine
> 
> On Tue, May 14, 2024 at 11:49:40AM +0800, Zhao Liu wrote:
> > > I'm failing to see what real world technical problems QEMU faces
> > > with a parameter being set to '1' by a mgmt app, when QEMU itself
> > > treats all omitted values as being '1' anyway.
> > > 
> > > If we're trying to faithfully model the real world, then restricting
> > > the topology against machine types though still looks inherantly wrong.
> > > The valid topology ought to be constrained based on the named CPU model.
> > > eg it doesn't make sense to allow 'dies=4' with a Skylake CPU model,
> > > only an EPYC CPU model, especially if we want to model cache info in
> > > a way that matches the real world silicon better.
> > 
> > Thanks for figuring out this. This issue is related with Intel CPU
> > cache model: currently Intel code defaults L3 shared at die level.
> > This could be resolved by defining the accurate default cache topology
> > level for CPU model and make Intel CPU models share L3 at package level
> > except only Cascadelake.
> > 
> > Then user could define any other topology levels (die/module) for
> > Icelake and this won't change the cache topology, unless the user adds
> > more sockets or further customizes the cache topology in another way [1].
> > Do you agree with this solution?
> 
> Broadly speaking yes. Historically we have created trouble for
> ourselves (and or our users) by allowing creation of "wierd"
> guest CPU models, which don't resemble those which can be found
> in real world silicon. Problems specifically have been around
> unsual combinations of CPUID features eg user enabled X, but not Y,
> where real silicon always has X + Y enabled, and guest OS assumed
> this is always the case.
> 
> So if our named CPU models can more faithfully match what you might
> see in terms of cache topology in the real world, that's likely to
> be a good thing.

Good to know what you think, it's the important guide for our work.

> > > As above, I think that restrictions based on machine type, while nice and
> > > simple, are incorrect long term. If we did impose restrictions based on
> > > CPU model, then we could trivially expose this info to mgmt apps via the
> > > existing mechanism for querying supported CPU models. Limiting based on
> > > CPU model, however, has potentially greater back compat issues, though
> > > it would be strictly more faithful to hardware.
> > 
> > I think as long as the default cache topology model is clearly defined,
> > users can further customize the CPU topology and adjust the cache
> > topology based on it. After all, topology is architectural, not CPU
> > model-specific (linux support for topology does not take into account
> > specific CPU models).
> > 
> > For example, x86, for simplicity, can we assume that all x86 CPU models
> > support all x86 topology levels (thread/core/module/die/package) without
> > making distinctions based on specific CPU models?
> 
> Hmm, true, if we have direct control over cache topology, the
> CPU topology is less critical. I'd still be wary of suggesting
> it is a good idea to use CPU topology configs that don't reflect
> something the CPU vendor has concievably used in real silicon.

Thank you! Yes, besides cache, it may affect other features like
Anthony's RAPL support, MSR is (die/package) topology-aware.

There's still a lot of open here, I'll take your warning into serious
consideration.

> > That way as long as the user doesn't change the default topology, then
> > Guest's cache and other topology information won't be "corrupted".
> 
> > And there's one more question, does this rollback mean that smp's
> > parameters must have compatible default values for all architectures?
> 
> Historically we preferred "sockets", when filling missing topology,
> then more recently we switched to prefer "cores", since high core
> counts are generally more common in real world than high socket
> counts.
> 
> In theory at some point, one might want to fill in 'dies > 0' for
> EPYC, or 'modules > 0' for appropriate Intel CPU models, but doing
> the reverse while theoretically valid, would be wierd as no such
> topology would exist in real silicon.
> 
> Ultimately if you're allowing QEMU guest vCPUs threads to float
> freely across host CPUs, there is little point in setting dies

Re: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any machine

2024-05-20 Thread Zhao Liu
On Mon, May 13, 2024 at 01:33:57PM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 13 May 2024 13:33:57 +0100
> From: "Daniel P. Berrangé" 
> Subject: [PATCH 1/2] hw/core: allow parameter=1 for SMP topology on any
>  machine
> 
> This effectively reverts
> 
>   commit 54c4ea8f3ae614054079395842128a856a73dbf9
>   Author: Zhao Liu 
>   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é 
> ---
>  docs/about/deprecated.rst   | 14 ---
>  hw/core/machine-smp.c   | 82 -
>  tests/unit/test-smp-parse.c |  8 ++--
>  3 files changed, 30 insertions(+), 74 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index e22acb17f2..5b551b12a6 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -47,20 +47,6 @@ as short-form boolean values, and passed to plugins as 
> ``arg_name=on``.
>  However, short-form booleans are deprecated and full explicit ``arg_name=on``
>  form is preferred.
>  
> -``-smp`` (Unsupported "parameter=1" SMP configurations) (since 9.0)
> -'''
> -
> -Specified CPU topology parameters must be supported by the machine.
> -
> -In the SMP configuration, users should provide the CPU topology parameters 
> that
> -are supported by the target machine.
> -
> -However, historically it was allowed for users to specify the unsupported
> -topology parameter as "1", which is meaningless. So support for this kind of
> -configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
> -marked deprecated since 9.0, users have to ensure that all the topology 
> members
> -described with -smp are supported by the target machine.
> -
>  User-mode emulator command line arguments
>  -
>  
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 2b93fa99c9..eb43caca9b 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -119,75 +119,45 @@ void machine_parse_smp_config(MachineState *ms,
>  
>  /*
>   * If not supported by the machine, a topology parameter must be

Also need to change this line as:

s/must be/must/

> - * omitted.
> + * not be set to a value greater than 1.
>   */

Only the above nit,

Reviewed-by: Zhao Liu 


Re: [PATCH 2/2] tests: add testing of parameter=1 for SMP topology

2024-05-20 Thread Zhao Liu
On Mon, May 13, 2024 at 01:33:58PM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 13 May 2024 13:33:58 +0100
> From: "Daniel P. Berrangé" 
> Subject: [PATCH 2/2] tests: add testing of parameter=1 for SMP topology
> 
> 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é 
> ---
>  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..56ce5128f1 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=4,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),
>  },
>  };
>  

As Xiaoyao's suggestion, only the nit in the comment.

Others look good to me, so,

Reviewed-by: Zhao Liu