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

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

[Johny Mattsson]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

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()
     }
@@ -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


-- 
Johny Mattsson
Senior Software Engineer

DiUS Computing Pty. Ltd.

*where ideas are engineered *

[-- Attachment #2: Type: text/html, Size: 3799 bytes --]

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

[Peter Maydell]

On 21 August 2014 08:35, Johny Mattsson <jmattsson@dius.com.au> wrote:
> 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>

Paolo -- remind me, is it the device model's job to keep track of
what its memory regions are doing, rather than the memory region's
job to make sure it's migrated? I definitely remember having this
argument with Avi before but I can't remember the outcome...

> ---
>  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()

Isn't this a migration format change in a device that's in
the PC model? That probably needs thought to avoid
migration-compatibility breaks...

>      }
> @@ -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


thanks
-- PMM

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

[Markus Armbruster]

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.

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

[Paolo Bonzini]

Il 21/08/2014 11:22, Peter Maydell ha scritto:
> On 21 August 2014 08:35, Johny Mattsson <jmattsson@dius.com.au> wrote:
>> > 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>
> Paolo -- remind me, is it the device model's job to keep track of
> what its memory regions are doing, rather than the memory region's
> job to make sure it's migrated? I definitely remember having this
> argument with Avi before but I can't remember the outcome...
> 

It's the device model's job.  I agree with your review, and I think
romd=1 is the right default value if the subsection is not present.

Paolo

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

[Johny Mattsson]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On 21 August 2014 19:48, Markus Armbruster <armbru@redhat.com> wrote:
>
> Doesn't this break migration to/from unfixed versions?
>

It might. It is not something we've tried here, and I'm certainly not
familiar enough with QEMU internals myself to say one way or another. We
encountered this issue in what I can best describe as a short-and-furious
proof-of-concept project, and as part of the wrapping up of this I'm going
over our change logs to see if anything can be contributed back upstream.
It seemed like a trivial fix in the tree, but I acknowledge it may have
ramifications we didn't consider.


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

And here I'm well and truly out of my depth! If someone who actually knows
what they're doing would like to run with this, please do.

Sorry to effectively do a dump-and-run here - I hope its a net positive at
least.

Regards,
/Johny
-- 
Johny Mattsson
Senior Software Engineer

DiUS Computing Pty. Ltd.

*where ideas are engineered *

[-- Attachment #2: Type: text/html, Size: 1954 bytes --]

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

[Markus Armbruster]

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

> On 21 August 2014 19:48, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Doesn't this break migration to/from unfixed versions?
>>
>
> It might. It is not something we've tried here, and I'm certainly not
> familiar enough with QEMU internals myself to say one way or another. We
> encountered this issue in what I can best describe as a short-and-furious
> proof-of-concept project, and as part of the wrapping up of this I'm going
> over our change logs to see if anything can be contributed back upstream.
> It seemed like a trivial fix in the tree, but I acknowledge it may have
> ramifications we didn't consider.
>
>
>> I suspect you need a subsection.  Initialize ->romd = 1, then let the
>> subsection override it.
>>
>
> And here I'm well and truly out of my depth! If someone who actually knows
> what they're doing would like to run with this, please do.
>
> Sorry to effectively do a dump-and-run here - I hope its a net positive at
> least.

Sure it is!  Imperfect patches can be a rather nice kind of bug report.