Re: Rust high-level pre/post migration callbacks

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

On Wed, Sep 10, 2025 at 01:33:53PM +0200, Paolo Bonzini wrote:
> Date: Wed, 10 Sep 2025 13:33:53 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: Rust high-level pre/post migration callbacks
> 
> On Wed, Sep 10, 2025 at 9:58 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > > If a pure snapshot is possible, implementing the new trait
> > > is also simple:
> > >
> > > impl_vmstate_struct!(MyDeviceRegisters, ...);
> > >
> > > impl ToMigrationState for MyDeviceRegisters {
> > >     type Migrated = Self;
> > >     fn to_migration_state(&self) ->
> >
> > Just about the name:
> >
> > `to_migration_state` and `restore_migrated_state*` sound not a proper pair.
> > What about `snapshoot_migration_state` and `restore_migration_state`?
> 
> to_migration_state is the one that creates a new migration state, but
> perhaps it could be implemented in terms of
> 
>     fn snapshot_migration_state(&self, target: &mut Self::Migrated) ->
>        Result<(), migration::InvalidError>;

Thanks for this snapshot_migration_state() example. Then I think the
original to_migration_state() is better (returning migration state in the
`Result` looks better than passing `&mut` :). )

> > > trait ToMigrationState {
> > >     type Migrated: Default + VMState;
> > >     fn to_migration_state(&self) ->
> >
> > I think maybe here it's also necessary to accept a `&mut self` since
> > device would make some changes in pre_save.
> >
> > Then this trate can provide mutable methods and ToMigrationStateShare
> > provides immuatable ones.
> 
> That should not be necessary with this approach,

Indeed, I had misunderstood this point. Rethink the pre_save: Use HPET
as the example - changing something within `pre_save` is what HPETState's
`pre_save` does, and it's unrelated to the current Migratable<>'s
`pre_save`.

Here, Migratable<>'s `pre_save` is used to assist in retrieving the
corresponding migration state.

> since all changes can
> be done in the newly-allocated migration state.

EMM, I didn't your point here... could you please talk more about this
point?

> > > impl<T> ToMigrationState for Mutex<T: ToMigrationState> {
> > >     type Migrated = T::Migrated;
> > >     fn to_migration_state(&self) ->
> > >         Result<Box<Self::Migrated>, migration::InvalidError> {
> > >         self.lock().to_migration_state()
> >
> > I'm considerring maybe we could use get_mut() (and check bql by
> > assert!(bql_locked())) instead of locking this Mutex.
> >
> > In this context, C side should hold the BQL lock so that this is
> > already a stronger protection.
> 
> For non-BQL-protected device I think you cannot know that another
> thread isn't taking the lock. For BQL the assertion is only needed in
> Migratable and BqlRefCell's implementation of ToMigrationStateShared.

Yes, I see.

> > This omits the restore_migrated_state_mut, I guess it should be
> > filled with `unimplemented!()`.
> 
> restore_migrated_state_mut() however *can* use get_mut().

Yes!

> > > unsafe impl VMState for Migratable<T: ToMigrationStateShared> {
> > >     const BASE: bindings::VMStateField = {
> > >         static VMSD: &$crate::bindings::VMStateDescription =
> > >             VMStateDescriptionBuilder::<Self>::new()
> > >                 .version_id(T::VMSD.version_id)
> > >                 .minimum_version_id(T::VMSD.minimum_version_id)
> > >                 .priority(T::VMSD.priority)
> > >                 .pre_load(Self::pre_load)
> > >                 .post_load(Self::post_load)
> > >                 .pre_save(Self::pre_save)
> > >                 .post_save(Self::post_save)
> >
> > Maybe performance is a thing, and it might be worth comparing the
> > impact of these additional callbacks.
> 
> This is only done once per device so it should be okay.

Okay, I agree.

Thanks,
Zhao