Re: [PATCH 8/9] rust/hpet: Support migration

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

On Tue, Apr 15, 2025 at 07:43:00PM +0200, Paolo Bonzini wrote:
> Date: Tue, 15 Apr 2025 19:43:00 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 8/9] rust/hpet: Support migration
> 
> On Tue, Apr 15, 2025 at 4:21 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > An additional difficult case is vmsd(). Passing the raw VMStateDescription
> > > looks not good, while passing the VMStateDescription<> wrapper requires
> > > bounding DeviceImpl with 'static. Ultimately, I added an extra
> > > StaticVMStateDescription trait to successfully compile...
> >
> > Hmm I cannot fully understand it so I'll check it out later.
> 
> So the problem is that, in a "&'a Foo<T>", T must also be "T: 'a".
> One solution is for vmsd() to return an
> Option<VMStateDescription<Self>>, and do Box::into_raw(Box::new(vmsd))
> in the class_init method. Once we have const_refs_static, "fn vmsd()"
> can become a const and the Box is not needed anymore.

Thanks so much, that's a good idea!

About `Box::into_raw(Box::new(vmsd))`, do you think it's necessary to use
Box::leak(Box::new(*))? (though the Box<> isn't actively dropped during
the class's existence)

    pub fn class_init<T: DeviceImpl>(&mut self) {
        ...
        if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
            let static_vmsd: &'static mut bindings::VMStateDescription = Box::leak(Box::new(vmsd.get_vmsd()));
            self.vmsd = static_vmsd;
        }
    }

> Also please turn get_vmsd_ptr() into get_vmsd_ref() so that we get
> more checks that things are not copied behind our back (leaving behind
> a dangling pointer)

Sure!

> I attach the conversion I did of the other devices and tests. I am not
> sure if it's possible to avoid having a huge patch to do everything at
> once (except HPET since that can be added separately).

Thank you again! From my initial thoughts: Splitting is also possible,
but it requires first renaming VMStateDescription<T> to
VMStateDescriptionWrapper<T>, then replacing it in pl011 and test (and
hpet) one by one, and finally renaming it back to VMStateDescription<T>.
If you prefer this approach, I can help you split your patch below.

> +const VMSTATE_HPET: VMStateDescription<HPETState> =
> +    VMStateDescriptionBuilder::<HPETState>::new()
> +        .name(c_str!("hpet"))
> +        .version_id(2)
> +        .minimum_version_id(1)
> +        .pre_save(&HPETState::pre_save)
> +        .post_load(&HPETState::post_load)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETState, config),
> +            vmstate_of!(HPETState, int_status),
> +            vmstate_of!(HPETState, counter),
> +            vmstate_of!(HPETState, num_timers_save).with_version_id(2),
> +            vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
> +            vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),

And it seems like you don't oppose the hack in patch 1? ;-)

> +        })
> +        .subsections(vmstate_subsections!(
> +            VMSTATE_HPET_RTC_IRQ_LEVEL,
> +            VMSTATE_HPET_OFFSET,
> +        ))
> +        .build();

Thanks,
Zhao