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

[Zhao Liu <zhao1.liu@intel.com>]

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>