[PATCH] hw/block/pflash_cfi01: Restore ROMD mode after migration

Bin Guo posted 1 patch 1 month, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260803041808.58174-1-guobin@linux.alibaba.com
Maintainers: "Philippe Mathieu-Daudé" <philmd@mailo.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
hw/block/pflash_cfi01.c | 10 ++++++++++
1 file changed, 10 insertions(+)
[PATCH] hw/block/pflash_cfi01: Restore ROMD mode after migration
Posted by Bin Guo 1 month, 3 weeks ago
pflash_post_load() did not restore the ROMD mode of the memory region.
Although cmd and wcycle are migrated, the destination retains the
default ROMD = true from realize.  When the source was in a non-array
mode (e.g. ID read, cmd = 0x90), reads on the destination bypass
pflash_read() via the ROM fast path and return raw storage bytes
instead of the command-specific response.

Derive ROMD from the migrated cmd/wcycle in pflash_post_load.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4042
Cc: qemu-stable@nongnu.org
Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 hw/block/pflash_cfi01.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 5b9ddb20b1..a13b91967e 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -1030,6 +1030,16 @@ static int pflash_post_load(void *opaque, int version_id)
 {
     PFlashCFI01 *pfl = opaque;
 
+    /*
+     * ROMD mode is not in the VMState; derive it from the migrated
+     * cmd and wcycle.  Only (wcycle == 0, cmd == 0x00) is read-array.
+     */
+    if (pfl->wcycle == 0 && pfl->cmd == 0x00) {
+        memory_region_rom_device_set_romd(&pfl->mem, true);
+    } else {
+        memory_region_rom_device_set_romd(&pfl->mem, false);
+    }
+
     if (!pfl->ro) {
         pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
                                                         pfl);
-- 
2.50.1 (Apple Git-155)
Re: [PATCH] hw/block/pflash_cfi01: Restore ROMD mode after migration
Posted by Peter Maydell 1 month, 3 weeks ago
On Mon, 3 Aug 2026 at 05:19, Bin Guo <guobin@linux.alibaba.com> wrote:
>
> pflash_post_load() did not restore the ROMD mode of the memory region.
> Although cmd and wcycle are migrated, the destination retains the
> default ROMD = true from realize.  When the source was in a non-array
> mode (e.g. ID read, cmd = 0x90), reads on the destination bypass
> pflash_read() via the ROM fast path and return raw storage bytes
> instead of the command-specific response.
>
> Derive ROMD from the migrated cmd/wcycle in pflash_post_load.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4042
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  hw/block/pflash_cfi01.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 5b9ddb20b1..a13b91967e 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -1030,6 +1030,16 @@ static int pflash_post_load(void *opaque, int version_id)
>  {
>      PFlashCFI01 *pfl = opaque;
>
> +    /*
> +     * ROMD mode is not in the VMState; derive it from the migrated
> +     * cmd and wcycle.  Only (wcycle == 0, cmd == 0x00) is read-array.
> +     */
> +    if (pfl->wcycle == 0 && pfl->cmd == 0x00) {
> +        memory_region_rom_device_set_romd(&pfl->mem, true);
> +    } else {
> +        memory_region_rom_device_set_romd(&pfl->mem, false);
> +    }

Confirming that this is correct is a bit tricky. It relies on:
 * when we set romd mode to true we also set wcycle = 0, cmd = 0
   (which we do, in reset and in the mode_read_array code)
 * when we set romd mode to false at the top of pflash_write(),
   all paths out of that function either go through the mode_read_array
   path, or else update pfl->cmd to something non-zero
 * nowhere outside pflash_write() udpates cmd or wcycle except
   for the "clear them to 0 and set romd mode" places

This is almost but not quite true. In pflash_read(), the default
case for the pfl->cmd switch sets wcycle = 0 cmd = 0 but doesn't
change the romd state. Luckily the "this should never happen"
comment is true -- there's no way to get a pfl->cmd that falls
into the default (except for being deliberately fed a bogus value
via inbound migration).

It might be worth having that pflash_read() code also set the
romd mode when it clears wcycle and cmd, to make the invariant
more clearly preserved.

But this patch is correct, so:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Re: [PATCH] hw/block/pflash_cfi01: Restore ROMD mode after migration
Posted by Philippe Mathieu-Daudé 1 month, 1 week ago
On 7/8/26 15:08, Peter Maydell wrote:
> On Mon, 3 Aug 2026 at 05:19, Bin Guo <guobin@linux.alibaba.com> wrote:
>>
>> pflash_post_load() did not restore the ROMD mode of the memory region.
>> Although cmd and wcycle are migrated, the destination retains the
>> default ROMD = true from realize.  When the source was in a non-array
>> mode (e.g. ID read, cmd = 0x90), reads on the destination bypass
>> pflash_read() via the ROM fast path and return raw storage bytes
>> instead of the command-specific response.
>>
>> Derive ROMD from the migrated cmd/wcycle in pflash_post_load.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4042
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
>> ---
>>   hw/block/pflash_cfi01.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 5b9ddb20b1..a13b91967e 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -1030,6 +1030,16 @@ static int pflash_post_load(void *opaque, int version_id)
>>   {
>>       PFlashCFI01 *pfl = opaque;
>>
>> +    /*
>> +     * ROMD mode is not in the VMState; derive it from the migrated
>> +     * cmd and wcycle.  Only (wcycle == 0, cmd == 0x00) is read-array.
>> +     */
>> +    if (pfl->wcycle == 0 && pfl->cmd == 0x00) {
>> +        memory_region_rom_device_set_romd(&pfl->mem, true);
>> +    } else {
>> +        memory_region_rom_device_set_romd(&pfl->mem, false);
>> +    }
> 
> Confirming that this is correct is a bit tricky. It relies on:
>   * when we set romd mode to true we also set wcycle = 0, cmd = 0
>     (which we do, in reset and in the mode_read_array code)
>   * when we set romd mode to false at the top of pflash_write(),
>     all paths out of that function either go through the mode_read_array
>     path, or else update pfl->cmd to something non-zero
>   * nowhere outside pflash_write() udpates cmd or wcycle except
>     for the "clear them to 0 and set romd mode" places
> 
> This is almost but not quite true. In pflash_read(), the default
> case for the pfl->cmd switch sets wcycle = 0 cmd = 0 but doesn't
> change the romd state. Luckily the "this should never happen"
> comment is true -- there's no way to get a pfl->cmd that falls
> into the default (except for being deliberately fed a bogus value
> via inbound migration).

Queued amending this audit note ^^, thanks.

> 
> It might be worth having that pflash_read() code also set the
> romd mode when it clears wcycle and cmd, to make the invariant
> more clearly preserved.
> 
> But this patch is correct, so:
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
>