* [Qemu-trivial] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
@ 2014-08-21 7:35 Johny Mattsson
2014-08-21 9:22 ` [Qemu-trivial] [Qemu-devel] " Peter Maydell
2014-08-21 9:48 ` [Qemu-trivial] [Qemu-devel] " Markus Armbruster
0 siblings, 2 replies; 6+ messages in thread
From: Johny Mattsson @ 2014-08-21 7:35 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Kevin Wolf, Stefan Hajnoczi
[-- 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 --]
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
2014-08-21 7:35 [Qemu-trivial] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration Johny Mattsson
@ 2014-08-21 9:22 ` Peter Maydell
2014-08-21 9:58 ` [Qemu-trivial] " Paolo Bonzini
2014-08-21 9:48 ` [Qemu-trivial] [Qemu-devel] " Markus Armbruster
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2014-08-21 9:22 UTC (permalink / raw)
To: Johny Mattsson
Cc: QEMU Trivial, Kevin Wolf, QEMU Developers, Stefan Hajnoczi,
Paolo Bonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-trivial] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
2014-08-21 9:22 ` [Qemu-trivial] [Qemu-devel] " Peter Maydell
@ 2014-08-21 9:58 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-08-21 9:58 UTC (permalink / raw)
To: Peter Maydell, Johny Mattsson
Cc: QEMU Trivial, Kevin Wolf, QEMU Developers, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
2014-08-21 7:35 [Qemu-trivial] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration Johny Mattsson
2014-08-21 9:22 ` [Qemu-trivial] [Qemu-devel] " Peter Maydell
@ 2014-08-21 9:48 ` Markus Armbruster
2014-08-22 7:53 ` Johny Mattsson
1 sibling, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-08-21 9:48 UTC (permalink / raw)
To: Johny Mattsson; +Cc: qemu-trivial, Kevin Wolf, qemu-devel, Stefan Hajnoczi
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
2014-08-21 9:48 ` [Qemu-trivial] [Qemu-devel] " Markus Armbruster
@ 2014-08-22 7:53 ` Johny Mattsson
2014-08-22 9:12 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Johny Mattsson @ 2014-08-22 7:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-trivial, Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration
2014-08-22 7:53 ` Johny Mattsson
@ 2014-08-22 9:12 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2014-08-22 9:12 UTC (permalink / raw)
To: Johny Mattsson; +Cc: qemu-trivial, Kevin Wolf, qemu-devel, Stefan Hajnoczi
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-22 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-21 7:35 [Qemu-trivial] [PATCH] Fix pflash_cfi01 to restore flash command/array state after migration Johny Mattsson
2014-08-21 9:22 ` [Qemu-trivial] [Qemu-devel] " Peter Maydell
2014-08-21 9:58 ` [Qemu-trivial] " Paolo Bonzini
2014-08-21 9:48 ` [Qemu-trivial] [Qemu-devel] " Markus Armbruster
2014-08-22 7:53 ` Johny Mattsson
2014-08-22 9:12 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).