Message ID | 20190729145654.14644-8-damien.hedde@greensocs.com |
---|---|
State | New |
Headers | show |
Series | Multi-phase reset mechanism | expand |
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
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); +} 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; } 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; +}
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