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. > 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