On Wed, Sep 10, 2025 at 9:58 AM Zhao Liu <[email protected]> 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
