Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration

[Markus Armbruster <armbru@redhat.com>]

I'm afraid this is *not* trivial.  Migration stuff rarely is :)

Johny Mattsson <jmattsson@dius.com.au> writes:

> When using snapshots on e.g. the VersatilePB board, the pflash driver
> would report an error and get reset at times after resuming the snapshot.
> This was traced down to the 'romd' flag in the pflash emulation which was
> not being persisted in the snapshot.
>
> Author: Bernd Meyer <bmeyer@dius.com.au>
> Signed-off-by: Johny Mattsson <jmattsson@dius.com.au>
>
> ---
>  hw/block/pflash_cfi01.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index f9507b4..83cb1d3 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -80,6 +80,7 @@ struct pflash_t {
>      int ro;
>      uint8_t cmd;
>      uint8_t status;
> +    uint8_t romd;
>      uint16_t ident0;
>      uint16_t ident1;
>      uint16_t ident2;
> @@ -94,14 +95,25 @@ struct pflash_t {
>      void *storage;
>  };
>
> +static int sync_romd(void* opaque, int version_id)
> +{
> +  (void)version_id;
> +  pflash_t *pfl = opaque;
> +
> +  memory_region_rom_device_set_romd(&pfl->mem,!!pfl->romd);
> +  return 0;
> +}
> +
>  static const VMStateDescription vmstate_pflash = {
>      .name = "pflash_cfi01",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = sync_romd,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(wcycle, pflash_t),
>          VMSTATE_UINT8(cmd, pflash_t),
>          VMSTATE_UINT8(status, pflash_t),
> +        VMSTATE_UINT8(romd, pflash_t),
>          VMSTATE_UINT64(counter, pflash_t),
>          VMSTATE_END_OF_LIST()
>      }

Doesn't this break migration to/from unfixed versions?

> @@ -114,6 +126,7 @@ static void pflash_timer (void *opaque)
>      DPRINTF("%s: command %02x done\n", __func__, pfl->cmd);
>      /* Reset flash */
>      pfl->status ^= 0x80;
> +    pfl->romd=1;
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
> @@ -453,6 +466,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>
>      if (!pfl->wcycle) {
>          /* Set the device in I/O access mode */
> +        pfl->romd=0;
>          memory_region_rom_device_set_romd(&pfl->mem, false);
>      }
>
> @@ -638,6 +652,7 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
>   reset_flash:
> +    pfl->romd=1;
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>
>      pfl->wcycle = 0;
> @@ -804,6 +819,7 @@ static void pflash_cfi01_realize(DeviceState *dev,
> Error **errp)
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> +    pfl->romd = 1;
>      /* Hardcoded CFI table */
>      pfl->cfi_len = 0x52;
>      /* Standard "QRY" string */
> -- 
> 2.0.0

I suspect you need a subsection.  Initialize ->romd = 1, then let the
subsection override it.