Re: [PATCH v23 01/20] CPU topology: extend with s390 specifics

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Wed, 2023-09-20 at 12:57 +0200, Markus Armbruster wrote:
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> 
> > 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/
> 
> 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.
> 
> [...]
>