From: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-s390x@nongnu.org,
"Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Richard Henderson" <richard.henderson@linaro.org>,
"David Hildenbrand" <david@redhat.com>,
"Halil Pasic" <pasic@linux.ibm.com>,
"Christian Borntraeger" <borntraeger@linux.ibm.com>,
"Eric Farman" <farman@linux.ibm.com>,
"Thomas Huth" <thuth@redhat.com>,
"Eric Blake" <eblake@redhat.com>,
"Michael Roth" <michael.roth@amd.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Yanan Wang" <wangyanan55@huawei.com>,
"Daniel P. Berrangé" <berrange@redhat.com>,
"Ilya Leoshkevich" <iii@linux.ibm.com>,
"Cleber Rosa" <crosa@redhat.com>,
"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
"Beraldo Leal" <bleal@redhat.com>,
"Pierre Morel" <pmorel@linux.ibm.com>
Subject: Re: [PATCH v23 01/20] CPU topology: extend with s390 specifics
Date: Tue, 19 Sep 2023 19:51:34 +0200 [thread overview]
Message-ID: <e6ab27f4ef55d88f9585101434f900bd066145d6.camel@linux.ibm.com> (raw)
In-Reply-To: <87ediuwcrt.fsf@pond.sub.org>
On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote:
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
>
> > From: Pierre Morel <pmorel@linux.ibm.com>
> >
> > S390 adds two new SMP levels, drawers and books to the CPU
> > topology.
> > S390 CPUs have specific topology features like dedication and
> > entitlement. These indicate to the guest information on host
> > vCPU scheduling and help the guest make better scheduling decisions.
> >
> > Let us provide the SMP properties with books and drawers levels
> > and S390 CPU with dedication and entitlement,
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> > qapi/machine-common.json | 21 +++++++++++++
> > qapi/machine.json | 19 ++++++++++--
> > include/hw/boards.h | 10 +++++-
> > include/hw/qdev-properties-system.h | 4 +++
> > target/s390x/cpu.h | 6 ++++
> > hw/core/machine-smp.c | 48 ++++++++++++++++++++++++-----
> > hw/core/machine.c | 4 +++
> > hw/core/qdev-properties-system.c | 13 ++++++++
> > hw/s390x/s390-virtio-ccw.c | 4 +++
> > softmmu/vl.c | 6 ++++
> > target/s390x/cpu.c | 7 +++++
> > qapi/meson.build | 1 +
> > qemu-options.hx | 7 +++--
> > 13 files changed, 137 insertions(+), 13 deletions(-)
> > create mode 100644 qapi/machine-common.json
> >
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > new file mode 100644
> > index 0000000000..e40421bb37
> > --- /dev/null
> > +++ b/qapi/machine-common.json
>
> Why do you need a separate QAPI sub-module?
See here https://lore.kernel.org/qemu-devel/d8da6f7d1e3addcb63614f548ed77ac1b8895e63.camel@linux.ibm.com/
>
> > @@ -0,0 +1,21 @@
> > +# -*- Mode: Python -*-
> > +# vim: filetype=python
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> > +# See the COPYING file in the top-level directory.
> > +
> > +##
> > +# = Machines S390 data types
> > +##
> > +
> > +##
> > +# @CpuS390Entitlement:
> > +#
> > +# An enumeration of cpu entitlements that can be assumed by a virtual
> > +# S390 CPU
> > +#
> > +# Since: 8.2
> > +##
> > +{ 'enum': 'CpuS390Entitlement',
> > + 'prefix': 'S390_CPU_ENTITLEMENT',
> > + 'data': [ 'auto', 'low', 'medium', 'high' ] }
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index a08b6576ca..a63cb951d2 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -9,6 +9,7 @@
> ##
> # = Machines
> > ##
> >
> > { 'include': 'common.json' }
> > +{ 'include': 'machine-common.json' }
>
> Section structure is borked :)
>
> Existing section "Machine" now ends at the new "Machines S390 data
> types" you pull in here. The contents of below moves from "Machines" to
> "Machines S390 data types".
>
> Before I explain how to avoid this, I'd like to understand why we need a
> new sub-module.
>
> >
> > ##
> > # @SysEmuTarget:
> > @@ -71,7 +72,7 @@
> ##
> # @CpuInfoFast:
> #
> # Information about a virtual CPU
> #
> # @cpu-index: index of the virtual CPU
> #
> # @qom-path: path to the CPU object in the QOM tree
> > #
> > # @thread-id: ID of the underlying host thread
> > #
> > -# @props: properties describing to which node/socket/core/thread
> > +# @props: properties describing to which node/drawer/book/socket/core/thread
> > # virtual CPU belongs to, provided if supported by board
>
> Is this description accurate?
Kinda, although the wording might not be the best.
All the CpuInstanceProperties fields are optional, it's like a superset of possible
properties across architectures.
Only a subset might be returned by query-cpus-fast.
Also die and cluster are missing.
>
> @props is of type CpuInstanceProperties, shown below. Its documentation
> describes it as "properties to be used for hotplugging a CPU instance,
> it should be passed by management with device_add command when a CPU is
> being hotplugged." Hmm.
>
> I figure details ("node/drawer/book/socket/core/thread") are better left
> to CpuInstanceProperties.
>
> The "provided if supported by board" part makes no sense to me. If
> @props is there, it lists the properties we need to provide with
> device_add. What if it's not there? Same as empty list, i.e. we don't
> need to provide properties with device_add?
There are default values/default logic.
For s390x, socket, book, drawer are calculated from the core id
if not provided with device_add.
Partial specifications are rejected.
>
> Not your patch's fault, but let's get this in shape if we can.
>
> > #
> > # @target: the QEMU system emulation target, which determines which
> > @@ -901,7 +902,11 @@
> > #
> > # @node-id: NUMA node ID the CPU belongs to
> > #
> > -# @socket-id: socket number within node/board the CPU belongs to
> > +# @drawer-id: drawer number within node/board the CPU belongs to (since 8.2)
> > +#
> > +# @book-id: book number within drawer/node/board the CPU belongs to (since 8.2)
>
> Long lines, please wrap:
>
> # @drawer-id: drawer number within node/board the CPU belongs to
> # (since 8.2)
> #
> # @book-id: book number within drawer/node/board the CPU belongs to
> # (since 8.2)
Ok.
>
> > +#
> > +# @socket-id: socket number within book/node/board the CPU belongs to
> > #
> > # @die-id: die number within socket the CPU belongs to (since 4.1)
> > #
> > @@ -912,7 +917,7 @@
> ##
> # @CpuInstanceProperties:
> #
> # List of properties to be used for hotplugging a CPU instance, it
> # should be passed by management with device_add command when a CPU is
> # being hotplugged.
> #
> # @node-id: NUMA node ID the CPU belongs to
> #
> # @socket-id: socket number within node/board the CPU belongs to
> #
> # @die-id: die number within socket the CPU belongs to (since 4.1)
> #
> # @cluster-id: cluster number within die the CPU belongs to (since
> # 7.1)
> #
> # @core-id: core number within cluster the CPU belongs to
> > #
> > # @thread-id: thread number within core the CPU belongs to
> > #
> > -# Note: currently there are 6 properties that could be present but
> > +# Note: currently there are 8 properties that could be present but
> > # management should be prepared to pass through other properties
> > # with device_add command to allow for future interface extension.
> > # This also requires the filed names to be kept in sync with the
> # properties passed to -device/device_add.
>
> The last sentence is for developers, not for users, which means it
> doesn't belong here. Suggest to move it to a non-doc comment, and
> rephrase the note like
>
> # Note: management should be prepared to pass through additional
> # properties with device_add.
>
> > @@ -922,6 +927,8 @@
> > ##
> > { 'struct': 'CpuInstanceProperties',
>
> Non-doc comment could go here:
>
> # Keep these in sync with the properties device_add accepts
>
> Again, not your patch's fault, but your help improving this stuff would
> be appreciated.
>
> > 'data': { '*node-id': 'int',
> > + '*drawer-id': 'int',
> > + '*book-id': 'int',
> > '*socket-id': 'int',
> > '*die-id': 'int',
> > '*cluster-id': 'int',
> '*core-id': 'int',
> '*thread-id': 'int'
> }
> }
>
[...]
next prev parent reply other threads:[~2023-09-19 17:52 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-14 12:06 [PATCH v23 00/20] s390x: CPU Topology Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 01/20] CPU topology: extend with s390 specifics Nina Schoetterl-Glausch
2023-09-19 12:47 ` Markus Armbruster
2023-09-19 17:51 ` Nina Schoetterl-Glausch [this message]
2023-09-20 10:57 ` Markus Armbruster
2023-09-21 19:02 ` Nina Schoetterl-Glausch
2023-09-22 6:13 ` Markus Armbruster
2023-09-25 16:06 ` Nina Schoetterl-Glausch
2023-09-25 17:19 ` Markus Armbruster
2023-09-20 11:11 ` Markus Armbruster
2023-09-22 11:11 ` Nina Schoetterl-Glausch
2023-09-22 13:15 ` Markus Armbruster
2023-09-14 12:06 ` [PATCH v23 02/20] s390x/cpu topology: add topology entries on CPU hotplug Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 03/20] target/s390x/cpu topology: handle STSI(15) and build the SYSIB Nina Schoetterl-Glausch
2023-09-19 13:37 ` Nina Schoetterl-Glausch
2023-09-20 11:13 ` Markus Armbruster
2023-09-14 12:06 ` [PATCH v23 04/20] s390x/sclp: reporting the maximum nested topology entries Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 05/20] s390x/cpu topology: resetting the Topology-Change-Report Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 06/20] s390x/cpu topology: interception of PTF instruction Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 07/20] target/s390x/cpu topology: activate CPU topology Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 08/20] qapi/s390x/cpu topology: set-cpu-topology qmp command Nina Schoetterl-Glausch
2023-09-20 11:36 ` Markus Armbruster
2023-09-25 16:00 ` Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 09/20] machine: adding s390 topology to query-cpu-fast Nina Schoetterl-Glausch
2023-09-20 11:44 ` Markus Armbruster
2023-09-14 12:06 ` [PATCH v23 10/20] machine: adding s390 topology to info hotpluggable-cpus Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 11/20] qapi/s390x/cpu topology: CPU_POLARIZATION_CHANGE qapi event Nina Schoetterl-Glausch
2023-09-20 11:49 ` Markus Armbruster
2023-09-14 12:06 ` [PATCH v23 12/20] qapi/s390x/cpu topology: query-cpu-polarization qmp command Nina Schoetterl-Glausch
2023-09-20 11:51 ` Markus Armbruster
2023-09-14 12:06 ` [PATCH v23 13/20] docs/s390x/cpu topology: document s390x cpu topology Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 14/20] tests/avocado: s390x cpu topology core Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 15/20] tests/avocado: s390x cpu topology polarization Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 16/20] tests/avocado: s390x cpu topology entitlement tests Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 17/20] tests/avocado: s390x cpu topology test dedicated CPU Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 18/20] tests/avocado: s390x cpu topology test socket full Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 19/20] tests/avocado: s390x cpu topology dedicated errors Nina Schoetterl-Glausch
2023-09-14 12:06 ` [PATCH v23 20/20] tests/avocado: s390x cpu topology bad move Nina Schoetterl-Glausch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e6ab27f4ef55d88f9585101434f900bd066145d6.camel@linux.ibm.com \
--to=nsg@linux.ibm.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=bleal@redhat.com \
--cc=borntraeger@linux.ibm.com \
--cc=crosa@redhat.com \
--cc=david@redhat.com \
--cc=eblake@redhat.com \
--cc=eduardo@habkost.net \
--cc=farman@linux.ibm.com \
--cc=iii@linux.ibm.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=michael.roth@amd.com \
--cc=pasic@linux.ibm.com \
--cc=pbonzini@redhat.com \
--cc=philmd@linaro.org \
--cc=pmorel@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=thuth@redhat.com \
--cc=wainersm@redhat.com \
--cc=wangyanan55@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).