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

[Peter Maydell]

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.

> 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