Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Janis Schoetterl-Glausch]

On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
> We will need a Topology device to transfer the topology
> during migration and to implement machine reset.
> 
> The device creation is fenced by s390_has_topology().
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>  include/hw/s390x/s390-virtio-ccw.h |  1 +
>  hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>  hw/s390x/meson.build               |  1 +
>  5 files changed, 158 insertions(+)
>  create mode 100644 include/hw/s390x/cpu-topology.h
>  create mode 100644 hw/s390x/cpu-topology.c
> 
[...]

> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> b/include/hw/s390x/s390-virtio-ccw.h
> index 9bba21a916..47ce0aa6fa 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>      bool dea_key_wrap;
>      bool pv;
>      uint8_t loadparm[8];
> +    DeviceState *topology;

Why is this a DeviceState, not S390Topology?
It *has* to be a S390Topology, right? Since you cast it to one in patch 2.

>  };
>  
>  struct S390CcwMachineClass {
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..bbf97cd66a
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> 
[...]
>  
> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
> +
> +    object_property_add_child(&machine->parent_obj,
> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));

Why set this property, and why on the machine parent?

> +    object_property_set_int(OBJECT(dev), "num-cores",
> +                            machine->smp.cores * machine->smp.threads, errp);
> +    object_property_set_int(OBJECT(dev), "num-sockets",
> +                            machine->smp.sockets, errp);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);

I must admit that I haven't fully grokked qemu's memory management yet.
Is the topology devices now owned by the sysbus?
If so, is it fine to have a pointer to it S390CcwMachineState?
> +
> +    return dev;
> +}
> +
[...]