[PATCH] include/hw/core/ptimer.h: Fix 'version' argument of VMSTATE_PTIMER

[PATCH] include/hw/core/ptimer.h: Fix 'version' argument of VMSTATE_PTIMER

[Peter Maydell]

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 <peter.maydell@linaro.org>
---
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,                   \
-- 
2.43.0

Re: [PATCH] include/hw/core/ptimer.h: Fix 'version' argument of VMSTATE_PTIMER

[Peter Maydell]

On Mon, 19 Oct 2026 at 14:16, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
> 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

Re: [PATCH] include/hw/core/ptimer.h: Fix 'version' argument of VMSTATE_PTIMER

[Peter Maydell]

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 <peter.maydell@linaro.org> wrote:
>
> On Mon, 19 Oct 2026 at 14:16, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> > ---
> > 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

Re: [PATCH] include/hw/core/ptimer.h: Fix 'version' argument of VMSTATE_PTIMER

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org> wrote:
>>
>> On Mon, 19 Oct 2026 at 14:16, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
>> > ---
>> > 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