On Mon, 19 Oct 2026 at 14:16, Peter Maydell <[email protected]> wrote: > > The VMSTATE_PTIMER macro is defined to pass a 'version' argument of 1 > to VMSTATE_STRUCT_POINTER_TEST_V. This means that the field will > only be used in migration when the vmstate struct that uses > VMSTATE_PTIMER has a version_id of at least 1. This doesn't match > how we define other VMSTATE_FOO which don't take an explicit > version_id, which is to use a version argument of 0 (e.g. all the > common macros like VMSTATE_UINT32 are written to pass 0 to their > VMSTATE_UINT32_V counterparts, and so on). It also doesn't match > how we define VMSTATE_PTIMER_ARRAY. > > The practical effect of this is that users of VMSTATE_PTIMER in > vmstate structs which explicitly or implicitly have a version_id of 0 > were actually not getting their ptimers migrated. > > This affects: > vmstate_xlnx_csu_dma in wh/dma/xlnx_csu_dma.c > vmstate_k230_wdt in hw/watchdog/k230_wdt.c > vmstate_imx2_wdt in hw/watchdog/wdt_imx2.c > > which are used in: > * Arm imx-based machines > * Arm Xilinx versal and zynq machines > * RISC-V K230 machine > > Most ptimer users are unaffected because they took the usual approach > of explicitly setting their starting version_id to 1. > > Fix the version_id in this field. This is a migration compatibility > break for all the above listed devices/machines. Since the timers > involved are in devices where the timer is usually stopped (e.g. the > watchdogs only have the timer active when the watchdog is enabled and > counting down), the consequences of the bug are likely not visible to > most users. > > We could perhaps attempt to preserve migration compatibility here, > but none of the affected machine types are versioned and so we do not > promise cross-version compatibility, so do the simple thing. > > Fixes: 852f771ec9e5c ("vmstate: port pmtimer") > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4705 > Signed-off-by: Peter Maydell <[email protected]> > --- > Not cc: stable because it's a migration format change and we > don't generally backport those. > > As an aside, is the expected way to write a new vmstate to give it > an explicit version_id of 1? If so, maybe we should look at the > vmstate structs which don't define one and so get the default 0. > Or is that just some odd quirk I've picked up because lots of > existing code uses 1 as its first version ID value? > > --- > include/hw/core/ptimer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/core/ptimer.h b/include/hw/core/ptimer.h > index 4dc02b0de47..8513f8917e3 100644 > --- a/include/hw/core/ptimer.h > +++ b/include/hw/core/ptimer.h > @@ -300,7 +300,7 @@ void ptimer_stop(ptimer_state *s); > extern const VMStateDescription vmstate_ptimer; > > #define VMSTATE_PTIMER(_field, _state) \ > - VMSTATE_STRUCT_POINTER_V(_field, _state, 1, vmstate_ptimer, ptimer_state) > + VMSTATE_STRUCT_POINTER_V(_field, _state, 0, vmstate_ptimer, ptimer_state) > > #define VMSTATE_PTIMER_ARRAY(_f, _s, _n) \ > VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(_f, _s, _n, 0, \
Followup query: should we also bump the version_id on the three affected vmstate structs, to make it clearer that the migration compat break has happened? Or does it not matter? thanks -- PMM
