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 <pbonz...@redhat.com>
> Subject: Re: Rust high-level pre/post migration callbacks
>
> On Wed, Sep 10, 2025 at 9:58 AM Zhao Liu <zhao1....@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
