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

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

On 4/18/23 10:53, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-04-03 at 18:28 +0200, 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>
>> ---
>>   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 ++++++++++
> Is hw/s390x the right path for cpu-topology?
> I haven't understood the difference between hw/s390x and target/s390x
> but target/s390x feels more correct, I could be mistaken though.

AFAIK target/s390 is for CPU emulation code while hw/s390 is for other 
emulation.

So it depends how we classify the CPU topology, it is related to CPU but 
it is no emulation.

Since Thomas approved this layout I would like to keep it like this.


>
>>   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/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' ] }
> You can get rid of the horizontal value now that the entitlement is ignored if the
> polarization is vertical.


Right, horizontal is not used, but what would you like?

- replace horizontal with 'none' ?

- add or substract 1 when we do the conversion between enum string and 
value ?

frankly I prefer to keep horizontal here which is exactly what is given 
in the documentation for entitlement = 0



>
> [...]
>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index b10a8541ff..57165fa3a0 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -37,6 +37,7 @@
>>   #ifndef CONFIG_USER_ONLY
>>   #include "sysemu/reset.h"
>>   #endif
>> +#include "hw/s390x/cpu-topology.h"
>>   
>>   #define CR0_RESET       0xE0UL
>>   #define CR14_RESET      0xC2000000UL;
>> @@ -259,6 +260,12 @@ static gchar *s390_gdb_arch_name(CPUState *cs)
>>   static Property s390x_cpu_properties[] = {
>>   #if !defined(CONFIG_USER_ONLY)
>>       DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0),
>> +    DEFINE_PROP_INT32("socket-id", S390CPU, env.socket_id, -1),
>> +    DEFINE_PROP_INT32("book-id", S390CPU, env.book_id, -1),
>> +    DEFINE_PROP_INT32("drawer-id", S390CPU, env.drawer_id, -1),
>> +    DEFINE_PROP_BOOL("dedicated", S390CPU, env.dedicated, false),
>> +    DEFINE_PROP_UINT8("entitlement", S390CPU, env.entitlement,
>> +                      S390_CPU_ENTITLEMENT__MAX),
> I would define an entitlement PropertyInfo in qdev-properties-system.[ch],
> then one can use e.g.
>
> -device z14-s390x-cpu,core-id=11,entitlement=high


Don't you think it is an enhancement we can do later?


>
> on the command line and cpu hotplug.
>
> I think setting the default entitlement to medium here should be fine.
>
> [...]

right, I had medium before and should not have change it.

Anyway what ever the default is, it must be changed later depending on 
dedication.