* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.he...@greensocs.com> wrote:
> >
> > This add the reset related sections for every QOM
> > device.
>
> A bit more detail in the commit message would help, I think --
> this is adding extra machinery which has to copy and modify
> the VMStateDescription passed in by the device in order to
> add the subsection that handles reset.
>
> I've added Dave Gilbert to the already long cc list since this
> is migration related.
I don't like dynamically modifying all the vmsds.
Aren't you going to have to understand each devices reset behaviour
and make sure it does something sane? e.g. it might have a postload
that registers a timer or something that you wouldn't want to do if it's
in reset.
The easiest way is to write a macro that you can easily add to devices
you've checked subsection list (like the way we have a
VMSTATE_USB_DEVICE).
Dave
>
> > Signed-off-by: Damien Hedde <damien.he...@greensocs.com>
> > ---
> > hw/core/qdev-vmstate.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > hw/core/qdev.c | 12 +++++++++++-
> > include/hw/qdev-core.h | 3 +++
> > stubs/Makefile.objs | 1 +
> > stubs/device.c | 7 +++++++
> > 5 files changed, 63 insertions(+), 1 deletion(-)
> > create mode 100644 stubs/device.c
> >
> > diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> > index 07b010811f..24f8465c61 100644
> > --- a/hw/core/qdev-vmstate.c
> > +++ b/hw/core/qdev-vmstate.c
> > @@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
> > VMSTATE_END_OF_LIST()
> > },
> > };
> > +
> > +static VMStateDescription *vmsd_duplicate_and_append(
> > + const VMStateDescription *old_vmsd,
> > + const VMStateDescription *new_subsection)
> > +{
> > + VMStateDescription *vmsd;
> > + int n = 0;
> > +
> > + assert(old_vmsd && new_subsection);
> > +
> > + vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
> > +
> > + if (old_vmsd->subsections) {
> > + while (old_vmsd->subsections[n]) {
> > + n += 1;
> > + }
> > + }
> > + vmsd->subsections = g_new(const VMStateDescription *, n + 2);
> > +
> > + if (old_vmsd->subsections) {
> > + memcpy(vmsd->subsections, old_vmsd->subsections,
> > + sizeof(VMStateDescription *) * n);
> > + }
> > + vmsd->subsections[n] = new_subsection;
> > + vmsd->subsections[n + 1] = NULL;
> > +
> > + return vmsd;
> > +}
> > +
> > +void device_class_build_extended_vmsd(DeviceClass *dc)
> > +{
> > + assert(dc->vmsd);
> > + assert(!dc->vmsd_ext);
> > +
> > + /* forge a subsection with proper name */
> > + VMStateDescription *reset;
> > + reset = g_memdup(&device_vmstate_reset, sizeof(*reset));
> > + reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
> > +
> > + dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
> > +}
>
> This will allocate memory, but there is no corresponding
> code which frees it. This means you'll have a memory leak
> across device realize->unrealize for hotplug devices.
>
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index e9e5f2d5f9..88387d3743 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
> > const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
> > {
> > DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > - return dc->vmsd;
> > +
> > + if (!dc->vmsd) {
> > + return NULL;
> > + }
> > +
> > + if (!dc->vmsd_ext) {
> > + /* build it first time we need it */
> > + device_class_build_extended_vmsd(dc);
> > + }
> > +
> > + return dc->vmsd_ext;
> > }
>
> Unfortunately not everything that wants the VMSD calls
> this function. migration/savevm.c:dump_vmstate_json_to_file()
> does a direct access to dc->vmsd, so we need to fix that first.
>
> Devices which don't use dc->vmsd won't get this and so
> their reset state won't be migrated. That's OK for anything
> that's still not yet a QOM device, I guess -- it's not possible
> for them to be in a 'held in reset' state anyway, so the
> extra subsection would never be needed.
>
> The one I'm less sure about is the 'virtio' devices, which
> have to do something odd with migration state for backwards
> compat reasons. At the moment they can't be in a situation
> where they're being held in reset when we do a migration,
> but since they're PCI devices they might in future be possible
> to put into new boards/pci controllers that would let them
> be in that situation.
>
> > static void bus_remove_child(BusState *bus, DeviceState *child)
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 1670ae41bb..926d4bbcb1 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -120,6 +120,7 @@ typedef struct DeviceClass {
> >
> > /* device state */
> > const struct VMStateDescription *vmsd;
> > + const struct VMStateDescription *vmsd_ext;
> >
> > /* Private to qdev / bus. */
> > const char *bus_type;
> > @@ -520,6 +521,8 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
> >
> > const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
> >
> > +void device_class_build_extended_vmsd(DeviceClass *dc);
> > +
> > const char *qdev_fw_name(DeviceState *dev);
> >
> > Object *qdev_get_machine(void);
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 9c7393b08c..432b56f290 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -40,4 +40,5 @@ stub-obj-y += pci-host-piix.o
> > stub-obj-y += ram-block.o
> > stub-obj-y += ramfb.o
> > stub-obj-y += fw_cfg.o
> > +stub-obj-y += device.o
> > stub-obj-$(CONFIG_SOFTMMU) += semihost.o
> > diff --git a/stubs/device.c b/stubs/device.c
> > new file mode 100644
> > index 0000000000..e9b4f57e5f
> > --- /dev/null
> > +++ b/stubs/device.c
> > @@ -0,0 +1,7 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev-core.h"
> > +
> > +void device_class_build_extended_vmsd(DeviceClass *dc)
> > +{
> > + return;
> > +}
> > --
> > 2.22.0
>
>
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
