Re: [Qemu-devel] [PATCH v3 06/33] add the vmstate description for device reset state

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde <address@hidden> wrote:
>
> It contains the resetting counter and cold flag status.
>
> At this point, migration of bus reset related state (counter and cold/warm
> flag) is handled by parent device. This done using the post_load

"is done"

> function in the vmsd subsection.
>
> This is last point allow to add an initial support of migration with part of
> qdev/qbus tree in reset state under the following condition:
> + time-lasting reset are asserted on Device only
>
> Note that if this condition is not respected, migration will succeed and
> no failure will occurs. The only impact is that the resetting counter
> of a bus may lower afer a migration.

We should just migrate the bus state correctly -- see below.

> Signed-off-by: Damien Hedde <address@hidden>
> ---
>  hw/core/Makefile.objs  |  1 +
>  hw/core/qdev-vmstate.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 hw/core/qdev-vmstate.c
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index d9234aa98a..49e9be0228 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
>  common-obj-y += resettable.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> +common-obj-$(CONFIG_SOFTMMU) += qdev-vmstate.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> new file mode 100644
> index 0000000000..07b010811f
> --- /dev/null
> +++ b/hw/core/qdev-vmstate.c
> @@ -0,0 +1,45 @@
> +/*
> + * Device vmstate
> + *
> + * Copyright (c) 2019 GreenSocs
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +#include "migration/vmstate.h"
> +
> +static bool device_vmstate_reset_needed(void *opaque)
> +{
> +    DeviceState *dev = (DeviceState *) opaque;
> +    return dev->resetting != 0;
> +}
> +
> +static int device_vmstate_reset_post_load(void *opaque, int version_id)
> +{
> +    DeviceState *dev = (DeviceState *) opaque;
> +    BusState *bus;
> +    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +        bus->resetting = dev->resetting;
> +        bus->reset_is_cold = dev->reset_is_cold;
> +    }

Bus reset state might not be the same as the parent device's
reset state, so we need to migrate them both separately.

The way to do this is that in a pre-save hook we iterate
through the child buses and capture their state into
an array. Then we can migrate the array. In the post-load
hook we can set the bus state fields from the array contents.
VMSTATE_WITH_TMP is useful for this kind of situation.

One thing I'm slightly wary of here is that this will mean
that the ordering of child buses within the child_bus
array becomes significant to avoid migration compat breaks.
I think this is OK, though.

> +    return 0;
> +}
> +
> +const struct VMStateDescription device_vmstate_reset = {
> +    .name = "device_reset",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = device_vmstate_reset_needed,
> +    .post_load = device_vmstate_reset_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(resetting, DeviceState),
> +        VMSTATE_BOOL(reset_is_cold, DeviceState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};

This isn't used -- I think you should squash patch 7 in
with this one.

thanks
-- PMM