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

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

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?


>
>> + *
>> + * Copyright IBM Corp. 2022,2023
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>
> qemu/typedefs.h is already included by osdep.h by default, no need to 
> re-include it here.


OK , I remove it

>
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
> ...> +
>> +/**
>> + * s390_topology_setup_cpu:
>> + * @ms: MachineState used to initialize the topology structure on
>> + *      first call.
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @errp: the error pointer
>> + *
>> + * Called from CPU Hotplug to check and setup the CPU attributes
>
> s/Hotplug/hotplug/

yes


>
>> + * before the CPU is inserted in the topology.
>> + * There is no need to update the MTCR explicitely here because it
>
> s/explicitely/explicitly/

thanx


>
>> + * will be updated by KVM on creation of the new CPU.
>> + */
>> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error 
>> **errp)
> ...
>
>  Thomas
>
>

Regards,

Pierre