Re: [PATCH v15 01/11] s390x/cpu topology: adding s390 specificities to CPU topology

[Thomas Huth <thuth@redhat.com>]

On 03/02/2023 10.39, Pierre Morel wrote:
> 
> @Thomas, these changes look good to me.
> What do you think, do I make the change and keep your RB ?

Yes, splitting that enum sounds cleaner, indeed.

  Thomas


> On 2/2/23 17:05, Nina Schoetterl-Glausch wrote:
>> Nit patch title: s390x/cpu topology: add s390 specifics to CPU topology ?
>>
> 
> OK
> 
>> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
>>> S390 adds two new SMP levels, drawers and books to the CPU
>>> topology.
>>> The S390 CPU have specific toplogy features like dedication
>>                                  ^o
> 
> Yes thx
> 
>>> and polarity 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 polarity,
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   qapi/machine.json               | 14 ++++++++--
>>>   include/hw/boards.h             | 10 ++++++-
>>>   include/hw/s390x/cpu-topology.h | 24 +++++++++++++++++
>>>   target/s390x/cpu.h              |  5 ++++
>>>   hw/core/machine-smp.c           | 48 ++++++++++++++++++++++++++++-----
>>>   hw/core/machine.c               |  4 +++
>>>   hw/s390x/s390-virtio-ccw.c      |  2 ++
>>>   softmmu/vl.c                    |  6 +++++
>>>   target/s390x/cpu.c              |  7 +++++
>>>   qemu-options.hx                 |  7 +++--
>>>   10 files changed, 115 insertions(+), 12 deletions(-)
>>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>>
>> [...]
>>>
>>> diff --git a/include/hw/s390x/cpu-topology.h 
>>> b/include/hw/s390x/cpu-topology.h
>>> new file mode 100644
>>> index 0000000000..7a84b30a21
>>> --- /dev/null
>>> +++ b/include/hw/s390x/cpu-topology.h
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * CPU Topology
>>> + *
>>> + * Copyright IBM Corp. 2022
>>> + *
>>> + * 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.
>>> + */
>>> +#ifndef HW_S390X_CPU_TOPOLOGY_H
>>> +#define HW_S390X_CPU_TOPOLOGY_H
>>> +
>>> +#define S390_TOPOLOGY_CPU_IFL   0x03
>>> +
>>> +enum s390_topology_polarity {
>>> +    POLARITY_HORIZONTAL,
>>> +    POLARITY_VERTICAL,
>>> +    POLARITY_VERTICAL_LOW = 1,
>>> +    POLARITY_VERTICAL_MEDIUM,
>>> +    POLARITY_VERTICAL_HIGH,
>>> +    POLARITY_MAX,
>>> +};
>>
>> Probably a good idea to keep the S390 prefix.
>> This works, but aliasing VERTICAL and VERTICAL_LOW is not
>> entirely straight forward.
>>
>> Why not have two enum?
>> enum s390_topology_polarity {
>>     S390_POLARITY_HORIZONTAL,
>>     S390_POLARITY_VERTICAL,
>> };
>>
>> enum s390_topology_entitlement {
>>     S390_ENTITLEMENT_LOW = 1,
>>     S390_ENTITLEMENT_MEDIUM,
>>     S390_ENTITLEMENT_HIGH,
>>     S390_ENTITLEMENT_MAX,
>> };
>> Maybe add an ENTITLEMENT_INVALID/NONE, if you need that, as first value.
>>
> 
> If Thomas agree, I do the changes.
> 
> Regards,
> Pierre
> 
>