Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

Cédric Le Goater
2022-10-24T09:22:37Z

On 10/19/22 17:39, Pierre Morel wrote:
>
>
> On 10/18/22 18:43, Cédric Le Goater wrote:
>> Hello Pierre,
>>
>> On 10/12/22 18:20, Pierre Morel wrote:
>>> In the S390x CPU topology the core_id specifies the CPU address
>>> and the position of the core withing the topology.
>>>
>>> Let's build the topology based on the core_id.
>>> s390x/cpu topology: core_id sets s390x CPU topology
>>>
>>> In the S390x CPU topology the core_id specifies the CPU address
>>> and the position of the cpu withing the topology.
>>>
>>> Let's build the topology based on the core_id.
>>
>> The commit log is doubled.
>
> Yes, thanks.
>
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>>   hw/s390x/meson.build            |   1 +
>>>   4 files changed, 199 insertions(+)
>>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>>   create mode 100644 hw/s390x/cpu-topology.c
>>>
>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>>> new file mode 100644
>>> index 0000000000..66c171d0bc
>>> --- /dev/null
>>> +++ b/include/hw/s390x/cpu-topology.h
>>> @@ -0,0 +1,45 @@
>>> +/*
>>> + * CPU Topology
>>> + *
>>> + * Copyright 2022 IBM Corp.
>>> + *
>>> + * 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
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +
>>> +typedef struct S390TopoContainer {
>>> +    int active_count;
>>> +} S390TopoContainer;
>>
>> This structure does not seem very useful.
>>
>>> +
>>> +#define S390_TOPOLOGY_CPU_IFL 0x03
>>> +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64)
>>> +typedef struct S390TopoTLE {
>>
>> The 'Topo' is redundant as TLE stands for 'topology-list entry'. This is minor.
>>
>>> +    uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN];
>>> +} S390TopoTLE;
>>> +
>>> +struct S390Topology {
>>> +    SysBusDevice parent_obj;
>>> +    int cpus;
>>> +    S390TopoContainer *socket;
>>> +    S390TopoTLE *tle;
>>> +    MachineState *ms;
>>
>> hmm, it would be cleaner to introduce the fields and properties needed
>> by the S390Topology model and avoid dragging the machine object pointer.
>> AFAICT, these properties would be :
>>
>>    "nr-cpus"
>>    "max-cpus"
>>    "nr-sockets"
>>
>
> OK
>
>>
>>
>>> +};
>>> +
>>> +#define TYPE_S390_CPU_TOPOLOGY "s390-topology"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY)
>>> +
>>> +S390Topology *s390_get_topology(void);
>>> +void s390_topology_new_cpu(int core_id);
>>> +
>>> +static inline bool s390_has_topology(void)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +#endif
>>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>>> new file mode 100644
>>> index 0000000000..42b22a1831
>>> --- /dev/null
>>> +++ b/hw/s390x/cpu-topology.c
>>> @@ -0,0 +1,132 @@
>>> +/*
>>> + * CPU Topology
>>> + *
>>> + * Copyright IBM Corp. 2022
>>
>> The Copyright tag is different in the .h file.
>
> OK, I change this to be like in the header file it seems to be the most used format.
>
>>
>>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>>> +
>>> + * 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.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "qemu/error-report.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/boards.h"
>>> +#include "qemu/typedefs.h"
>>> +#include "target/s390x/cpu.h"
>>> +#include "hw/s390x/s390-virtio-ccw.h"
>>> +#include "hw/s390x/cpu-topology.h"
>>> +
>>> +S390Topology *s390_get_topology(void)
>>> +{
>>> +    static S390Topology *s390Topology;
>>> +
>>> +    if (!s390Topology) {
>>> +        s390Topology = S390_CPU_TOPOLOGY(
>>> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
>>> +    }
>>> +
>>> +    return s390Topology;
>>
>> I am not convinced this routine is useful. The s390Topology pointer
>> could be stored under the machine state I think. It wouldn't be a
>> problem when CPUs are hot plugged since we have access to the machine
>> in the hot plug handler.
>
> OK, I add a pointer to the machine state that will be initialised during s390_init_topology()

LGTM.

>
>>
>> For the stsi call, 'struct ArchCPU' probably lacks a back pointer to
>> the machine objects with which CPU interact. These are typically
>> interrupt controllers or this new s390Topology model. You could add
>> the pointer there or, better, under a generic 'void *opaque' attribute.
>>
>> That said, what you did works fine. The modeling could be cleaner.
>
> Yes. I think you are right and I add a opaque pointer to the topology.

As an example, you could look at PPC where the PowerPCCPU CPU model is
shared between two differents machine, a baremetal one PowerNV and the
para-virtual one pSeries/sPAPR. Look for :

pnv_cpu_state(PowerPCCPU *cpu)
spapr_cpu_state(PowerPCCPU *cpu)

the machine CPU state is stored under an opaque cpu->machine_data which is
specific to each machine. It doesn't have to be as complex on s390 since
we only have one type of z-machine. An opaque is a good idea still.

Thanks,

C.