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 <pbonz...@redhat.com>
> Subject: Re: [PATCH v2] hpet: do not overwrite properties on post_load
>
> Il lun 17 feb 2025, 07:35 Zhao Liu <zhao1....@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....@intel.com>
