Re: [PATCH 4/7] rust: migration: add high-level migration wrappers

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

On Sat, Sep 20, 2025 at 04:29:55PM +0200, Paolo Bonzini wrote:
> Date: Sat, 20 Sep 2025 16:29:55 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/7] rust: migration: add high-level migration wrappers
> X-Mailer: git-send-email 2.51.0
> 
> Instead of dealing with pre/post callbacks, allow devices to
> implement a snapshot/restore mechanism; this has two main
> advantages:
> 
> - it can be easily implemented via procedural macros
> 
> - there can be generic implementations to deal with various
>   kinds of interior-mutable containers, from BqlRefCell to Mutex,
>   so that C code does not see Rust concepts such as Mutex<>.
> 
> Using it is easy; you can implement the snapshot/restore trait
> ToMigrationState and declare your state like:
> 
>      regs: Migratable<Mutex<MyDeviceRegisters>>
> 
> Migratable<> allows dereferencing to the underlying object with
> no run-time cost.
> 
> Note that Migratable<> actually does not accept ToMigrationState,
> only the similar ToMigrationStateShared trait that the user will mostly
> not care about.  This is required by the fact that pre/post callbacks
> take a &self, and ensures that the argument is a Mutex or BqlRefCell
> (including an array or Arc<> thereof).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/Cargo.lock                  |   1 +
>  rust/migration/Cargo.toml        |   1 +
>  rust/migration/meson.build       |   1 +
>  rust/migration/src/lib.rs        |   3 +
>  rust/migration/src/migratable.rs | 430 +++++++++++++++++++++++++++++++
>  5 files changed, 436 insertions(+)
>  create mode 100644 rust/migration/src/migratable.rs

The entire wrapper is quite nice. So,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>


Only a few comments for comments inline:

> +// Interior-mutable types only require ToMigrationState for the inner type!
> +

extra blank line.

> +impl<T: ToMigrationState> ToMigrationState for Mutex<T> {
> +    type Migrated = T::Migrated;
> +
> +    fn snapshot_migration_state(&self, target: &mut Self::Migrated) -> Result<(), InvalidError> {
> +        self.lock().unwrap().snapshot_migration_state(target)
> +    }

Or maybe your previous sentence is worth commenting on here:

// For non-BQL-protected device we cannot know that another
// thread isn't taking the lock. So, always acquire the lock.

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