[v3,07/33] automatically add vmstate for reset support in devices

This add the reset related sections for every QOM
device.

Signed-off-by: Damien Hedde <damien.hedde@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

On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.hedde@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

On 8/7/19 5:07 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.

Sorry for that, thought I've added some...

I've kept this patch separate from previous one because this it is
awkward. I'm not sure this is the right place (I mean in qdev files) do
this kind of stuff. It basically replaces every static vmsd by dynamic
ones, so it makes it harder to follow when debugging since there is no
symbol associated to them. But on the other hand, I don't see an
alternative.

I copy there what I've put in the cover-letter:
For devices, I've added a patch to automate the addition of reset
related subsection. In consequence it is not necessary to explicitly add
the reset subsection in every device specialization requiring it.
Right know this is kind of a hack into qdev to dynamically modify the
vmsd before the registration. There is probably a much cleaner way to do
this but I prefered to demonstrate it by keeping modification local to qdev.
As far as I can tell it's ok to dynamically add subsections at the end.
This does not prevent from further adding subsections in the orignal
vmsd: the subsections order in the array is irrelevant from migration
point-of-view. The loading part just lookup each subsection in the array
by name uppon reception.

> 
> I've added Dave Gilbert to the already long cc list since this
> is migration related.
> 
>> Signed-off-by: Damien Hedde <damien.hedde@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.

Right. I'll handle this along with the existing vmsd_unregister
in realize/unrealize method.

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

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.hedde@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 / dgilbert@redhat.com / Manchester, UK

On Thu, 8 Aug 2019 at 16:42, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.

Yeah, I'm not a huge fan of it either, but it does mean that
the state gets migrated and we don't have a pile of really
easy to miss bugs where we forgot to say "this device needs to
migrate the reset state" and it means we don't have every
device we ever write in the future needing to say "this device
needs to migrate reset state"...

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

One problem is that as soon as somebody writes a USB controller
or PCI controller model that can be held in reset, every
device that could sat behind it automatically now could find
that it's migrated while it's in reset.

thanks
-- PMM

On 8/9/19 12:07 PM, Peter Maydell wrote:
> On Thu, 8 Aug 2019 at 16:42, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>>
>> * Peter Maydell (peter.maydell@linaro.org) wrote:
>>> On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.
> 
> Yeah, I'm not a huge fan of it either, but it does mean that
> the state gets migrated and we don't have a pile of really
> easy to miss bugs where we forgot to say "this device needs to
> migrate the reset state" and it means we don't have every
> device we ever write in the future needing to say "this device
> needs to migrate reset state"...
> 
>> 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).
> 
> One problem is that as soon as somebody writes a USB controller
> or PCI controller model that can be held in reset, every
> device that could sat behind it automatically now could find
> that it's migrated while it's in reset.
> 

One way to keep the feature without copy-pasting vmsd would be to add
a new vmstate_register with an additional argument to pass the base
class vmsd section and handle the whole thing there.

--
Damien

On Fri, 9 Aug 2019 at 11:29, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> One way to keep the feature without copy-pasting vmsd would be to add
> a new vmstate_register with an additional argument to pass the base
> class vmsd section and handle the whole thing there.

If we have a vmstate section which contains no actual data,
only subsections with 'needed' functions, is it migration
compatible with previous versions in the same way that
tacking a subsection onto an existing function is?

thanks
-- PMM

On 8/9/19 12:32 PM, Peter Maydell wrote:
> On Fri, 9 Aug 2019 at 11:29, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> One way to keep the feature without copy-pasting vmsd would be to add
>> a new vmstate_register with an additional argument to pass the base
>> class vmsd section and handle the whole thing there.
> 
> If we have a vmstate section which contains no actual data,
> only subsections with 'needed' functions, is it migration
> compatible with previous versions in the same way that
> tacking a subsection onto an existing function is?

I don't think so because of the naming schema. I had to forge the
correct name for the reset subsection for every device.
Each subsection must be named after its parent section plus '/something'.

--
Damien

Peter Maydell <peter.maydell@linaro.org> wrote:
> On Fri, 9 Aug 2019 at 11:29, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> One way to keep the feature without copy-pasting vmsd would be to add
>> a new vmstate_register with an additional argument to pass the base
>> class vmsd section and handle the whole thing there.
>
> If we have a vmstate section which contains no actual data,
> only subsections with 'needed' functions, is it migration
> compatible with previous versions in the same way that
> tacking a subsection onto an existing function is?

Not now, but we can make it happen.

Right now, we first send the VMSD "header", and then we sent whatever
data is in it.

We can it make work the way you want.  Not sure if that is the best way,
though.  Sections that can(or can't) be there make just reasoning about
the stream more difficult.

Later, Juan.

Damien Hedde <damien.hedde@greensocs.com> wrote:
> On 8/9/19 12:32 PM, Peter Maydell wrote:
>> On Fri, 9 Aug 2019 at 11:29, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>
>>> One way to keep the feature without copy-pasting vmsd would be to add
>>> a new vmstate_register with an additional argument to pass the base
>>> class vmsd section and handle the whole thing there.
>> 
>> If we have a vmstate section which contains no actual data,
>> only subsections with 'needed' functions, is it migration
>> compatible with previous versions in the same way that
>> tacking a subsection onto an existing function is?
>
> I don't think so because of the naming schema. I had to forge the
> correct name for the reset subsection for every device.
> Each subsection must be named after its parent section plus '/something'.

That bit is easy.  You jsut named it: "parent_name/subsection_name", no?

Later, Juan.

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 8 Aug 2019 at 16:42, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > On Mon, 29 Jul 2019 at 15:59, Damien Hedde <damien.hedde@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.
> 
> Yeah, I'm not a huge fan of it either, but it does mean that
> the state gets migrated and we don't have a pile of really
> easy to miss bugs where we forgot to say "this device needs to
> migrate the reset state" and it means we don't have every
> device we ever write in the future needing to say "this device
> needs to migrate reset state"...
> 
> > 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).
> 
> One problem is that as soon as somebody writes a USB controller
> or PCI controller model that can be held in reset, every
> device that could sat behind it automatically now could find
> that it's migrated while it's in reset.

I'm not convinced though that they'll all be fixed by adding this
subsection; I think some of those devices you're going to have to
look at and see what they do.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK