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

[Pierre Morel <pmorel@linux.ibm.com>]

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