[v3,06/33] add the vmstate description for device reset state

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

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/Makefile.objs  |  1 +
 hw/core/qdev-vmstate.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 hw/core/qdev-vmstate.c

On Mon, Jul 29, 2019 at 04:56:27PM +0200, Damien Hedde 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
> 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.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  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;

Having redundant copies of the resetting bit in the bridge and every
bus instance seems kind of bogus.

> +        bus->reset_is_cold = dev->reset_is_cold;
> +    }
> +    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()
> +    },
> +};

On 7/31/19 8:08 AM, David Gibson wrote:
> On Mon, Jul 29, 2019 at 04:56:27PM +0200, Damien Hedde 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
>> 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.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>  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;
> 
> Having redundant copies of the resetting bit in the bridge and every
> bus instance seems kind of bogus.

Currently we duplicate the resetting bit of parent into children when we
do the reset propagation into the tree. It means resetting count of an
device/bus contains the value of its parent plus any additional bit
local to the object (due to a reset from an gpio for example).

I'm not sure if we can avoid that. It would require the
"get_resetting_count" somehow to be recursive and fetch parent value and
so on. I need to work on it to know if it's really possible.

> 
>> +        bus->reset_is_cold = dev->reset_is_cold;
>> +    }
>> +    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()
>> +    },
>> +};
>

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

On Mon, 29 Jul 2019 at 15:58, Damien Hedde <damien.hedde@greensocs.com> 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
> 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.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>


> +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()
> +    },
> +};
> -

Forgot to ask -- why don't we migrate the hold_needed flags?

thanks
-- PMM

On 8/7/19 4:54 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde <damien.hedde@greensocs.com> 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
>> 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.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> 
>> +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()
>> +    },
>> +};
>> -
> 
> Forgot to ask -- why don't we migrate the hold_needed flags?

The flag is only used to keep the info between executing the init
and hold phases. We can't interrupt the code in between since this
mess is during resettable_assert_reset method which is atomic.
I can add a comment to explain that.

> 
> thanks
> -- PMM
>