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

[Markus Armbruster]

Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:

> 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?

Would work for me.

If the target-specific stuff in machine.json (discussed below) bothers
us, we can clean up on top.

>>     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.

Asking you to figure this out for all the targets wouldn't be fair, so I
won't.

Perhaps the documentation should state explicitly that properties may or
may not be optional, and ideally point to documentation that tells you
more, like the stuff you show below.  This might also address my
"undocumented magic" remark below.

Again, I'm not asking you to create documentation for the other targets.

>> 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.

Makes sense.  Let's replace it.

>> > > @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.
>> 
>> [...]
>>