On Wed, 2023-09-20 at 12:57 +0200, Markus Armbruster wrote: > Nina Schoetterl-Glausch <n...@linux.ibm.com> writes: > > > 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/ > > Quote: > > CpuS390Entitlement would be useful in both machine.json and > machine-target.json > > This is not obvious from this patch. I figure this patch could add it > to machine.json just fine. The use in machine-target.json in appears > only in PATCH 08.
Want me to add the rational to the commit message? > > because query-cpu-fast is defined in machine.json and set-cpu-topology is > defined > in machine-target.json. > > So then the question is where best to define CpuS390Entitlement. > In machine.json and include machine.json in machine-target.json? > Or define it in another file and include it from both? > > You do the latter in this patch. > > I figure the former would be tolerable, too. > > That said, having target-specific stuff in machine.json feels... odd. > Before this series, we have CpuInfoS390 and CpuS390State there, for > query-cpus-fast. That command returns a list of objects where common > members are target-independent, and the variant members are > target-dependent. qmp_query_cpus_fast() uses a CPU method to populate > the target-dependent members. > > I'm not sure splitting query-cpus-fast into a target-dependent and a > target-independent part is worth the bother. > > In this patch, you work with the structure you found. Can't fault you > for that :) > > > > > @@ -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. > > Let's see whether I got this right... > > The members of CpuInstanceProperties are properties you can pass to > device_add for some targets. Yes. > > The members present in a response from query-cpus-fast are properties > you must pass to device_add in this VM. Or is that a "may pass"? On x86 must pass, s390x may pass, I haven't checked other architectures. s390x shows the defaults calculated. > > On what exactly does the set of present members depend? Just the > target? The machine type? The CPU? Anything else? The target and the machine I'd say. On x86 if you have one die per socket you don't need to provide a die_id on device_add. > > > Also die and cluster are missing. > > Does this need fixing? Only if we keep the list of properties here. > > > @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. > > Undocumented magic? Patch 13 documents it: Default topology usage ---------------------- [...] When a CPU is defined by the ``-device`` command argument, the tree topology attributes must all be defined or all not defined. .. code-block:: bash -device gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 or .. code-block:: bash -device gen16b-s390x-cpu,core-id=1,dedicated=true If none of the tree attributes (drawer, book, sockets), are specified for the ``-device`` argument, like for all CPUs defined with the ``-smp`` command argument the topology tree attributes will be set by simply adding the CPUs to the topology based on the core-id. QEMU will not try to resolve collisions and will report an error if the CPU topology defined explicitly or implicitly on a ``-device`` argument collides with the definition of a CPU implicitly defined on the ``-smp`` argument. When the topology modifier attributes are not defined for the ``-device`` command argument they takes following default values: - dedicated: ``false`` - entitlement: ``medium`` Hot plug ++++++++ New CPUs can be plugged using the device_add hmp command as in: .. code-block:: bash (qemu) device_add gen16b-s390x-cpu,core-id=9 The placement of the CPU is derived from the core-id as described above. The topology can of course also be fully defined: .. code-block:: bash (qemu) device_add gen16b-s390x-cpu,drawer-id=1,book-id=1,socket-id=2,core-id=1 > > > > Not your patch's fault, but let's get this in shape if we can. > > [...] >