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

[Damien Hedde]

On 8/7/19 5:07 PM, Peter Maydell 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.

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

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
>