Re: [PATCH] hw/block: Add migration support for pflash_cfi02.c

[Om]

On 27/08/26 15:38, Peter Maydell wrote:
On Sat, 22 Aug 2026 at 19:39, Om Barkare <ombarkare...@gmail.com> wrote:
From: OmBarkare <ombarkare...@gmail.com>

Device did not have migration support, which would result in its
internal state being lost during migration or saves.

Add VMStateDescription to serialize state and set memory regions
romd_mode to rom_mode field in post load hook

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4157

Signed-off-by: Om Barkare <ombarkare...@gmail.com>
---
  hw/block/pflash_cfi02.c | 73 ++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 72 insertions(+), 1 deletion(-)
Thanks for this patch; I have a few comments below, but mostly
this looks good.

We could mention in the commit message that the only boards
using pflash_cfi02 are the sh4 r2d and the arm canon-a1100,
musicpal and xilinx-zynq-a9.
Will mention that

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6f952fe7de..82965e6cdc 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,6 +46,7 @@
  #include "qemu/module.h"
  #include "hw/core/sysbus.h"
  #include "migration/vmstate.h"
+#include "system/runstate.h"
  #include "trace.h"

  #define PFLASH_LAZY_ROMD_THRESHOLD 42
@@ -71,7 +72,7 @@ struct PFlashCFI02 {
      BlockBackend *blk;
      uint32_t uniform_nb_blocs;
      uint32_t uniform_sector_len;
-    uint32_t total_sectors;
+    int32_t total_sectors;
I think it's worth mentioning in the commit message that we
have to change total_sectors from uint32_t to int32_t to
satisfy the VMSTATE_BITMAP macro, but that this is OK because
it's a value we calculate based on the size of the flash,
and it's never going to be large enough to overflow an int32_t.
Will mention as this was something that was changed :)
      uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
      uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
      uint32_t chip_len;
@@ -107,6 +108,50 @@ struct PFlashCFI02 {
      unsigned long *sector_erase_map;
      char *name;
      void *storage;
+    VMChangeStateEntry *vmstate;
+};
+
+static int pflash_post_load(void *opaque, int version_id);
+
+static bool pflash_sector_erase_needed(void *opaque)
+{
+    PFlashCFI02 *pfl = opaque;
+
+    return (pfl->sectors_to_erase > 0);
+}
+
+static const VMStateDescription vmstate_pflash_sector_erase = {
+    .name = "pflash_cfi02_erase_sector",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pflash_sector_erase_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_INT32(sectors_to_erase, PFlashCFI02),
+        VMSTATE_UINT64(erase_time_remaining, PFlashCFI02),
+        VMSTATE_BITMAP(sector_erase_map, PFlashCFI02, 1, total_sectors),
+        VMSTATE_END_OF_LIST()
+    }
+
+};
Why did you choose to put this in a subsection ?
These states are accessed only if an erase is ongoing or if it is pending
which can happen if migration is performed mid-erase or when an erase
is suspended. As they are only accessed during an erase, so I thought I 
would
put them in a subsection
+
+static const VMStateDescription vmstate_pflash = {
+    .name = "pflash_cfi02",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = pflash_post_load,
+    .fields = (const VMStateField[]) {
+        VMSTATE_INT32(wcycle, PFlashCFI02),
+        VMSTATE_UINT8(cmd, PFlashCFI02),
+        VMSTATE_UINT8(status, PFlashCFI02),
+        VMSTATE_INT32(read_counter, PFlashCFI02),
+        VMSTATE_BOOL(rom_mode, PFlashCFI02),
+        VMSTATE_TIMER(timer, PFlashCFI02),
You've missed out "bypass", which is also state that the guest
can cause the device to change at runtime.
oops, will add that


Thanks for the review