On Sat, 22 Aug 2026 at 19:39, Om Barkare <[email protected]> wrote:
>
> From: OmBarkare <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
