Peter Maydell <[email protected]> writes: > Ping for review from the migration folks on this one, and especially > on the followup query at the bottom about version_ids in the > vmstates using this macro. > > thanks > -- PMM > > On Mon, 19 Oct 2026 at 15:58, Peter Maydell <[email protected]> wrote: >> >> 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?
Well, the version difference is compatibility mechanism, so I don't think there can be any prescription on which version to use. The vmsd version_id should match or be greater than the field->version_id for the fields contained in that version. >> > >> > --- >> > 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? >> We should, otherwise the old src QEMU (field_id=1,version_id=0) won't save the state that the new dst QEMU (field_id=0,version_id=0) will expect to be able to load. That errors out migration in the best case and causes a crash in the worst. Bumping both version_id and minimum_version_id, results in the old src QEMU (field_id=1,version_id=0) not saving the state, but the new dst QEMU (field_id=0,minimum_version_id=1) will reject the load because of the (old)version_id vs. (new)minimum_version_id mismatch. We could also keep compatibility old->new by bumping version_id only and adding a field->exists function that returns false when version_id==0. Then the old src QEMU (field_id=1,version_id=0) does not save the state, and the new dst QEMU (field_id=0,version_id=1,minimum_version_id=0) will not attempt to load because fiels->exists() returns false. The field->exists() function is only there when version_id=1 so it will never skip its own save. There's no chance for full compatibility because when new->old, the new src QEMU (field_id=0,version_id=1) already has a version_id too new for old dst QEMU (field_id=1,version_id=0). The migration will fail. >> thanks >> -- PMM
