Re: Rust high-level pre/post migration callbacks

[Paolo Bonzini <pbonzini@redhat.com>]

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>;

> > 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, since all changes can
be done in the newly-allocated migration state.

> > 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.

> This omits the restore_migrated_state_mut, I guess it should be
> filled with `unimplemented!()`.

restore_migrated_state_mut() however *can* use get_mut().

> > 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.

Paolo