Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices

[Dr. David Alan Gilbert]

* Peter Maydell (address@hidden) wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <address@hidden> 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 <address@hidden>
> > ---
> >  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 / address@hidden / Manchester, UK