On Tue, 2023-09-19 at 14:47 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <n...@linux.ibm.com> writes: > > > From: Pierre Morel <pmo...@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 <pmo...@linux.ibm.com> > > Reviewed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com> > > Signed-off-by: Nina Schoetterl-Glausch <n...@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.ca...@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' > } > } > [...]
