hw/timer/hpet.c | 38 ++++++++++---------------------------- 1 file changed, 10 insertions(+), 28 deletions(-)
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> @@ -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>
© 2016 - 2025 Red Hat, Inc.