Re: [PATCH v19 01/21] s390x/cpu topology: add s390 specifics to CPU topology

[Pierre Morel]

On 4/4/23 09:03, Cédric Le Goater wrote:
> On 4/3/23 18:28, Pierre Morel wrote:
> > S390 adds two new SMP levels, drawers and books to the CPU
> > topology.
> > The S390 CPU have specific topology features like dedication
> > and entitlement to give to the guest indications on the host
> > vCPUs scheduling and help the guest take the best decisions
> > on the scheduling of threads on the vCPUs.
> >
> > 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: Thomas Huth <thuth@redhat.com>
>
> Some minor comments below,
>
> > ---
> >   MAINTAINERS                     |  5 ++++
> >   qapi/machine-common.json        | 22 ++++++++++++++
> >   qapi/machine-target.json        | 12 ++++++++
> >   qapi/machine.json               | 17 +++++++++--
> >   include/hw/boards.h             | 10 ++++++-
> >   include/hw/s390x/cpu-topology.h | 15 ++++++++++
> >   target/s390x/cpu.h              |  5 ++++
> >   hw/core/machine-smp.c           | 53 ++++++++++++++++++++++++++++-----
> >   hw/core/machine.c               |  4 +++
> >   hw/s390x/s390-virtio-ccw.c      |  2 ++
> >   softmmu/vl.c                    |  6 ++++
> >   target/s390x/cpu.c              |  7 +++++
> >   qapi/meson.build                |  1 +
> >   qemu-options.hx                 |  7 +++--
> >   14 files changed, 152 insertions(+), 14 deletions(-)
> >   create mode 100644 qapi/machine-common.json
> >   create mode 100644 include/hw/s390x/cpu-topology.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5340de0515..9b1f80739e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1654,6 +1654,11 @@ F: hw/s390x/event-facility.c
> >   F: hw/s390x/sclp*.c
> >   L: qemu-s390x@nongnu.org
> >   +S390 CPU topology
> > +M: Pierre Morel <pmorel@linux.ibm.com>
> > +S: Supported
> > +F: include/hw/s390x/cpu-topology.h
> > +
> >   X86 Machines
> >   ------------
> >   PC
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > new file mode 100644
> > index 0000000000..73ea38d976
> > --- /dev/null
> > +++ b/qapi/machine-common.json
> > @@ -0,0 +1,22 @@
> > +# -*- 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.1
> > +##
> > +{ 'enum': 'CpuS390Entitlement',
> > +  'prefix': 'S390_CPU_ENTITLEMENT',
> > +  'data': [ 'horizontal', 'low', 'medium', 'high' ] }
> > +
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 2e267fa458..42a6a40333 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -342,3 +342,15 @@
> >                      'TARGET_S390X',
> >                      'TARGET_MIPS',
> >                      'TARGET_LOONGARCH64' ] } }
> > +
> > +##
> > +# @CpuS390Polarization:
> > +#
> > +# An enumeration of cpu polarization that can be assumed by a virtual
> > +# S390 CPU
> > +#
> > +# Since: 8.1
> > +##
> > +{ 'enum': 'CpuS390Polarization',
> > +  'prefix': 'S390_CPU_POLARIZATION',
> > +  'data': [ 'horizontal', 'vertical' ] }
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 604b686e59..1cdd83f3fd 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -9,6 +9,7 @@
> >   ##
> >     { 'include': 'common.json' }
> > +{ 'include': 'machine-common.json' }
> >     ##
> >   # @SysEmuTarget:
> > @@ -70,7 +71,7 @@
> >   #
> >   # @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
> >   #
> >   # @target: the QEMU system emulation target, which determines which
> > @@ -902,13 +903,15 @@
> >   # 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
> > +# @drawer-id: drawer number within node/board the CPU belongs to 
> > (since 8.1)
> > +# @book-id: book number within drawer/node/board the CPU belongs to 
> > (since 8.1)
> > +# @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)
> >   # @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
> > +# 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
> > @@ -918,6 +921,8 @@
> >   ##
> >   { 'struct': 'CpuInstanceProperties',
> >     'data': { '*node-id': 'int',
> > +            '*drawer-id': 'int',
> > +            '*book-id': 'int',
> >               '*socket-id': 'int',
> >               '*die-id': 'int',
> >               '*cluster-id': 'int',
> > @@ -1467,6 +1472,10 @@
> >   #
> >   # @cpus: number of virtual CPUs in the virtual machine
> >   #
> > +# @drawers: number of drawers in the CPU topology (since 8.1)
> > +#
> > +# @books: number of books in the CPU topology (since 8.1)
> > +#
> >   # @sockets: number of sockets in the CPU topology
> >   #
> >   # @dies: number of dies per socket in the CPU topology
> > @@ -1483,6 +1492,8 @@
> >   ##
> >   { 'struct': 'SMPConfiguration', 'data': {
> >        '*cpus': 'int',
> > +     '*drawers': 'int',
> > +     '*books': 'int',
> >        '*sockets': 'int',
> >        '*dies': 'int',
> >        '*clusters': 'int',
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 6fbbfd56c8..9ef0bb76cf 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -131,12 +131,16 @@ typedef struct {
> >    * @clusters_supported - whether clusters are supported by the machine
> >    * @has_clusters - whether clusters are explicitly specified in the 
> > user
> >    *                 provided SMP configuration
> > + * @books_supported - whether books are supported by the machine
> > + * @drawers_supported - whether drawers are supported by the machine
> >    */
> >   typedef struct {
> >       bool prefer_sockets;
> >       bool dies_supported;
> >       bool clusters_supported;
> >       bool has_clusters;
> > +    bool books_supported;
> > +    bool drawers_supported;
> >   } SMPCompatProps;
> >     /**
> > @@ -301,7 +305,9 @@ typedef struct DeviceMemoryState {
> >   /**
> >    * CpuTopology:
> >    * @cpus: the number of present logical processors on the machine
> > - * @sockets: the number of sockets on the machine
> > + * @drawers: the number of drawers on the machine
> > + * @books: the number of books in one drawer
> > + * @sockets: the number of sockets in one book
> >    * @dies: the number of dies in one socket
> >    * @clusters: the number of clusters in one die
> >    * @cores: the number of cores in one cluster
> > @@ -310,6 +316,8 @@ typedef struct DeviceMemoryState {
> >    */
> >   typedef struct CpuTopology {
> >       unsigned int cpus;
> > +    unsigned int drawers;
> > +    unsigned int books;
> >       unsigned int sockets;
> >       unsigned int dies;
> >       unsigned int clusters;
> > diff --git a/include/hw/s390x/cpu-topology.h 
> > b/include/hw/s390x/cpu-topology.h
> > new file mode 100644
> > index 0000000000..83f31604cc
> > --- /dev/null
> > +++ b/include/hw/s390x/cpu-topology.h
> > @@ -0,0 +1,15 @@
> > +/*
> > + * CPU Topology
> > + *
> > + * Copyright IBM Corp. 2022
>
> Shouldn't we have some range : 2022-2023 ?

There was a discussion on this in the first spins, I think to remember 
that Nina wanted 22 and Thomas 23,

now we have a third opinion :) .

I must say that all three have their reasons and I take what the 
majority wants.

A vote?


> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 
> > or (at
> > + * your option) any later version. See the COPYING file in the 
> > top-level
> > + * directory.
> > + */
>
> QEMU uses a SPDX tag like the kernel now :
>
> /* SPDX-License-Identifier: GPL-2.0-or-later */


OK, so I will add it on all .c and .h new files


> > +#ifndef HW_S390X_CPU_TOPOLOGY_H
> > +#define HW_S390X_CPU_TOPOLOGY_H
> > +
> > +#define S390_TOPOLOGY_CPU_IFL   0x03
>
> This definition is only used in patch 3. May be introduce it then,
> it would cleaner.


yes.


> > +
> > +#endif
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index 7d6d01325b..f2b2a38fe7 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -131,6 +131,11 @@ struct CPUArchState {
> >     #if !defined(CONFIG_USER_ONLY)
> >       uint32_t core_id; /* PoP "CPU address", same as cpu_index */
> > +    int32_t socket_id;
> > +    int32_t book_id;
> > +    int32_t drawer_id;
> > +    bool dedicated;
> > +    uint8_t entitlement;        /* Used only for vertical 
> > polarization */
> >       uint64_t cpuid;
> >   #endif
> >   diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index c3dab007da..6f5622a024 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -30,8 +30,19 @@ static char *cpu_hierarchy_to_string(MachineState 
> > *ms)
> >   {
> >       MachineClass *mc = MACHINE_GET_CLASS(ms);
> >       GString *s = g_string_new(NULL);
> > +    const char *multiply = " * ", *prefix = "";
> >   -    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
> > +    if (mc->smp_props.drawers_supported) {
> > +        g_string_append_printf(s, "drawers (%u)", ms->smp.drawers);
> > +    prefix = multiply;
>
> indent issue.

right, seems I forgot to update the patch set after the checkpatch.


> > +    }
> > +
> > +    if (mc->smp_props.books_supported) {
> > +        g_string_append_printf(s, "%sbooks (%u)", prefix, 
> > ms->smp.books);
> > +    prefix = multiply;
>
> ditto.
>
> > +    }
> > +

[...]

Thanks

Regards,

Pierre