| Message ID | 20250216092853.4169458-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hpet: do not overwrite properties on post_load | expand |
> @@ -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 > >
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 > > > > > >
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>
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()
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(-)