Re: [PATCH v20 02/21] s390x/cpu topology: add topology entries on CPU hotplug

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

On 5/3/23 12:23, Cédric Le Goater wrote:
> Hello,
>
> On 5/3/23 11:12, Thomas Huth wrote:
>> On 28/04/2023 14.35, Pierre Morel wrote:
>>>
>>> On 4/27/23 15:38, Thomas Huth wrote:
>>>> On 25/04/2023 18.14, Pierre Morel wrote:
>>>>> The topology information are attributes of the CPU and are
>>>>> specified during the CPU device creation.
>>>>>
>>>>> On hot plug we:
>>>>> - calculate the default values for the topology for drawers,
>>>>>    books and sockets in the case they are not specified.
>>>>> - verify the CPU attributes
>>>>> - check that we have still room on the desired socket
>>>>>
>>>>> The possibility to insert a CPU in a mask is dependent on the
>>>>> number of cores allowed in a socket, a book or a drawer, the
>>>>> checking is done during the hot plug of the CPU to have an
>>>>> immediate answer.
>>>>>
>>>>> If the complete topology is not specified, the core is added
>>>>> in the physical topology based on its core ID and it gets
>>>>> defaults values for the modifier attributes.
>>>>>
>>>>> This way, starting QEMU without specifying the topology can
>>>>> still get some advantage of the CPU topology.
>>>>>
>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>> ---
>>>> ...
>>>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>>>> new file mode 100644
>>>>> index 0000000000..471e0e7292
>>>>> --- /dev/null
>>>>> +++ b/hw/s390x/cpu-topology.c
>>>>> @@ -0,0 +1,259 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * CPU Topology
>>>>
>>>> Since you later introduce a file with almost the same name in the 
>>>> target/s390x/ folder, it would be fine to have some more 
>>>> explanation here what this file is all about (especially with 
>>>> regards to the other file in target/s390x/).
>>>
>>>
>>> I first did put the interceptions in target/s390/ then moved them in 
>>> target/s390x/kvm because it is KVM related then again only let STSI 
>>> interception.
>>>
>>> But to be honest I do not see any reason why not put everything in 
>>> hw/s390x/ if CPU topology is implemented for TCG I think the code 
>>> will call insert_stsi_15_1_x() too.
>>>
>>> no?
>>
>> Oh well, it's all so borderline ... whether you rather think of this 
>> as part of the CPU (like the STSI instruction) or rather part of the 
>> machine (drawers, books, ...).
>> I don't mind too much, as long as we don't have two files around with 
>> almost the same name (apart from "_" vs. "-"). So either keep the 
>> stsi part in target/s390x and use a better file name for that, or put 
>> everything together in one "cpu-topology.c" file.
>> Or what do others think about it?
>
> Would it make sense to have a target/s390x/stsi.c file with the stsi
> routines to be called from TCG insn helpers and from the KVM backend ?
> This suggestion is based on the services found in the ioinst.c file.
>
> So, target/s390x/kvm/cpu_topology.c would become target/s390x/stsi.c
> and stsi services would be moved there, if that makes sense.
>
> Or target/s390x/kvm/stsi.c to start with because services are only
> active for KVM targets.
>
>
> Looking at hw/s390x/meson.build :
>
>   s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>     'tod-kvm.c',
>     's390-skeys-kvm.c',
>     's390-stattrib-kvm.c',
>     'pv.c',
>     's390-pci-kvm.c',
>     'cpu-topology.c',
>   ))
>
> It seems cpu-topology.c should be named cpu-topology-kvm.c to follow
> the same convention.
>
> However, I don't see much reason for the KVM condition, apart from
> the new polarization definitions in machine-target.json which depend
> on KVM. cpu-topology.c could well be compiled without the KVM #ifdef,
> all seems in place to detect support at runtime.
>
> In this file, we find a s390_handle_ptf() which is called from
> kvm_handle_ptf() in target/s390x/kvm/kvm.c. Is it the right place for
> it ? Shouldn't we move the service under target/s390x/kvm/kvm.c ?
>
> Thanks,
>
> C.
>
>
Hello Cédric,

The reason to have the s390_handle_ptf() here is to be ready for TCG.

We have the choice to let the cpu-topology.c file inside the KVM list of 
meson.build until TCG provides the CPU_TOPOLOGY facility and then move 
it to the general list
or
to move it now to the general list inside meson.build and keep code that 
will only be useful when TCG provides the CPU_TOPOLOGY facility

The option taken here is to not let the code active if it is not in use.

Did I answered the question or forgot something?

Regards,

Pierre