Rust high-level pre/post migration callbacks

Rust high-level pre/post migration callbacks

[Paolo Bonzini]

Hi,

based on the low-level sketch in Zhao and my presentation,
I would like to propose this more high-level implementation
of pre/post migration callbacks.

Instead of dealing with pre/post callbacks, devices implement a
snapshot/restore mechanism; this way, C code sees a simplified
picture and does not have to deal with Rust concepts such as
Mutex<>.

Using it is very easy, you can just declare your state like:

     regs: Migratable<Mutex<MyDeviceRegisters>>

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) ->
         Result<Box<Self>, ...> {
         Ok(Box::new(self.clone()))
     }

     fn restore_migrated_state_mut(&mut self, source: &Self,
         _version_id: u8) -> Result<(), migration::InvalidError> {
         *self = source;
         Ok(())
     }
}

I'm really bad at writing Rust code with the correct syntax
from the get-go, but I'll try anyway.

new traits:

/// Enables QEMU migration support for types that may be wrapped in
/// synchronization primitives (like `Mutex`) that the C migration
/// code cannot directly handle. The trait provides methods to
/// extract essential state for migration and restore it after
/// migration completes.
///
/// On top of extracting data from synchronization wrappers during save
/// and restoring it during load, it's also possible to convert
/// runtime representations to migration-safe formats.
trait ToMigrationState {
     type Migrated: Default + VMState;
     fn to_migration_state(&self) ->
         Result<Box<Self::Migrated>, migration::InvalidError>;
     fn restore_migrated_state_mut(&mut self, source: &Self::Migrated,
         version_id: u8) -> Result<(), migration::InvalidError>;
}

/// Extension trait for types that support migration state restoration
/// through interior mutability.
///
/// This trait extends `ToMigrationState` for types that can restore
/// their state without requiring mutable access.  While user structs
/// will generally use `ToMigrationState`, the device will have multiple
/// references and therefore the device struct has to employ an interior
/// mutability wrapper like `Mutex`, `RefCell`, or `BqlRefCell`.  In
/// turn, wrappers implementing this trait can be used within `Migratable<T>`,
/// which makes no assumptions on how to achieve mutable access to the
/// run-time state.
trait ToMigrationStateShared: ToMigrationState {
     fn restore_migrated_state(&self, source: &Self::Migrated) ->
         Result<(), migration::InvalidError>;
}


with implementations for wrapper types like:

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()
     }
     ...
}

impl<T> ToMigrationStateShared for Mutex<T: ToMigrationState> {
     fn restore_migrated_state(&self, source: &Self::Migrated,
         version_id: u8) -> Result<(), migration::InvalidError>{
         self.lock().restore_migrated_state_mut(source, version_id)
     }
}

impl<T> ToMigrationState for BqlRefCell<T: ToMigrationState> {
     type Migrated = T::Migrated;
     fn to_migration_state(&self) ->
         Result<Box<Self::Migrated>, migration::InvalidError> {
         self.borrow().to_migration_state()
     }
     ...
}

impl<T> ToMigrationStateShared for BqlRefCell<T: ToMigrationState> {
     fn restore_migrated_state(&self, source: &Self::Migrated,
         version_id: u8) ->Result<(), migration::InvalidError> {
         self.borrow_mut().restore_migrated_state_mut(source, version_id)
     }
}

new struct maps the above trait to the C-style callbacks:

/// A wrapper that bridges Rust types with QEMU's C-based migration system.
///
/// `Migratable<T>` enables QEMU migration support for Rust types that implement
/// `ToMigrationState`, as long as they are wrapped with an interior mutability
/// like `Mutex` or `BqlRefCell`.  It provides translation functionality as well
/// as access to synchronization primitives that the C code cannot directly handle.
///
/// This wrapper acts as a transparent proxy during normal operation
/// (via `Deref`/`DerefMut`), while handling state extraction and restoration
/// around migration.
pub struct<T: ToMigrationStateShared> Migratable {
     runtime_state: T,
     // C vmstate does not support NULL pointers, so no Option<Box<>>
     // Actually a BqlCell<*mut T::Migrated>, but keeping it simple
     // for now.
     migration_state: *mut T::Migrated
};

unsafe impl<T> Send for Migratable<T: Send> {}
unsafe impl<T> Sync for Migratable<T: Sync> {}

// just return runtime_state
impl<T> Deref for Migratable<T: ToMigrationStateShared> {
     type Migrated = T;
     ...
}
impl<T> DerefMut for Migratable<T: ToMigrationStateShared> {
     ...
}

impl Migratable {
     fn pre_save(...) -> ... {
         self.migration_state = Box::into_raw(self.0.to_migration_state()?);
     }

     fn post_save(...) -> ... {
         drop(Box::from_raw(self.migration_state.replace(ptr::null_mut()));
     }

     fn pre_load(...) -> ... {
         self.migration_state = Box::into_raw(Box::default());
     }

     fn post_load(...) -> ... {
         let state = Box::from_raw(self.migration_state.replace(ptr::null_mut());
         self.0.restore_migrated_state(state, version_id)
     }
}

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)
                 .fields(vmstate_fields! {
                     vmstate_of!(Migratable<T>, migration_state)
                 }
                 .build();

         bindings::VMStateField {
             vmsd: addr_of!(*VMSD),
             size: size_of::<Migratable<T>>(),
             flags: bindings::VMStateFlags::VMS_STRUCT,
             ..common::Zeroable::ZERO
         }
     };
}

This is just a sketch but should give the idea.

Paolo

Re: Rust high-level pre/post migration callbacks

[Paolo Bonzini]

Oh, and here is a possible way that ToMigrationState could be
implemented automatically:

#[derive(ToMigrationState)]
struct DeviceRegisters {
    #[migration_state(omit)]
    runtime_field: u32,

    #[migration_state(clone)]
    shared_data: String,

    #[migration_state(into(Cow<'static, str>), clone)]
    converted_field: String,

    #[migration_state(try_into(i8))]
    fallible_field: u32,

    // Default: use ToMigrationState trait recursively
    nested_field: NestedStruct,

    // Primitive types have a default implementation of ToMigrationState
    simple_field: u32,
}

Paolo

On Sat, Sep 6, 2025 at 8:45 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Hi,
>
> based on the low-level sketch in Zhao and my presentation,
> I would like to propose this more high-level implementation
> of pre/post migration callbacks.
>
> Instead of dealing with pre/post callbacks, devices implement a
> snapshot/restore mechanism; this way, C code sees a simplified
> picture and does not have to deal with Rust concepts such as
> Mutex<>.
>
> Using it is very easy, you can just declare your state like:
>
>      regs: Migratable<Mutex<MyDeviceRegisters>>
>
> 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) ->
>          Result<Box<Self>, ...> {
>          Ok(Box::new(self.clone()))
>      }
>
>      fn restore_migrated_state_mut(&mut self, source: &Self,
>          _version_id: u8) -> Result<(), migration::InvalidError> {
>          *self = source;
>          Ok(())
>      }
> }
>
> I'm really bad at writing Rust code with the correct syntax
> from the get-go, but I'll try anyway.
>
> new traits:
>
> /// Enables QEMU migration support for types that may be wrapped in
> /// synchronization primitives (like `Mutex`) that the C migration
> /// code cannot directly handle. The trait provides methods to
> /// extract essential state for migration and restore it after
> /// migration completes.
> ///
> /// On top of extracting data from synchronization wrappers during save
> /// and restoring it during load, it's also possible to convert
> /// runtime representations to migration-safe formats.
> trait ToMigrationState {
>      type Migrated: Default + VMState;
>      fn to_migration_state(&self) ->
>          Result<Box<Self::Migrated>, migration::InvalidError>;
>      fn restore_migrated_state_mut(&mut self, source: &Self::Migrated,
>          version_id: u8) -> Result<(), migration::InvalidError>;
> }
>
> /// Extension trait for types that support migration state restoration
> /// through interior mutability.
> ///
> /// This trait extends `ToMigrationState` for types that can restore
> /// their state without requiring mutable access.  While user structs
> /// will generally use `ToMigrationState`, the device will have multiple
> /// references and therefore the device struct has to employ an interior
> /// mutability wrapper like `Mutex`, `RefCell`, or `BqlRefCell`.  In
> /// turn, wrappers implementing this trait can be used within `Migratable<T>`,
> /// which makes no assumptions on how to achieve mutable access to the
> /// run-time state.
> trait ToMigrationStateShared: ToMigrationState {
>      fn restore_migrated_state(&self, source: &Self::Migrated) ->
>          Result<(), migration::InvalidError>;
> }
>
>
> with implementations for wrapper types like:
>
> 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()
>      }
>      ...
> }
>
> impl<T> ToMigrationStateShared for Mutex<T: ToMigrationState> {
>      fn restore_migrated_state(&self, source: &Self::Migrated,
>          version_id: u8) -> Result<(), migration::InvalidError>{
>          self.lock().restore_migrated_state_mut(source, version_id)
>      }
> }
>
> impl<T> ToMigrationState for BqlRefCell<T: ToMigrationState> {
>      type Migrated = T::Migrated;
>      fn to_migration_state(&self) ->
>          Result<Box<Self::Migrated>, migration::InvalidError> {
>          self.borrow().to_migration_state()
>      }
>      ...
> }
>
> impl<T> ToMigrationStateShared for BqlRefCell<T: ToMigrationState> {
>      fn restore_migrated_state(&self, source: &Self::Migrated,
>          version_id: u8) ->Result<(), migration::InvalidError> {
>          self.borrow_mut().restore_migrated_state_mut(source, version_id)
>      }
> }
>
> new struct maps the above trait to the C-style callbacks:
>
> /// A wrapper that bridges Rust types with QEMU's C-based migration system.
> ///
> /// `Migratable<T>` enables QEMU migration support for Rust types that implement
> /// `ToMigrationState`, as long as they are wrapped with an interior mutability
> /// like `Mutex` or `BqlRefCell`.  It provides translation functionality as well
> /// as access to synchronization primitives that the C code cannot directly handle.
> ///
> /// This wrapper acts as a transparent proxy during normal operation
> /// (via `Deref`/`DerefMut`), while handling state extraction and restoration
> /// around migration.
> pub struct<T: ToMigrationStateShared> Migratable {
>      runtime_state: T,
>      // C vmstate does not support NULL pointers, so no Option<Box<>>
>      // Actually a BqlCell<*mut T::Migrated>, but keeping it simple
>      // for now.
>      migration_state: *mut T::Migrated
> };
>
> unsafe impl<T> Send for Migratable<T: Send> {}
> unsafe impl<T> Sync for Migratable<T: Sync> {}
>
> // just return runtime_state
> impl<T> Deref for Migratable<T: ToMigrationStateShared> {
>      type Migrated = T;
>      ...
> }
> impl<T> DerefMut for Migratable<T: ToMigrationStateShared> {
>      ...
> }
>
> impl Migratable {
>      fn pre_save(...) -> ... {
>          self.migration_state = Box::into_raw(self.0.to_migration_state()?);
>      }
>
>      fn post_save(...) -> ... {
>          drop(Box::from_raw(self.migration_state.replace(ptr::null_mut()));
>      }
>
>      fn pre_load(...) -> ... {
>          self.migration_state = Box::into_raw(Box::default());
>      }
>
>      fn post_load(...) -> ... {
>          let state = Box::from_raw(self.migration_state.replace(ptr::null_mut());
>          self.0.restore_migrated_state(state, version_id)
>      }
> }
>
> 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)
>                  .fields(vmstate_fields! {
>                      vmstate_of!(Migratable<T>, migration_state)
>                  }
>                  .build();
>
>          bindings::VMStateField {
>              vmsd: addr_of!(*VMSD),
>              size: size_of::<Migratable<T>>(),
>              flags: bindings::VMStateFlags::VMS_STRUCT,
>              ..common::Zeroable::ZERO
>          }
>      };
> }
>
> This is just a sketch but should give the idea.
>
> Paolo

Re: Rust high-level pre/post migration callbacks

[Manos Pitsidianakis]

Hi Paolo,

I'm not familiar with how migration works under the hood, but this
data transformation design looks very clean and neat to me.

On Sat, Sep 6, 2025 at 9:45 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Hi,
>
> based on the low-level sketch in Zhao and my presentation,
> I would like to propose this more high-level implementation
> of pre/post migration callbacks.
>
> Instead of dealing with pre/post callbacks, devices implement a
> snapshot/restore mechanism; this way, C code sees a simplified
> picture and does not have to deal with Rust concepts such as
> Mutex<>.
>
> Using it is very easy, you can just declare your state like:
>
>      regs: Migratable<Mutex<MyDeviceRegisters>>
>

Hm it's a shame we cannot do this with a trait since it requires state
storage for migration_state.

A suggestion: we could declare a "mirror" struct to hold
`migration_state` with a Derive macro. This is what the `rkyv` crate
does with its `Archive` derive macro and trait
<https://docs.rs/rkyv/latest/rkyv/trait.Archive.html>

Re: Rust high-level pre/post migration callbacks

[Paolo Bonzini]

On 9/8/25 12:02, Manos Pitsidianakis wrote:
> Hi Paolo,
> 
> I'm not familiar with how migration works under the hood, but this
> data transformation design looks very clean and neat to me.

Thanks!

>> Using it is very easy, you can just declare your state like:
>>
>>       regs: Migratable<Mutex<MyDeviceRegisters>>
> 
> Hm it's a shame we cannot do this with a trait since it requires state
> storage for migration_state.

Yeah, but a Box is just a single pointer.  We need to have an actual 
location in memory for the C code to retrieve the pointer.

> A suggestion: we could declare a "mirror" struct to hold
> `migration_state` with a Derive macro. This is what the `rkyv` crate
> does with its `Archive` derive macro and trait
> <https://docs.rs/rkyv/latest/rkyv/trait.Archive.html>

Nice!  I'm not familiar with rkyv but it does look very similar.

When I tried to clean up the HPET (create HPETRegisters, and replace use 
of raw pointers with self_cell to implement self-referential structs), 
migration was the big mess, so I hope that this design will fix both 
that and Mutex<>.  And indeed having a derive macro is very similar to 
what I came up with one day later :) while thinking about how HPET could 
use it in practice.

Probably the derive macro would require some changes to the trait, but 
the basic idea remains the same.

Paolo

Re: Rust high-level pre/post migration callbacks

[Manos Pitsidianakis]

On Mon, Sep 8, 2025 at 1:19 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 9/8/25 12:02, Manos Pitsidianakis wrote:
> > Hi Paolo,
> >
> > I'm not familiar with how migration works under the hood, but this
> > data transformation design looks very clean and neat to me.
>
> Thanks!
>
> >> Using it is very easy, you can just declare your state like:
> >>
> >>       regs: Migratable<Mutex<MyDeviceRegisters>>
> >
> > Hm it's a shame we cannot do this with a trait since it requires state
> > storage for migration_state.
>
> Yeah, but a Box is just a single pointer.  We need to have an actual
> location in memory for the C code to retrieve the pointer.

Oh yeah I got that. I was talking about needing to introduce new
wrapper types for fields.

>
> > A suggestion: we could declare a "mirror" struct to hold
> > `migration_state` with a Derive macro. This is what the `rkyv` crate
> > does with its `Archive` derive macro and trait
> > <https://docs.rs/rkyv/latest/rkyv/trait.Archive.html>
>
> Nice!  I'm not familiar with rkyv but it does look very similar.
>
> When I tried to clean up the HPET (create HPETRegisters, and replace use
> of raw pointers with self_cell to implement self-referential structs),
> migration was the big mess, so I hope that this design will fix both
> that and Mutex<>.  And indeed having a derive macro is very similar to
> what I came up with one day later :) while thinking about how HPET could
> use it in practice.
>
> Probably the derive macro would require some changes to the trait, but
> the basic idea remains the same.

Yes exactly. You could store the migration data in another struct
(e.g. `FooDeviceMigrationState`) and use it as a conduit between the
device and C migration logic.

>
> Paolo
>

Re: Rust high-level pre/post migration callbacks

[Zhao Liu]

> Instead of dealing with pre/post callbacks, devices implement a
> snapshot/restore mechanism; this way, C code sees a simplified
> picture and does not have to deal with Rust concepts such as
> Mutex<>.
> 
> Using it is very easy, you can just declare your state like:
> 
>     regs: Migratable<Mutex<MyDeviceRegisters>>

I realized there's no need for QemuMutex binding, since we can use Rust
native Mutex directly, except some case which will share the Mutex with
C side.

> 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`?

>         Result<Box<Self>, ...> {
>         Ok(Box::new(self.clone()))
>     }
> 
>     fn restore_migrated_state_mut(&mut self, source: &Self,
>         _version_id: u8) -> Result<(), migration::InvalidError> {
>         *self = source;
>         Ok(())
>     }
> }
> 
> I'm really bad at writing Rust code with the correct syntax
> from the get-go, but I'll try anyway.
> 
> new traits:
> 
> /// Enables QEMU migration support for types that may be wrapped in
> /// synchronization primitives (like `Mutex`) that the C migration
> /// code cannot directly handle. The trait provides methods to
> /// extract essential state for migration and restore it after
> /// migration completes.
> ///
> /// On top of extracting data from synchronization wrappers during save
> /// and restoring it during load, it's also possible to convert
> /// runtime representations to migration-safe formats.
> 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.

BTW, if possible, maybe we can rename the traits like:
* ToMigrationState -> ToMigrationStateMut
* ToMigrationStateShared -> ToMigrationState

>         Result<Box<Self::Migrated>, migration::InvalidError>;
>     fn restore_migrated_state_mut(&mut self, source: &Self::Migrated,
>         version_id: u8) -> Result<(), migration::InvalidError>;
> }
> 
> /// Extension trait for types that support migration state restoration
> /// through interior mutability.
> ///
> /// This trait extends `ToMigrationState` for types that can restore
> /// their state without requiring mutable access.  While user structs
> /// will generally use `ToMigrationState`, the device will have multiple
> /// references and therefore the device struct has to employ an interior
> /// mutability wrapper like `Mutex`, `RefCell`, or `BqlRefCell`.  In
> /// turn, wrappers implementing this trait can be used within `Migratable<T>`,
> /// which makes no assumptions on how to achieve mutable access to the
> /// run-time state.
> trait ToMigrationStateShared: ToMigrationState {
>     fn restore_migrated_state(&self, source: &Self::Migrated) ->
>         Result<(), migration::InvalidError>;
> }
> 
> 
> with implementations for wrapper types like:
> 
> 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.

>     }
>     ...

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

> }
> 
> impl<T> ToMigrationStateShared for Mutex<T: ToMigrationState> {
>     fn restore_migrated_state(&self, source: &Self::Migrated,
>         version_id: u8) -> Result<(), migration::InvalidError>{
>         self.lock().restore_migrated_state_mut(source, version_id)

And here, maybe `get_mut()` + `assert!(bql_locked())`?

>     }
> }

...

> new struct maps the above trait to the C-style callbacks:
> 
> /// A wrapper that bridges Rust types with QEMU's C-based migration system.
> ///
> /// `Migratable<T>` enables QEMU migration support for Rust types that implement
> /// `ToMigrationState`, as long as they are wrapped with an interior mutability
> /// like `Mutex` or `BqlRefCell`.  It provides translation functionality as well
> /// as access to synchronization primitives that the C code cannot directly handle.
> ///
> /// This wrapper acts as a transparent proxy during normal operation
> /// (via `Deref`/`DerefMut`), while handling state extraction and restoration
> /// around migration.
> pub struct<T: ToMigrationStateShared> Migratable {
>     runtime_state: T,
>     // C vmstate does not support NULL pointers, so no Option<Box<>>
>     // Actually a BqlCell<*mut T::Migrated>, but keeping it simple
>     // for now.
>     migration_state: *mut T::Migrated
> };
> 
> unsafe impl<T> Send for Migratable<T: Send> {}
> unsafe impl<T> Sync for Migratable<T: Sync> {}
> 
> // just return runtime_state
> impl<T> Deref for Migratable<T: ToMigrationStateShared> {
>     type Migrated = T;
>     ...
> }
> impl<T> DerefMut for Migratable<T: ToMigrationStateShared> {
>     ...
> }
> 
> impl Migratable {
>     fn pre_save(...) -> ... {
>         self.migration_state = Box::into_raw(self.0.to_migration_state()?);
>     }
> 
>     fn post_save(...) -> ... {
>         drop(Box::from_raw(self.migration_state.replace(ptr::null_mut()));
>     }
> 
>     fn pre_load(...) -> ... {
>         self.migration_state = Box::into_raw(Box::default());
>     }
> 
>     fn post_load(...) -> ... {
>         let state = Box::from_raw(self.migration_state.replace(ptr::null_mut());
>         self.0.restore_migrated_state(state, version_id)
>     }

Yes, this is a nice way to handle Rust wrappers.

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

>                 .fields(vmstate_fields! {
>                     vmstate_of!(Migratable<T>, migration_state)
>                 }
>                 .build();
> 
>         bindings::VMStateField {
>             vmsd: addr_of!(*VMSD),
>             size: size_of::<Migratable<T>>(),
>             flags: bindings::VMStateFlags::VMS_STRUCT,
>             ..common::Zeroable::ZERO
>         }
>     };
> }

Overall, the design looks great!

Thanks,
Zhao

Re: Rust high-level pre/post migration callbacks

[Paolo Bonzini]

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

Re: Rust high-level pre/post migration callbacks

[Zhao Liu]

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