hw/block/pflash_cfi02.c | 73 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-)
From: OmBarkare <ombarkare123@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 <ombarkare123@gmail.com>
---
hw/block/pflash_cfi02.c | 73 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
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;
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()
+ }
+
+};
+
+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),
+ VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * const []) {
+ &vmstate_pflash_sector_erase,
+ NULL
+ }
};
/*
@@ -976,6 +1021,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, const void *data)
device_class_set_legacy_reset(dc, pflash_cfi02_reset);
dc->unrealize = pflash_cfi02_unrealize;
device_class_set_props(dc, pflash_cfi02_properties);
+ dc->vmsd = &vmstate_pflash;
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
}
@@ -1028,3 +1074,28 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
return PFLASH_CFI02(dev);
}
+
+static void postload_update_cb(void *opaque, bool running, RunState state)
+{
+ PFlashCFI02 *pfl = opaque;
+
+ /* This is called after bdrv_activate_all. */
+ qemu_del_vm_change_state_handler(pfl->vmstate);
+ pfl->vmstate = NULL;
+
+ trace_pflash_postload_cb(pfl->name);
+ pflash_update(pfl, 0, pfl->chip_len);
+}
+
+static int pflash_post_load(void *opaque, int version_id)
+{
+ PFlashCFI02 *pfl = opaque;
+
+ if (!pfl->ro) {
+ pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb, pfl);
+ }
+
+ memory_region_rom_device_set_romd(&pfl->orig_mem, pfl->rom_mode);
+
+ return 0;
+}
--
2.55.0On Sat, 22 Aug 2026 at 19:39, Om Barkare <ombarkare123@gmail.com> wrote:
>
> From: OmBarkare <ombarkare123@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 <ombarkare123@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.
> 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.
> 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 ?
> +
> +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.
> + VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription * const []) {
> + &vmstate_pflash_sector_erase,
> + NULL
> + }
> };
thanks
-- PMM
On 27/08/26 15:38, Peter Maydell wrote:
> On Sat, 22 Aug 2026 at 19:39, Om Barkare <ombarkare123@gmail.com> wrote:
>> From: OmBarkare <ombarkare123@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 <ombarkare123@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
On Thu, 27 Aug 2026 at 17:12, Om <ombarkare123@gmail.com> wrote:
>
>
> On 27/08/26 15:38, Peter Maydell wrote:
> > On Sat, 22 Aug 2026 at 19:39, Om Barkare <ombarkare123@gmail.com> wrote:
> >> +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
Putting them into a subsection means extra code complexity here,
and a little bit of extra overhead in the on-the-wire format.
We usually only put things in a subsection if we need to do
that for migration compatibility (e.g. we added something to the
migration state later and so we want to avoid transmitting the
extra thing unless we really need to, so that we can still work
with an old QEMU that doesn't expect it). In this case the bitmap
is 1 bit per sector, and e.g. xilinx_zynq has a 128K sector size
and 64MB total size, for 512 sectors. That's only 64 bytes for the
bitmap, which is too small to be worth worrying about not sending.
So I think we should just include these fields directly in the
main vmstate.
-- PMM
On 27/08/26 23:09, Peter Maydell wrote:
> On Thu, 27 Aug 2026 at 17:12, Om <ombarkare123@gmail.com> wrote:
>>
>> On 27/08/26 15:38, Peter Maydell wrote:
>>> On Sat, 22 Aug 2026 at 19:39, Om Barkare <ombarkare123@gmail.com> wrote:
>>>> +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
> Putting them into a subsection means extra code complexity here,
> and a little bit of extra overhead in the on-the-wire format.
> We usually only put things in a subsection if we need to do
> that for migration compatibility (e.g. we added something to the
> migration state later and so we want to avoid transmitting the
> extra thing unless we really need to, so that we can still work
> with an old QEMU that doesn't expect it). In this case the bitmap
> is 1 bit per sector, and e.g. xilinx_zynq has a 128K sector size
> and 64MB total size, for 512 sectors. That's only 64 bytes for the
> bitmap, which is too small to be worth worrying about not sending.
>
> So I think we should just include these fields directly in the
> main vmstate.
>
> -- PMM
Thanks for the review
Will make the changes and submit the patch
© 2016 - 2026 Red Hat, Inc.