Re: [PATCH v3 20/22] hw/i386/pc: Remove deprecated pc-i440fx-2.3 machine
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
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
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
> 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
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
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
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
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
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
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
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
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
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
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()
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
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'
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'
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
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()
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()
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
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
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
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
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
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
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
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()
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
> 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
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
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
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
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
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
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
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
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