| Message ID | 20221129174206.84882-2-pmorel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: CPU Topology | expand |
On 29/11/2022 18.42, 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> > --- ... > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 2e64ffab45..973bbdd36e 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -44,6 +44,7 @@ > #include "hw/s390x/pv.h" > #include "migration/blocker.h" > #include "qapi/visitor.h" > +#include "hw/s390x/cpu-topology.h" > > static Error *pv_mig_blocker; > > @@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine) > } > } > > +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)); > + object_property_set_int(OBJECT(dev), "num-cores", > + machine->smp.cores * machine->smp.threads, errp); I wonder what will happen if we ever support multithreading on s390x later? ... won't this cause some oddities when migrating older machines types with smp.threads > 1 later? Maybe we should prohibit to enable the CPU topology instead if a user tried to use threads > 1 with an older machine type? Thomas > + object_property_set_int(OBJECT(dev), "num-sockets", > + machine->smp.sockets, errp); > + > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp); > + > + return dev; > +}
On 12/1/22 10:08, Thomas Huth wrote: > On 29/11/2022 18.42, 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> >> --- > ... >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 2e64ffab45..973bbdd36e 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -44,6 +44,7 @@ >> #include "hw/s390x/pv.h" >> #include "migration/blocker.h" >> #include "qapi/visitor.h" >> +#include "hw/s390x/cpu-topology.h" >> static Error *pv_mig_blocker; >> @@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine) >> } >> } >> +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)); >> + object_property_set_int(OBJECT(dev), "num-cores", >> + machine->smp.cores * >> machine->smp.threads, errp); > > I wonder what will happen if we ever support multithreading on s390x > later? ... won't this cause some oddities when migrating older machines > types with smp.threads > 1 later? Maybe we should prohibit to enable the > CPU topology instead if a user tried to use threads > 1 with an older > machine type? Yes, right, I forgot to change this back. Anyway it has no sens for new machine which prohibit smp.threads > 1 neither. I change this by returning an error in case we have smp.threads > 1 Thanks. Pierre > > Thomas > > >> + object_property_set_int(OBJECT(dev), "num-sockets", >> + machine->smp.sockets, errp); >> + >> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp); >> + >> + return dev; >> +} >
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; > +} > + [...]
On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: > 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. Yes, currently it is the S390Topology. The idea of Cedric was to have something more generic for future use. > >> }; >> >> 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? For what I understood setting the num_cores and num_sockets as properties of the CPU Topology object allows to have them better integrated in the QEMU object framework. The topology is added to the S390CcwmachineState, it is the parent of the machine. > >> + 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? Yes it is so we see it on the qtree with its properties. > If so, is it fine to have a pointer to it S390CcwMachineState? Why not? It seems logical to me that the sysbus belong to the virtual machine. But sometime the way of QEMU are not very transparent for me :) so I can be wrong. Regards, Pierre >> + >> + return dev; >> +} >> + > [...]
On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: > > On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: > > 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. > > Yes, currently it is the S390Topology. > The idea of Cedric was to have something more generic for future use. But it still needs to be a S390Topology otherwise you cannot cast it to one, can you? > > > > > > }; > > > > > > 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? > > For what I understood setting the num_cores and num_sockets as > properties of the CPU Topology object allows to have them better > integrated in the QEMU object framework. That I understand. > > The topology is added to the S390CcwmachineState, it is the parent of > the machine. But why? And is it added to the S390CcwMachineState, or its 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? > > Yes it is so we see it on the qtree with its properties. > > > > If so, is it fine to have a pointer to it S390CcwMachineState? > > Why not? If it's owned by the sysbus and the object is not explicitly referenced for the pointer, it might be deallocated and then you'd have a dangling pointer. > It seems logical to me that the sysbus belong to the virtual machine. > But sometime the way of QEMU are not very transparent for me :) > so I can be wrong. > > Regards, > Pierre > > > > + > > > + return dev; > > > +} > > > + > > [...] >
On 12/6/22 14:35, Janis Schoetterl-Glausch wrote: > On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: >> >> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: >>> 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. >> >> Yes, currently it is the S390Topology. >> The idea of Cedric was to have something more generic for future use. > > But it still needs to be a S390Topology otherwise you cannot cast it to one, can you? May be I did not understand correctly what Cedric wants. For my part I agree with you I do not see the point to have something different than a S390Topology pointer. Also doing that is more secure as we do not need cast... which reveals a bug I have in setup_stsi().... !!!! Let's do that and see what Cedric says. >> >>> >>>> }; >>>> >>>> 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? >> >> For what I understood setting the num_cores and num_sockets as >> properties of the CPU Topology object allows to have them better >> integrated in the QEMU object framework. > > That I understand. >> >> The topology is added to the S390CcwmachineState, it is the parent of >> the machine. > > But why? And is it added to the S390CcwMachineState, or its parent? it is added to the S390CcwMachineState. We receive the MachineState as the "machine" parameter here and it is added to the "machine->parent_obj" which is the S390CcwMachineState. >> >> >>> >>>> + 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? >> >> Yes it is so we see it on the qtree with its properties. >> >> >>> If so, is it fine to have a pointer to it S390CcwMachineState? >> >> Why not? > > If it's owned by the sysbus and the object is not explicitly referenced > for the pointer, it might be deallocated and then you'd have a dangling pointer. Why would it be deallocated ? as long it is not unrealized it belongs to the sysbus doesn't it? Regards, Pierre
On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote: > > On 12/6/22 14:35, Janis Schoetterl-Glausch wrote: > > On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: > > > > > > On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: > > > > 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 > > > > [...] > > > > > > > > > > +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? > > > > > > For what I understood setting the num_cores and num_sockets as > > > properties of the CPU Topology object allows to have them better > > > integrated in the QEMU object framework. > > > > That I understand. > > > > > > The topology is added to the S390CcwmachineState, it is the parent of > > > the machine. > > > > But why? And is it added to the S390CcwMachineState, or its parent? > > it is added to the S390CcwMachineState. > We receive the MachineState as the "machine" parameter here and it is > added to the "machine->parent_obj" which is the S390CcwMachineState. Oh, I was confused. &machine->parent_obj is just a cast of MachineState* to Object*. It's the very same object. And what is the reason to add the topology as child property? Just so it shows up in the qtree? Wouldn't it anyway under the sysbus? > > > > > > > > > > > > > > > > > > + 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? > > > > > > Yes it is so we see it on the qtree with its properties. > > > > > > > > > > If so, is it fine to have a pointer to it S390CcwMachineState? > > > > > > Why not? > > > > If it's owned by the sysbus and the object is not explicitly referenced > > for the pointer, it might be deallocated and then you'd have a dangling pointer. > > Why would it be deallocated ? That's beside the point, if you transfer ownership, you have no control over when the deallocation happens. It's going to be fine in practice, but I don't think you should rely on it. I think you could just do sysbus_realize instead of ..._and_unref, but like I said, I haven't fully understood qemu memory management. (It would also leak in a sense, but since the machine exists forever that should be fine) > as long it is not unrealized it belongs to the sysbus doesn't it? > > Regards, > Pierre >
On 12/6/22 22:06, Janis Schoetterl-Glausch wrote: > On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote: >> >> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote: >>> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: >>>> >>>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: >>>>> 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 >>>>> > [...] >>>>>> >>>>>> +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? >>>> >>>> For what I understood setting the num_cores and num_sockets as >>>> properties of the CPU Topology object allows to have them better >>>> integrated in the QEMU object framework. >>> >>> That I understand. >>>> >>>> The topology is added to the S390CcwmachineState, it is the parent of >>>> the machine. >>> >>> But why? And is it added to the S390CcwMachineState, or its parent? >> >> it is added to the S390CcwMachineState. >> We receive the MachineState as the "machine" parameter here and it is >> added to the "machine->parent_obj" which is the S390CcwMachineState. > > Oh, I was confused. &machine->parent_obj is just a cast of MachineState* to Object*. > It's the very same object. > And what is the reason to add the topology as child property? > Just so it shows up in the qtree? Wouldn't it anyway under the sysbus? Yes it would appear on the info qtree but not in the qom-tree >> >> >> >>>> >>>> >>>>> >>>>>> + 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? >>>> >>>> Yes it is so we see it on the qtree with its properties. >>>> >>>> >>>>> If so, is it fine to have a pointer to it S390CcwMachineState? >>>> >>>> Why not? >>> >>> If it's owned by the sysbus and the object is not explicitly referenced >>> for the pointer, it might be deallocated and then you'd have a dangling pointer. >> >> Why would it be deallocated ? > > That's beside the point, if you transfer ownership, you have no control over when > the deallocation happens. > It's going to be fine in practice, but I don't think you should rely on it. > I think you could just do sysbus_realize instead of ..._and_unref, > but like I said, I haven't fully understood qemu memory management. > (It would also leak in a sense, but since the machine exists forever that should be fine) If I understand correctly: - qdev_new adds a reference count to the new created object, dev. - object_property_add_child adds a reference count to the child also here the new created device dev so the ref count of dev is 2 . after the unref on dev, the ref count of dev get down to 1 then it seems OK. Did I miss something? Regards, Pierre > >> as long it is not unrealized it belongs to the sysbus doesn't it? >> >> Regards, >> Pierre >> >
* On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote: > > On 12/6/22 22:06, Janis Schoetterl-Glausch wrote: > > On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote: > > > > > > On 12/6/22 14:35, Janis Schoetterl-Glausch wrote: > > > > On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: > > > > > > > > > > On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: > > > > > > 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 > > > > > > [...] > > > > > > > + 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? > > > > > > > > > > Yes it is so we see it on the qtree with its properties. > > > > > > > > > > > > > > > > If so, is it fine to have a pointer to it S390CcwMachineState? > > > > > > > > > > Why not? > > > > > > > > If it's owned by the sysbus and the object is not explicitly referenced > > > > for the pointer, it might be deallocated and then you'd have a dangling pointer. > > > > > > Why would it be deallocated ? > > > > That's beside the point, if you transfer ownership, you have no control over when > > the deallocation happens. > > It's going to be fine in practice, but I don't think you should rely on it. > > I think you could just do sysbus_realize instead of ..._and_unref, > > but like I said, I haven't fully understood qemu memory management. > > (It would also leak in a sense, but since the machine exists forever that should be fine) > > If I understand correctly: > > - qdev_new adds a reference count to the new created object, dev. > > - object_property_add_child adds a reference count to the child also > here the new created device dev so the ref count of dev is 2 . > > after the unref on dev, the ref count of dev get down to 1 > > then it seems OK. Did I miss something? The properties ref belongs to the property, if the property were removed, it would be unref'ed. There is no extra ref for the pointer in S390CcwMachineState. I'm coming from a clean code perspective, I don't think we'd run into this problem in practice. > > Regards, > Pierre > > > > > > as long it is not unrealized it belongs to the sysbus doesn't it? > > > > > > Regards, > > > Pierre > > > > > >
On 12/7/22 12:38, Janis Schoetterl-Glausch wrote: > * On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote: >> >> On 12/6/22 22:06, Janis Schoetterl-Glausch wrote: >>> On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote: >>>> >>>> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote: >>>>> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote: >>>>>> >>>>>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote: >>>>>>> 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 >>>>>>> > [...] > >>>>>>>> + 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? >>>>>> >>>>>> Yes it is so we see it on the qtree with its properties. >>>>>> >>>>>> >>>>>>> If so, is it fine to have a pointer to it S390CcwMachineState? >>>>>> >>>>>> Why not? >>>>> >>>>> If it's owned by the sysbus and the object is not explicitly referenced >>>>> for the pointer, it might be deallocated and then you'd have a dangling pointer. >>>> >>>> Why would it be deallocated ? >>> >>> That's beside the point, if you transfer ownership, you have no control over when >>> the deallocation happens. >>> It's going to be fine in practice, but I don't think you should rely on it. >>> I think you could just do sysbus_realize instead of ..._and_unref, >>> but like I said, I haven't fully understood qemu memory management. >>> (It would also leak in a sense, but since the machine exists forever that should be fine) >> >> If I understand correctly: >> >> - qdev_new adds a reference count to the new created object, dev. >> >> - object_property_add_child adds a reference count to the child also >> here the new created device dev so the ref count of dev is 2 . >> >> after the unref on dev, the ref count of dev get down to 1 >> >> then it seems OK. Did I miss something? > > The properties ref belongs to the property, if the property were removed, > it would be unref'ed. There is no extra ref for the pointer in S390CcwMachineState. > I'm coming from a clean code perspective, I don't think we'd run into this problem in practice. OK, I understand, you are right. My original code used object_resolve_path() to retrieve the object what made things cleaner I think. For performance reason, Cedric proposed during the review of V10 to add the pointer to the machine state instead. I must say that I am not very comfortable to argument on this. @Cedric what do you think? Regards, Pierre
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h new file mode 100644 index 0000000000..e88059ccec --- /dev/null +++ b/include/hw/s390x/cpu-topology.h @@ -0,0 +1,44 @@ +/* + * 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 + +#include "hw/qdev-core.h" +#include "qom/object.h" + +#define S390_TOPOLOGY_CPU_IFL 0x03 +#define S390_TOPOLOGY_MAX_ORIGIN ((63 + S390_MAX_CPUS) / 64) + +#define S390_TOPOLOGY_POLARITY_HORIZONTAL 0x00 +#define S390_TOPOLOGY_POLARITY_VERTICAL_LOW 0x01 +#define S390_TOPOLOGY_POLARITY_VERTICAL_MEDIUM 0x02 +#define S390_TOPOLOGY_POLARITY_VERTICAL_HIGH 0x03 + +typedef struct S390TopoSocket { + int active_count; + uint64_t mask[S390_TOPOLOGY_MAX_ORIGIN]; +} S390TopoSocket; + +struct S390Topology { + SysBusDevice parent_obj; + uint32_t num_cores; + uint32_t num_sockets; + S390TopoSocket *socket; +}; + +#define TYPE_S390_CPU_TOPOLOGY "s390-topology" +OBJECT_DECLARE_SIMPLE_TYPE(S390Topology, S390_CPU_TOPOLOGY) + +static inline bool s390_has_topology(void) +{ + return false; +} + +#endif 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; }; 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 @@ -0,0 +1,87 @@ +/* + * CPU Topology + * + * Copyright IBM Corp. 2022 + * 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" + +/** + * s390_topology_realize: + * @dev: the device state + * + * We free the socket array allocated in realize. + */ +static void s390_topology_unrealize(DeviceState *dev) +{ + S390Topology *topo = S390_CPU_TOPOLOGY(dev); + + g_free(topo->socket); +} + +/** + * s390_topology_realize: + * @dev: the device state + * @errp: the error pointer (not used) + * + * During realize the machine CPU topology is initialized with the + * QEMU -smp parameters. + * The maximum count of CPU TLE in the all Topology can not be greater + * than the maximum CPUs. + */ +static void s390_topology_realize(DeviceState *dev, Error **errp) +{ + S390Topology *topo = S390_CPU_TOPOLOGY(dev); + + topo->socket = g_new0(S390TopoSocket, topo->num_sockets); +} + +static Property s390_topology_properties[] = { + DEFINE_PROP_UINT32("num-cores", S390Topology, num_cores, 1), + DEFINE_PROP_UINT32("num-sockets", S390Topology, num_sockets, 1), + DEFINE_PROP_END_OF_LIST(), +}; + +/** + * topology_class_init: + * @oc: Object class + * @data: (not used) + * + * A very simple object we will need for reset and migration. + */ +static void topology_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = s390_topology_realize; + dc->unrealize = s390_topology_unrealize; + device_class_set_props(dc, s390_topology_properties); + set_bit(DEVICE_CATEGORY_MISC, dc->categories); +} + +static const TypeInfo cpu_topology_info = { + .name = TYPE_S390_CPU_TOPOLOGY, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(S390Topology), + .class_init = topology_class_init, +}; + +static void topology_register(void) +{ + type_register_static(&cpu_topology_info); +} +type_init(topology_register); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 2e64ffab45..973bbdd36e 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -44,6 +44,7 @@ #include "hw/s390x/pv.h" #include "migration/blocker.h" #include "qapi/visitor.h" +#include "hw/s390x/cpu-topology.h" static Error *pv_mig_blocker; @@ -102,6 +103,24 @@ static void s390_init_cpus(MachineState *machine) } } +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)); + 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); + + return dev; +} + static const char *const reset_dev_types[] = { TYPE_VIRTUAL_CSS_BRIDGE, "s390-sclp-event-facility", @@ -255,6 +274,12 @@ static void ccw_init(MachineState *machine) /* init CPUs (incl. CPU model) early so s390_has_feature() works */ s390_init_cpus(machine); + /* Need CPU model to be determined before we can set up topology */ + if (s390_has_topology()) { + S390_CCW_MACHINE(machine)->topology = s390_init_topology(machine, + &error_fatal); + } + /* Need CPU model to be determined before we can set up PV */ s390_pv_init(machine->cgs, &error_fatal); diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build index f291016fee..653f6ab488 100644 --- a/hw/s390x/meson.build +++ b/hw/s390x/meson.build @@ -2,6 +2,7 @@ s390x_ss = ss.source_set() s390x_ss.add(files( 'ap-bridge.c', 'ap-device.c', + 'cpu-topology.c', 'ccw-device.c', 'css-bridge.c', 'css.c',
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