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

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

On 4/18/23 14:38, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-04-18 at 12:01 +0200, Pierre Morel wrote:
>> 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>
>>>> ---
> [...]
>>>> 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 ?
> Yeah, I would completely drop it because it is a meaningless value
> and adjust the conversion to the cpu value accordingly.
>> frankly I prefer to keep horizontal here which is exactly what is given
>> in the documentation for entitlement = 0
> Not sure what you mean with this.

I mean: Extract from the PoP:

----

The following values are used:
PP Meaning
0 The one or more CPUs represented by the TLE are
horizontally polarized.
1 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is low.
2 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is medium.
3 The one or more CPUs represented by the TLE are
vertically polarized. Entitlement is high.

----

Also I find that using an enum to systematically add/subtract a value is 
for me weird.

so I really prefer to keep "horizontal", "low", "medium", "high" event 
"horizontal" will never appear.

A mater of taste, it does not change anything to the functionality or 
the API.


>>
>>
>>> [...]
>>>
>>>> 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?
> It's a user visible change, so no.


We could have kept both string and integer.


> But it's not complicated, should be just:
>
> const PropertyInfo qdev_prop_cpus390entitlement = {
>      .name = "CpuS390Entitlement",
>      .enum_table = &CpuS390Entitlement_lookup,
>      .get   = qdev_propinfo_get_enum,
>      .set   = qdev_propinfo_set_enum,
>      .set_default_value = qdev_propinfo_set_default_value_enum,
> };
>
> Plus a comment & build bug in qdev-properties-system.c
>
> and
>
> extern const PropertyInfo qdev_prop_cpus390entitlement;
> #define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_cpus390entitlement, \
>                         CpuS390Entitlement)
>
> in qdev-properties-system.h
>
> You need to change the type of env.entitlement and set the default to 1 for medium
> and that should be it.


OK, it does not change anything to the functionality but is a little bit 
more pretty.


>>
>>> 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.
> No, you can just set it to medium and get rid of the adjustment code.
> s390_topology_check will reject invalid changes and the default above
> is fine since dedication is false.


I do not want a default specification for the entitlement to depend on 
the polarization.

If we do as you propose, by horizontal polarization a default 
entitlement with dedication will be accepted but will be refused after 
the guest switched for vertical polarization.

So we need adjustment before the check in both cases.

I find it easier and more logical if there is no default value than to 
have a default we need to overwrite.