Re: [PATCH 10/11] rust: migration: implement ToMigrationState for Timer

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

On Wed, Oct 01, 2025 at 09:52:09AM +0200, Paolo Bonzini wrote:
> Date: Wed,  1 Oct 2025 09:52:09 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/11] rust: migration: implement ToMigrationState for Timer
> X-Mailer: git-send-email 2.51.0
> 
> Timer is a complex struct, allow adding it to a struct that
> uses #[derive(ToMigrationState)]; similar to vmstate_timer, only
> the expiration time has to be preserved.
> 
> In fact, because it is thread-safe, ToMigrationStateShared can
> also be implemented without needing a cell or mutex that wraps
> the timer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/migration/src/migratable.rs | 31 +++++++++++++++++++++++++++++++
>  rust/util/src/timer.rs           | 10 +++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/migration/src/migratable.rs b/rust/migration/src/migratable.rs
> index ded6fe8f4a6..e85ef810efc 100644
> --- a/rust/migration/src/migratable.rs
> +++ b/rust/migration/src/migratable.rs
> @@ -140,6 +140,26 @@ fn restore_migrated_state_mut(
>  
>  impl_for_primitive!(u8, u16, u32, u64, i8, i16, i32, i64, bool);
>  
> +impl ToMigrationState for util::timer::Timer {
> +    type Migrated = i64;
> +
> +    fn snapshot_migration_state(&self, target: &mut i64) -> Result<(), InvalidError> {
> +        // SAFETY: as_ptr() is unsafe to ensure that the caller reasons about
> +        // the pinning of the data inside the Opaque<>.  Here all we do is
> +        // access a field.
> +        *target = self.expire_time_ns().unwrap_or(-1);
> +        Ok(())
> +    }
> +
> +    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, const N: usize> ToMigrationState for [T; N]
>  where
>      [T::Migrated; N]: Default,
> @@ -237,6 +257,17 @@ fn restore_migrated_state(
>      ) -> Result<(), InvalidError>;
>  }
>  
> +impl ToMigrationStateShared for util::timer::Timer {
> +    fn restore_migrated_state(&self, source: i64, _version_id: u8) -> Result<(), InvalidError> {
> +        if source >= 0 {
> +            self.modify(source as u64);

Timer::modify() is the wrapper of timer_mod(), and timer_mod() will
re-compute the expire time with scale factor.

So here we should use a binding of timer_mod_ns() - Timer::modify_ns()
instead of Timer::modify(), just like C side did.

Moreover, for Timer::modify_ns() & Timer::modify(), the simplest
way is to build them based on timer_mod_ns() & timer_mod() (but this
sounds like shifting the responsibility for type checking and overflow
checking to the C side :-( ).

Alternative, we can make Timer::modify() based on Timer::modify_ns(),
but then it involves many numeric type conversions: scale is i32 but
expire_time is u64, and Timer::modify_ns() may accept a u64 argument.

Fortunately,, we can make Timer's modify() accept i64 to reduce type
conversions.

> +        } else {
> +            self.delete();
> +        }
> +        Ok(())
> +    }
> +}
> +
>  impl<T: ToMigrationStateShared, const N: usize> ToMigrationStateShared for [T; N]
>  where
>      [T::Migrated; N]: Default,
> diff --git a/rust/util/src/timer.rs b/rust/util/src/timer.rs
> index c6b3e4088ec..a99ff5e7ef7 100644
> --- a/rust/util/src/timer.rs
> +++ b/rust/util/src/timer.rs
> @@ -10,7 +10,8 @@
>  use common::{callbacks::FnCall, Opaque};
>  
>  use crate::bindings::{
> -    self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
> +    self, qemu_clock_get_ns, timer_del, timer_expire_time_ns, timer_init_full, timer_mod,
> +    QEMUClockType,
>  };
>  
>  /// A safe wrapper around [`bindings::QEMUTimer`].
> @@ -86,6 +87,13 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
>          }
>      }
>  
> +    pub fn expire_time_ns(&self) -> Option<i64> {
> +        // SAFETY: the only way to obtain a Timer safely is via methods that
> +        // take a Pin<&mut Self>, therefore the timer is pinned
> +        let ret = unsafe { timer_expire_time_ns(self.as_ptr()) };
> +        i64::try_from(ret).ok()

Good! This detects all negative values.

> +    }
> +
>      pub fn modify(&self, expire_time: u64) {
>          // SAFETY: the only way to obtain a Timer safely is via methods that
>          // take a Pin<&mut Self>, therefore the timer is pinned
> -- 
> 2.51.0
>