[PATCH v2] hpet: do not overwrite properties on post_load

[PATCH v2] hpet: do not overwrite properties on post_load

[Paolo Bonzini]

Migration relies on having the same device configuration on the source
and destination.  Therefore, there is no need to modify flags,
timer capabilities and the fw_cfg HPET block id on migration;
it was set to exactly the same values by realize.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: also do not overwrite num_timers

 hw/timer/hpet.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index dcff18a9871..ccb97b68066 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -77,6 +77,7 @@ struct HPETState {
     uint8_t rtc_irq_level;
     qemu_irq pit_enabled;
     uint8_t num_timers;
+    uint8_t num_timers_save;
     uint32_t intcap;
     HPETTimer timer[HPET_MAX_TIMERS];
 
@@ -237,15 +238,12 @@ static int hpet_pre_save(void *opaque)
         s->hpet_counter = hpet_get_ticks(s);
     }
 
-    return 0;
-}
-
-static int hpet_pre_load(void *opaque)
-{
-    HPETState *s = opaque;
-
-    /* version 1 only supports 3, later versions will load the actual value */
-    s->num_timers = HPET_MIN_TIMERS;
+    /*
+     * The number of timers must match on source and destination, but it was
+     * also added to the migration stream.  Check that it matches the value
+     * that was configured.
+     */
+    s->num_timers_save = s->num_timers;
     return 0;
 }
 
@@ -253,12 +251,7 @@ static bool hpet_validate_num_timers(void *opaque, int version_id)
 {
     HPETState *s = opaque;
 
-    if (s->num_timers < HPET_MIN_TIMERS) {
-        return false;
-    } else if (s->num_timers > HPET_MAX_TIMERS) {
-        return false;
-    }
-    return true;
+    return s->num_timers == s->num_timers_save;
 }
 
 static int hpet_post_load(void *opaque, int version_id)
@@ -277,16 +270,6 @@ static int hpet_post_load(void *opaque, int version_id)
                         - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     }
 
-    /* Push number of timers into capability returned via HPET_ID */
-    s->capability &= ~HPET_ID_NUM_TIM_MASK;
-    s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
-    hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
-
-    /* Derive HPET_MSI_SUPPORT from the capability of the first timer. */
-    s->flags &= ~(1 << HPET_MSI_SUPPORT);
-    if (s->timer[0].config & HPET_TN_FSB_CAP) {
-        s->flags |= 1 << HPET_MSI_SUPPORT;
-    }
     return 0;
 }
 
@@ -347,14 +330,13 @@ static const VMStateDescription vmstate_hpet = {
     .version_id = 2,
     .minimum_version_id = 1,
     .pre_save = hpet_pre_save,
-    .pre_load = hpet_pre_load,
     .post_load = hpet_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT64(config, HPETState),
         VMSTATE_UINT64(isr, HPETState),
         VMSTATE_UINT64(hpet_counter, HPETState),
-        VMSTATE_UINT8_V(num_timers, HPETState, 2),
-        VMSTATE_VALIDATE("num_timers in range", hpet_validate_num_timers),
+        VMSTATE_UINT8_V(num_timers_save, HPETState, 2),
+        VMSTATE_VALIDATE("num_timers must match", hpet_validate_num_timers),
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
                                     vmstate_hpet_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
-- 
2.48.1

Re: [PATCH v2] hpet: do not overwrite properties on post_load

[Zhao Liu]

> @@ -347,14 +330,13 @@ static const VMStateDescription vmstate_hpet = {
>      .version_id = 2,
>      .minimum_version_id = 1,
>      .pre_save = hpet_pre_save,
> -    .pre_load = hpet_pre_load,
>      .post_load = hpet_post_load,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT64(config, HPETState),
>          VMSTATE_UINT64(isr, HPETState),
>          VMSTATE_UINT64(hpet_counter, HPETState),
> -        VMSTATE_UINT8_V(num_timers, HPETState, 2),
> -        VMSTATE_VALIDATE("num_timers in range", hpet_validate_num_timers),
> +        VMSTATE_UINT8_V(num_timers_save, HPETState, 2),

This change is safe since it doesn't change the vmstate layout so that
there's no need for bumping up the version.

But I still have the question as the comment in v1 [*]. User doesn't
have any way to modify the number of timers, why not just replace this
vmstate field with "VMSTATE_UNUSED_V(2, 1)"?

Or do you think we should keep the status quo for the future use, even
if these properties have not been modified yet?

[*]: https://lore.kernel.org/qemu-devel/Z5Oq4LppNuN7N6NL@intel.com/

> +        VMSTATE_VALIDATE("num_timers must match", hpet_validate_num_timers),
>          VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
>                                      vmstate_hpet_timer, HPETTimer),
>          VMSTATE_END_OF_LIST()
> -- 
> 2.48.1
> 
> 

Re: [PATCH v2] hpet: do not overwrite properties on post_load

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1811 bytes --]

Il lun 17 feb 2025, 07:35 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> > @@ -347,14 +330,13 @@ static const VMStateDescription vmstate_hpet = {
> >      .version_id = 2,
> >      .minimum_version_id = 1,
> >      .pre_save = hpet_pre_save,
> > -    .pre_load = hpet_pre_load,
> >      .post_load = hpet_post_load,
> >      .fields = (const VMStateField[]) {
> >          VMSTATE_UINT64(config, HPETState),
> >          VMSTATE_UINT64(isr, HPETState),
> >          VMSTATE_UINT64(hpet_counter, HPETState),
> > -        VMSTATE_UINT8_V(num_timers, HPETState, 2),
> > -        VMSTATE_VALIDATE("num_timers in range",
> hpet_validate_num_timers),
> > +        VMSTATE_UINT8_V(num_timers_save, HPETState, 2),
>
> This change is safe since it doesn't change the vmstate layout so that
> there's no need for bumping up the version.
>
> But I still have the question as the comment in v1 [*]. User doesn't
> have any way to modify the number of timers, why not just replace this
> vmstate field with "VMSTATE_UNUSED_V(2, 1)"?
>

Because I didn't want to bump the version; your suggestion is indeed
simpler but it would break migration to past versions of QEMU, which check
that num_timers is in range. VMSTATE_UNUSED would write a zero.

For Rust, I think it's easier to place the check in the post_load callback
BTW.

Paolo


Or do you think we should keep the status quo for the future use, even
> if these properties have not been modified yet?
>
> [*]: https://lore.kernel.org/qemu-devel/Z5Oq4LppNuN7N6NL@intel.com/
>
> > +        VMSTATE_VALIDATE("num_timers must match",
> hpet_validate_num_timers),
> >          VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
> >                                      vmstate_hpet_timer, HPETTimer),
> >          VMSTATE_END_OF_LIST()
> > --
> > 2.48.1
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2942 bytes --]

Re: [PATCH v2] hpet: do not overwrite properties on post_load

[Zhao Liu]

On Mon, Feb 17, 2025 at 09:02:24AM +0100, Paolo Bonzini wrote:
> Date: Mon, 17 Feb 2025 09:02:24 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v2] hpet: do not overwrite properties on post_load
> 
> Il lun 17 feb 2025, 07:35 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> 
> > > @@ -347,14 +330,13 @@ static const VMStateDescription vmstate_hpet = {
> > >      .version_id = 2,
> > >      .minimum_version_id = 1,
> > >      .pre_save = hpet_pre_save,
> > > -    .pre_load = hpet_pre_load,
> > >      .post_load = hpet_post_load,
> > >      .fields = (const VMStateField[]) {
> > >          VMSTATE_UINT64(config, HPETState),
> > >          VMSTATE_UINT64(isr, HPETState),
> > >          VMSTATE_UINT64(hpet_counter, HPETState),
> > > -        VMSTATE_UINT8_V(num_timers, HPETState, 2),
> > > -        VMSTATE_VALIDATE("num_timers in range",
> > hpet_validate_num_timers),
> > > +        VMSTATE_UINT8_V(num_timers_save, HPETState, 2),
> >
> > This change is safe since it doesn't change the vmstate layout so that
> > there's no need for bumping up the version.
> >
> > But I still have the question as the comment in v1 [*]. User doesn't
> > have any way to modify the number of timers, why not just replace this
> > vmstate field with "VMSTATE_UNUSED_V(2, 1)"?
> >
> 
> Because I didn't want to bump the version; your suggestion is indeed
> simpler but it would break migration to past versions of QEMU, which check
> that num_timers is in range. VMSTATE_UNUSED would write a zero.

Yes, this way needs to tweak num_timers in post_load.

> For Rust, I think it's easier to place the check in the post_load callback
> BTW.

Yes, I agree. Will honor this change on Rust side.

Well, pls let me add my r/b tag as well,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>