diff mbox series

[8/9] rust/hpet: Support migration

Message ID 20250414144943.1112885-9-zhao1.liu@intel.com (mailing list archive)
State New
Headers show
Series rust/hpet: Initial support for migration | expand

Commit Message

Zhao Liu April 14, 2025, 2:49 p.m. UTC
Based on commit 1433e38cc8 ("hpet: do not overwrite properties on
post_load"), add the basic migration support to Rust HPET.

The current migration implementation introduces multiple unsafe
callbacks. Before the vmstate builder, one possible cleanup approach is
to wrap callbacks in the vmstate binding using a method similar to the
vmstate_exist_fn macro.

However, this approach would also create a lot of repetitive code (since
vmstate has so many callbacks: pre_load, post_load, pre_save, post_save,
needed and dev_unplug_pending). Although it would be cleaner, it would
somewhat deviate from the path of the vmstate builder.

Therefore, firstly focus on completing the functionality of HPET, and
those current unsafe callbacks can at least clearly indicate the needed
functionality of vmstate. The next step is to consider refactoring
vmstate to move towards the vmstate builder direction.

Additionally, update rust.rst about Rust HPET can support migration.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 docs/devel/rust.rst            |   3 +-
 rust/hw/timer/hpet/src/hpet.rs | 146 ++++++++++++++++++++++++++++++++-
 2 files changed, 146 insertions(+), 3 deletions(-)

Comments

Zhao Liu April 15, 2025, 12:01 p.m. UTC | #1
Hi Paolo,

Based on the this patch, I simply copied your MemoryRegionOpsBuilder
and quickly tried out the vmstate builder over a few cups of tea.

Although it can handle callbacks well, I found that the difficulty still
lies in the fact that the vmstate_fields and vmstate_subsections macros
cannot be eliminated, because any dynamic creation of arrays is not
allowed in a static context! (As I understand it, lazy_static might
still be needed.)

An additional difficult case is vmsd(). Passing the raw VMStateDescription
looks not good, while passing the VMStateDescription<> wrapper requires
bounding DeviceImpl with 'static. Ultimately, I added an extra
StaticVMStateDescription trait to successfully compile... In any case,
it's definitely still rough, but hope it helps and takes a small step
forward.

(Thanks for the patch 2 comment, I'll reply later!)

Thanks,
Zhao

---
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index a3538af14b48..16d495508424 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,7 +4,6 @@

 use std::{
     ffi::CStr,
-    os::raw::{c_int, c_void},
     pin::Pin,
     ptr::{addr_of_mut, null_mut, NonNull},
     slice::from_ref,
@@ -27,9 +26,8 @@
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
-    vmstate::VMStateDescription,
+    vmstate::{StaticVMStateDescription, VMStateDescription, VMStateDescriptionBuilder},
     vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_validate,
-    zeroable::Zeroable,
 };

 use crate::fw_cfg::HPETFwConfig;
@@ -943,104 +941,73 @@ impl ObjectImpl for HPETState {
     ),
 }

-unsafe extern "C" fn hpet_rtc_irq_level_needed(opaque: *mut c_void) -> bool {
-    // SAFETY:
-    // the pointer is convertible to a reference
-    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
-    state.is_rtc_irq_level_needed()
-}
-
-unsafe extern "C" fn hpet_offset_needed(opaque: *mut c_void) -> bool {
-    // SAFETY:
-    // the pointer is convertible to a reference
-    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
-    state.is_offset_needed()
-}
-unsafe extern "C" fn hpet_pre_save(opaque: *mut c_void) -> c_int {
-    // SAFETY:
-    // the pointer is convertible to a reference
-    let state: &mut HPETState =
-        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
-    state.pre_save() as c_int
-}
-
-unsafe extern "C" fn hpet_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
-    // SAFETY:
-    // the pointer is convertible to a reference
-    let state: &mut HPETState =
-        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
-    let version: u8 = version_id.try_into().unwrap();
-    state.post_load(version) as c_int
-}
-
-static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription = VMStateDescription {
-    name: c_str!("hpet/rtc_irq_level").as_ptr(),
-    version_id: 1,
-    minimum_version_id: 1,
-    needed: Some(hpet_rtc_irq_level_needed),
-    fields: vmstate_fields! {
-        vmstate_of!(HPETState, rtc_irq_level),
-    },
-    ..Zeroable::ZERO
-};
-
-static VMSTATE_HPET_OFFSET: VMStateDescription = VMStateDescription {
-    name: c_str!("hpet/offset").as_ptr(),
-    version_id: 1,
-    minimum_version_id: 1,
-    needed: Some(hpet_offset_needed),
-    fields: vmstate_fields! {
-        vmstate_of!(HPETState, hpet_offset),
-    },
-    ..Zeroable::ZERO
-};
-
-static VMSTATE_HPET_TIMER: VMStateDescription = VMStateDescription {
-    name: c_str!("hpet_timer").as_ptr(),
-    version_id: 1,
-    minimum_version_id: 1,
-    fields: vmstate_fields! {
-        vmstate_of!(HPETTimer, index),
-        vmstate_of!(HPETTimer, config),
-        vmstate_of!(HPETTimer, cmp),
-        vmstate_of!(HPETTimer, fsb),
-        vmstate_of!(HPETTimer, period),
-        vmstate_of!(HPETTimer, wrap_flag),
-        vmstate_of!(HPETTimer, qemu_timer),
-    },
-    ..Zeroable::ZERO
-};
+static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription<HPETState> =
+    VMStateDescriptionBuilder::<HPETState>::new()
+        .name(c_str!("hpet/rtc_irq_level"))
+        .version_id(1)
+        .minimum_version_id(1)
+        .needed(&HPETState::is_rtc_irq_level_needed)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETState, rtc_irq_level),
+        })
+        .build();
+
+static VMSTATE_HPET_OFFSET: VMStateDescription<HPETState> =
+    VMStateDescriptionBuilder::<HPETState>::new()
+        .name(c_str!("hpet/offset"))
+        .version_id(1)
+        .minimum_version_id(1)
+        .needed(&HPETState::is_offset_needed)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETState, hpet_offset),
+        })
+        .build();
+
+static VMSTATE_HPET_TIMER: VMStateDescription<BqlRefCell<HPETTimer>> =
+    VMStateDescriptionBuilder::<BqlRefCell<HPETTimer>>::new()
+        .name(c_str!("hpet_timer"))
+        .version_id(1)
+        .minimum_version_id(1)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETTimer, index),
+            vmstate_of!(HPETTimer, config),
+            vmstate_of!(HPETTimer, cmp),
+            vmstate_of!(HPETTimer, fsb),
+            vmstate_of!(HPETTimer, period),
+            vmstate_of!(HPETTimer, wrap_flag),
+            vmstate_of!(HPETTimer, qemu_timer),
+        })
+        .build();

 const VALIDATE_TIMERS_NAME: &CStr = c_str!("num_timers must match");

-static VMSTATE_HPET: VMStateDescription = VMStateDescription {
-    name: c_str!("hpet").as_ptr(),
-    version_id: 2,
-    minimum_version_id: 1,
-    pre_save: Some(hpet_pre_save),
-    post_load: Some(hpet_post_load),
-    fields: vmstate_fields! {
-        vmstate_of!(HPETState, config),
-        vmstate_of!(HPETState, int_status),
-        vmstate_of!(HPETState, counter),
-        vmstate_of!(HPETState, num_timers_save).with_version_id(2),
-        vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
-        vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
-    },
-    subsections: vmstate_subsections! {
-        VMSTATE_HPET_RTC_IRQ_LEVEL,
-        VMSTATE_HPET_OFFSET,
-    },
-    ..Zeroable::ZERO
-};
+static VMSTATE_HPET: VMStateDescription<HPETState> =
+    VMStateDescriptionBuilder::<HPETState>::new()
+        .name(c_str!("hpet"))
+        .version_id(2)
+        .minimum_version_id(1)
+        .pre_save(&HPETState::pre_save)
+        .post_load(&HPETState::post_load)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETState, config),
+            vmstate_of!(HPETState, int_status),
+            vmstate_of!(HPETState, counter),
+            vmstate_of!(HPETState, num_timers_save).with_version_id(2),
+            vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
+            vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
+        })
+        .subsections(vmstate_subsections!(
+            VMSTATE_HPET_RTC_IRQ_LEVEL,
+            VMSTATE_HPET_OFFSET,
+        ))
+        .build();

 impl DeviceImpl for HPETState {
     fn properties() -> &'static [Property] {
         &HPET_PROPERTIES
     }

-    fn vmsd() -> Option<&'static VMStateDescription> {
+    fn vmsd() -> Option<&'static dyn StaticVMStateDescription> {
         Some(&VMSTATE_HPET)
     }

diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 18b4a9ba687d..3398167d2b4d 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -20,7 +20,7 @@
     irq::InterruptSource,
     prelude::*,
     qom::{ObjectClass, ObjectImpl, Owned},
-    vmstate::VMStateDescription,
+    vmstate::{StaticVMStateDescription, VMStateDescription},
 };

 /// A safe wrapper around [`bindings::Clock`].
@@ -121,7 +121,7 @@ fn properties() -> &'static [Property] {
     /// A `VMStateDescription` providing the migration format for the device
     /// Not a `const` because referencing statics in constants is unstable
     /// until Rust 1.83.0.
-    fn vmsd() -> Option<&'static VMStateDescription> {
+    fn vmsd() -> Option<&'static dyn StaticVMStateDescription> {
         None
     }
 }
@@ -170,7 +170,7 @@ pub fn class_init<T: DeviceImpl>(&mut self) {
             self.realize = Some(rust_realize_fn::<T>);
         }
         if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
-            self.vmsd = vmsd;
+            self.vmsd = vmsd.get_vmsd_ptr();
         }
         let prop = <T as DeviceImpl>::properties();
         if !prop.is_empty() {
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 7d9f3a2ca6f2..35d4d12c8ed6 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -25,11 +25,18 @@
 //!   functionality that is missing from `vmstate_of!`.

 use core::{marker::PhantomData, mem, ptr::NonNull};
-use std::os::raw::{c_int, c_void};
+use std::{
+    ffi::CStr,
+    os::raw::{c_int, c_void},
+};

-pub use crate::bindings::{VMStateDescription, VMStateField};
+pub use crate::bindings::{MigrationPriority, VMStateField};
 use crate::{
-    bindings::VMStateFlags, callbacks::FnCall, prelude::*, qom::Owned, zeroable::Zeroable,
+    bindings::{VMStateDescription as RawVMStateDescription, VMStateFlags},
+    callbacks::FnCall,
+    prelude::*,
+    qom::Owned,
+    zeroable::Zeroable,
 };

 /// This macro is used to call a function with a generic argument bound
@@ -489,7 +496,7 @@ macro_rules! vmstate_struct {
             },
             size: ::core::mem::size_of::<$type>(),
             flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
-            vmsd: $vmsd,
+            vmsd: $vmsd.get_vmsd_ptr(),
             $(field_exists: $crate::vmstate_exist_fn!($struct_name, $test_fn),)?
             ..$crate::zeroable::Zeroable::ZERO
          } $(.with_varray_flag_unchecked(
@@ -586,11 +593,188 @@ macro_rules! vmstate_subsections {
     ($($subsection:expr),*$(,)*) => {{
         static _SUBSECTIONS: $crate::vmstate::VMStateSubsectionsWrapper = $crate::vmstate::VMStateSubsectionsWrapper(&[
             $({
-                static _SUBSECTION: $crate::bindings::VMStateDescription = $subsection;
+                static _SUBSECTION: $crate::bindings::VMStateDescription = $subsection.get_vmsd();
                 ::core::ptr::addr_of!(_SUBSECTION)
             }),*,
             ::core::ptr::null()
         ]);
-        _SUBSECTIONS.0.as_ptr()
+        &_SUBSECTIONS
     }}
 }
+
+pub struct VMStateDescription<T>(RawVMStateDescription, PhantomData<fn(&T)>);
+
+// SAFETY: When a *const T is passed to the callbacks, the call itself
+// is done in a thread-safe manner.  The invocation is okay as long as
+// T itself is `Sync`.
+unsafe impl<T: Sync> Sync for VMStateDescription<T> {}
+
+#[derive(Clone)]
+pub struct VMStateDescriptionBuilder<T>(RawVMStateDescription, PhantomData<fn(&T)>);
+
+unsafe extern "C" fn vmstate_pre_load_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
+    opaque: *mut c_void,
+) -> c_int {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
+}
+
+unsafe extern "C" fn vmstate_post_load_cb<T, F: for<'a> FnCall<(&'a T, u8), i32>>(
+    opaque: *mut c_void,
+    version_id: c_int,
+) -> c_int {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+    let version: u8 = version_id.try_into().unwrap();
+    F::call((owner, version)) as c_int
+}
+
+unsafe extern "C" fn vmstate_pre_save_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
+    opaque: *mut c_void,
+) -> c_int {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
+}
+
+unsafe extern "C" fn vmstate_post_save_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
+    opaque: *mut c_void,
+) -> c_int {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
+}
+
+unsafe extern "C" fn vmstate_needed_cb<T, F: for<'a> FnCall<(&'a T,), bool>>(
+    opaque: *mut c_void,
+) -> bool {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    F::call((unsafe { &*(opaque.cast::<T>()) },))
+}
+
+unsafe extern "C" fn vmstate_dev_unplug_pending_cb<T, F: for<'a> FnCall<(&'a T,), bool>>(
+    opaque: *mut c_void,
+) -> bool {
+    // SAFETY: the opaque was passed as a reference to `T`.
+    F::call((unsafe { &*(opaque.cast::<T>()) },))
+}
+
+impl<T> VMStateDescriptionBuilder<T> {
+    #[must_use]
+    pub const fn name(mut self, name_str: &CStr) -> Self {
+        self.0.name = ::std::ffi::CStr::as_ptr(name_str);
+        self
+    }
+
+    #[must_use]
+    pub const fn unmigratable(mut self) -> Self {
+        self.0.unmigratable = true;
+        self
+    }
+
+    #[must_use]
+    pub const fn early_setup(mut self) -> Self {
+        self.0.early_setup = true;
+        self
+    }
+
+    #[must_use]
+    pub const fn version_id(mut self, version: u8) -> Self {
+        self.0.version_id = version as c_int;
+        self
+    }
+
+    #[must_use]
+    pub const fn minimum_version_id(mut self, min_version: u8) -> Self {
+        self.0.minimum_version_id = min_version as c_int;
+        self
+    }
+
+    #[must_use]
+    pub const fn priority(mut self, priority: MigrationPriority) -> Self {
+        self.0.priority = priority;
+        self
+    }
+
+    #[must_use]
+    pub const fn pre_load<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
+        self.0.pre_load = Some(vmstate_pre_load_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn post_load<F: for<'a> FnCall<(&'a T, u8), i32>>(mut self, _f: &F) -> Self {
+        self.0.post_load = Some(vmstate_post_load_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn pre_save<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
+        self.0.pre_save = Some(vmstate_pre_save_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn post_save<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
+        self.0.post_save = Some(vmstate_post_save_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn needed<F: for<'a> FnCall<(&'a T,), bool>>(mut self, _f: &F) -> Self {
+        self.0.needed = Some(vmstate_needed_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn unplug_pending<F: for<'a> FnCall<(&'a T,), bool>>(mut self, _f: &F) -> Self {
+        self.0.dev_unplug_pending = Some(vmstate_dev_unplug_pending_cb::<T, F>);
+        self
+    }
+
+    #[must_use]
+    pub const fn fields(mut self, fields: *const VMStateField) -> Self {
+        self.0.fields = fields;
+        self
+    }
+
+    #[must_use]
+    pub const fn subsections(mut self, subs: &'static VMStateSubsectionsWrapper) -> Self {
+        self.0.subsections = subs.0.as_ptr();
+        self
+    }
+
+    #[must_use]
+    pub const fn build(self) -> VMStateDescription<T> {
+        VMStateDescription::<T>(self.0, PhantomData)
+    }
+
+    #[must_use]
+    pub const fn new() -> Self {
+        Self(RawVMStateDescription::ZERO, PhantomData)
+    }
+}
+
+impl<T> Default for VMStateDescriptionBuilder<T> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl<T> VMStateDescription<T> {
+    pub const fn get_vmsd(&self) -> RawVMStateDescription {
+        self.0
+    }
+
+    pub const fn get_vmsd_ptr(&self) -> *const RawVMStateDescription {
+        &self.0 as *const _ as *const RawVMStateDescription
+    }
+}
+
+pub trait StaticVMStateDescription {
+    fn get_vmsd_ptr(&'static self) -> *const RawVMStateDescription;
+}
+
+impl<T> StaticVMStateDescription for VMStateDescription<T> {
+    fn get_vmsd_ptr(&'static self) -> *const RawVMStateDescription {
+        self.get_vmsd_ptr()
+    }
+}
Paolo Bonzini April 15, 2025, 2:21 p.m. UTC | #2
On 4/15/25 14:01, Zhao Liu wrote:
> Hi Paolo,
> 
> Based on the this patch, I simply copied your MemoryRegionOpsBuilder
> and quickly tried out the vmstate builder over a few cups of tea.

Good idea (the tea :)).

> Although it can handle callbacks well, I found that the difficulty still
> lies in the fact that the vmstate_fields and vmstate_subsections macros
> cannot be eliminated, because any dynamic creation of arrays is not
> allowed in a static context!

Yes, this makes sense.  Array size must be known inside a const function 
and the extra terminator at the end of fields and subsections cannot be 
added by the builder itself.  c_str! has the same issue for the name, if 
I understand correctly.

> An additional difficult case is vmsd(). Passing the raw VMStateDescription
> looks not good, while passing the VMStateDescription<> wrapper requires
> bounding DeviceImpl with 'static. Ultimately, I added an extra
> StaticVMStateDescription trait to successfully compile...

Hmm I cannot fully understand it so I'll check it out later.

> In any case, it's definitely still rough, but hope it helps and
> takes a small step forward.

Yes, of course---this:

+static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription<HPETState> =
+    VMStateDescriptionBuilder::<HPETState>::new()
+        .name(c_str!("hpet/rtc_irq_level"))
+        .version_id(1)
+        .minimum_version_id(1)
+        .needed(&HPETState::is_rtc_irq_level_needed)
+        .fields(vmstate_fields! {
+            vmstate_of!(HPETState, rtc_irq_level),
+        })
+        .build();
+

is readable, not foreign (it's similar to the MemoryRegionOps) and 
provides an easy way to insert FFI wrappers.

Right now it's now fully typesafe, because the VMStateField returned by 
vmstate_of! (as well as the arrays returned by vmstate_fields! and 
vmstate_subsections!) does not track that it's for an HPETState; but 
that's a small thing overall and getting the basic builder right is more 
important.

I also made a note to check which callbacks could have a Result<> as the 
return type, possibly reusing the Errno module (Result<(), ()> looks a 
bit silly); but that is also not needed for this early stage.

Just a couple notes:

> +    bindings::{VMStateDescription as RawVMStateDescription, VMStateFlags},

I would use bindings::VMStateDescription throughout, similar to how
it's done in memory.rs.

> +    pub const fn name(mut self, name_str: &CStr) -> Self {
> +        self.0.name = ::std::ffi::CStr::as_ptr(name_str);


This can use "name_str.as_ptr()" because the type of name_str is known 
(unlike in macros, such as define_property! or vmstate_validate!).

(By the way, talking about macros, I have just stumbled on the attrs 
crate, which is something to keep an eye on for when QOM/qdev bindings 
are extended along the lines of 
https://lore.kernel.org/qemu-devel/e8e55772-906b-42cb-a744-031e6ae65f16@redhat.com/T/. 
  But I don't think procedural macros are a good match for VMState).

Paolo

> (Thanks for the patch 2 comment, I'll reply later!)
> 
> Thanks,
> Zhao
> 
> ---
> diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
> index a3538af14b48..16d495508424 100644
> --- a/rust/hw/timer/hpet/src/hpet.rs
> +++ b/rust/hw/timer/hpet/src/hpet.rs
> @@ -4,7 +4,6 @@
> 
>   use std::{
>       ffi::CStr,
> -    os::raw::{c_int, c_void},
>       pin::Pin,
>       ptr::{addr_of_mut, null_mut, NonNull},
>       slice::from_ref,
> @@ -27,9 +26,8 @@
>       qom_isa,
>       sysbus::{SysBusDevice, SysBusDeviceImpl},
>       timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
> -    vmstate::VMStateDescription,
> +    vmstate::{StaticVMStateDescription, VMStateDescription, VMStateDescriptionBuilder},
>       vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_validate,
> -    zeroable::Zeroable,
>   };
> 
>   use crate::fw_cfg::HPETFwConfig;
> @@ -943,104 +941,73 @@ impl ObjectImpl for HPETState {
>       ),
>   }
> 
> -unsafe extern "C" fn hpet_rtc_irq_level_needed(opaque: *mut c_void) -> bool {
> -    // SAFETY:
> -    // the pointer is convertible to a reference
> -    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
> -    state.is_rtc_irq_level_needed()
> -}
> -
> -unsafe extern "C" fn hpet_offset_needed(opaque: *mut c_void) -> bool {
> -    // SAFETY:
> -    // the pointer is convertible to a reference
> -    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
> -    state.is_offset_needed()
> -}
> -unsafe extern "C" fn hpet_pre_save(opaque: *mut c_void) -> c_int {
> -    // SAFETY:
> -    // the pointer is convertible to a reference
> -    let state: &mut HPETState =
> -        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
> -    state.pre_save() as c_int
> -}
> -
> -unsafe extern "C" fn hpet_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
> -    // SAFETY:
> -    // the pointer is convertible to a reference
> -    let state: &mut HPETState =
> -        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
> -    let version: u8 = version_id.try_into().unwrap();
> -    state.post_load(version) as c_int
> -}
> -
> -static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription = VMStateDescription {
> -    name: c_str!("hpet/rtc_irq_level").as_ptr(),
> -    version_id: 1,
> -    minimum_version_id: 1,
> -    needed: Some(hpet_rtc_irq_level_needed),
> -    fields: vmstate_fields! {
> -        vmstate_of!(HPETState, rtc_irq_level),
> -    },
> -    ..Zeroable::ZERO
> -};
> -
> -static VMSTATE_HPET_OFFSET: VMStateDescription = VMStateDescription {
> -    name: c_str!("hpet/offset").as_ptr(),
> -    version_id: 1,
> -    minimum_version_id: 1,
> -    needed: Some(hpet_offset_needed),
> -    fields: vmstate_fields! {
> -        vmstate_of!(HPETState, hpet_offset),
> -    },
> -    ..Zeroable::ZERO
> -};
> -
> -static VMSTATE_HPET_TIMER: VMStateDescription = VMStateDescription {
> -    name: c_str!("hpet_timer").as_ptr(),
> -    version_id: 1,
> -    minimum_version_id: 1,
> -    fields: vmstate_fields! {
> -        vmstate_of!(HPETTimer, index),
> -        vmstate_of!(HPETTimer, config),
> -        vmstate_of!(HPETTimer, cmp),
> -        vmstate_of!(HPETTimer, fsb),
> -        vmstate_of!(HPETTimer, period),
> -        vmstate_of!(HPETTimer, wrap_flag),
> -        vmstate_of!(HPETTimer, qemu_timer),
> -    },
> -    ..Zeroable::ZERO
> -};
> +static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription<HPETState> =
> +    VMStateDescriptionBuilder::<HPETState>::new()
> +        .name(c_str!("hpet/rtc_irq_level"))
> +        .version_id(1)
> +        .minimum_version_id(1)
> +        .needed(&HPETState::is_rtc_irq_level_needed)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETState, rtc_irq_level),
> +        })
> +        .build();
> +
> +static VMSTATE_HPET_OFFSET: VMStateDescription<HPETState> =
> +    VMStateDescriptionBuilder::<HPETState>::new()
> +        .name(c_str!("hpet/offset"))
> +        .version_id(1)
> +        .minimum_version_id(1)
> +        .needed(&HPETState::is_offset_needed)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETState, hpet_offset),
> +        })
> +        .build();
> +
> +static VMSTATE_HPET_TIMER: VMStateDescription<BqlRefCell<HPETTimer>> =
> +    VMStateDescriptionBuilder::<BqlRefCell<HPETTimer>>::new()
> +        .name(c_str!("hpet_timer"))
> +        .version_id(1)
> +        .minimum_version_id(1)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETTimer, index),
> +            vmstate_of!(HPETTimer, config),
> +            vmstate_of!(HPETTimer, cmp),
> +            vmstate_of!(HPETTimer, fsb),
> +            vmstate_of!(HPETTimer, period),
> +            vmstate_of!(HPETTimer, wrap_flag),
> +            vmstate_of!(HPETTimer, qemu_timer),
> +        })
> +        .build();
> 
>   const VALIDATE_TIMERS_NAME: &CStr = c_str!("num_timers must match");
> 
> -static VMSTATE_HPET: VMStateDescription = VMStateDescription {
> -    name: c_str!("hpet").as_ptr(),
> -    version_id: 2,
> -    minimum_version_id: 1,
> -    pre_save: Some(hpet_pre_save),
> -    post_load: Some(hpet_post_load),
> -    fields: vmstate_fields! {
> -        vmstate_of!(HPETState, config),
> -        vmstate_of!(HPETState, int_status),
> -        vmstate_of!(HPETState, counter),
> -        vmstate_of!(HPETState, num_timers_save).with_version_id(2),
> -        vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
> -        vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
> -    },
> -    subsections: vmstate_subsections! {
> -        VMSTATE_HPET_RTC_IRQ_LEVEL,
> -        VMSTATE_HPET_OFFSET,
> -    },
> -    ..Zeroable::ZERO
> -};
> +static VMSTATE_HPET: VMStateDescription<HPETState> =
> +    VMStateDescriptionBuilder::<HPETState>::new()
> +        .name(c_str!("hpet"))
> +        .version_id(2)
> +        .minimum_version_id(1)
> +        .pre_save(&HPETState::pre_save)
> +        .post_load(&HPETState::post_load)
> +        .fields(vmstate_fields! {
> +            vmstate_of!(HPETState, config),
> +            vmstate_of!(HPETState, int_status),
> +            vmstate_of!(HPETState, counter),
> +            vmstate_of!(HPETState, num_timers_save).with_version_id(2),
> +            vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
> +            vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
> +        })
> +        .subsections(vmstate_subsections!(
> +            VMSTATE_HPET_RTC_IRQ_LEVEL,
> +            VMSTATE_HPET_OFFSET,
> +        ))
> +        .build();
> 
>   impl DeviceImpl for HPETState {
>       fn properties() -> &'static [Property] {
>           &HPET_PROPERTIES
>       }
> 
> -    fn vmsd() -> Option<&'static VMStateDescription> {
> +    fn vmsd() -> Option<&'static dyn StaticVMStateDescription> {
>           Some(&VMSTATE_HPET)
>       }
> 
> diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
> index 18b4a9ba687d..3398167d2b4d 100644
> --- a/rust/qemu-api/src/qdev.rs
> +++ b/rust/qemu-api/src/qdev.rs
> @@ -20,7 +20,7 @@
>       irq::InterruptSource,
>       prelude::*,
>       qom::{ObjectClass, ObjectImpl, Owned},
> -    vmstate::VMStateDescription,
> +    vmstate::{StaticVMStateDescription, VMStateDescription},
>   };
> 
>   /// A safe wrapper around [`bindings::Clock`].
> @@ -121,7 +121,7 @@ fn properties() -> &'static [Property] {
>       /// A `VMStateDescription` providing the migration format for the device
>       /// Not a `const` because referencing statics in constants is unstable
>       /// until Rust 1.83.0.
> -    fn vmsd() -> Option<&'static VMStateDescription> {
> +    fn vmsd() -> Option<&'static dyn StaticVMStateDescription> {
>           None
>       }
>   }
> @@ -170,7 +170,7 @@ pub fn class_init<T: DeviceImpl>(&mut self) {
>               self.realize = Some(rust_realize_fn::<T>);
>           }
>           if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
> -            self.vmsd = vmsd;
> +            self.vmsd = vmsd.get_vmsd_ptr();
>           }
>           let prop = <T as DeviceImpl>::properties();
>           if !prop.is_empty() {
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index 7d9f3a2ca6f2..35d4d12c8ed6 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -25,11 +25,18 @@
>   //!   functionality that is missing from `vmstate_of!`.
> 
>   use core::{marker::PhantomData, mem, ptr::NonNull};
> -use std::os::raw::{c_int, c_void};
> +use std::{
> +    ffi::CStr,
> +    os::raw::{c_int, c_void},
> +};
> 
> -pub use crate::bindings::{VMStateDescription, VMStateField};
> +pub use crate::bindings::{MigrationPriority, VMStateField};
>   use crate::{
> -    bindings::VMStateFlags, callbacks::FnCall, prelude::*, qom::Owned, zeroable::Zeroable,
> +    bindings::{VMStateDescription as RawVMStateDescription, VMStateFlags},
> +    callbacks::FnCall,
> +    prelude::*,
> +    qom::Owned,
> +    zeroable::Zeroable,
>   };
> 
>   /// This macro is used to call a function with a generic argument bound
> @@ -489,7 +496,7 @@ macro_rules! vmstate_struct {
>               },
>               size: ::core::mem::size_of::<$type>(),
>               flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
> -            vmsd: $vmsd,
> +            vmsd: $vmsd.get_vmsd_ptr(),
>               $(field_exists: $crate::vmstate_exist_fn!($struct_name, $test_fn),)?
>               ..$crate::zeroable::Zeroable::ZERO
>            } $(.with_varray_flag_unchecked(
> @@ -586,11 +593,188 @@ macro_rules! vmstate_subsections {
>       ($($subsection:expr),*$(,)*) => {{
>           static _SUBSECTIONS: $crate::vmstate::VMStateSubsectionsWrapper = $crate::vmstate::VMStateSubsectionsWrapper(&[
>               $({
> -                static _SUBSECTION: $crate::bindings::VMStateDescription = $subsection;
> +                static _SUBSECTION: $crate::bindings::VMStateDescription = $subsection.get_vmsd();
>                   ::core::ptr::addr_of!(_SUBSECTION)
>               }),*,
>               ::core::ptr::null()
>           ]);
> -        _SUBSECTIONS.0.as_ptr()
> +        &_SUBSECTIONS
>       }}
>   }
> +
> +pub struct VMStateDescription<T>(RawVMStateDescription, PhantomData<fn(&T)>);
> +
> +// SAFETY: When a *const T is passed to the callbacks, the call itself
> +// is done in a thread-safe manner.  The invocation is okay as long as
> +// T itself is `Sync`.
> +unsafe impl<T: Sync> Sync for VMStateDescription<T> {}
> +
> +#[derive(Clone)]
> +pub struct VMStateDescriptionBuilder<T>(RawVMStateDescription, PhantomData<fn(&T)>);
> +
> +unsafe extern "C" fn vmstate_pre_load_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
> +    opaque: *mut c_void,
> +) -> c_int {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
> +}
> +
> +unsafe extern "C" fn vmstate_post_load_cb<T, F: for<'a> FnCall<(&'a T, u8), i32>>(
> +    opaque: *mut c_void,
> +    version_id: c_int,
> +) -> c_int {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    let owner: &T = unsafe { &*(opaque.cast::<T>()) };
> +    let version: u8 = version_id.try_into().unwrap();
> +    F::call((owner, version)) as c_int
> +}
> +
> +unsafe extern "C" fn vmstate_pre_save_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
> +    opaque: *mut c_void,
> +) -> c_int {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
> +}
> +
> +unsafe extern "C" fn vmstate_post_save_cb<T, F: for<'a> FnCall<(&'a T,), i32>>(
> +    opaque: *mut c_void,
> +) -> c_int {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    F::call((unsafe { &*(opaque.cast::<T>()) },)) as c_int
> +}
> +
> +unsafe extern "C" fn vmstate_needed_cb<T, F: for<'a> FnCall<(&'a T,), bool>>(
> +    opaque: *mut c_void,
> +) -> bool {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    F::call((unsafe { &*(opaque.cast::<T>()) },))
> +}
> +
> +unsafe extern "C" fn vmstate_dev_unplug_pending_cb<T, F: for<'a> FnCall<(&'a T,), bool>>(
> +    opaque: *mut c_void,
> +) -> bool {
> +    // SAFETY: the opaque was passed as a reference to `T`.
> +    F::call((unsafe { &*(opaque.cast::<T>()) },))
> +}
> +
> +impl<T> VMStateDescriptionBuilder<T> {
> +    #[must_use]
> +    pub const fn name(mut self, name_str: &CStr) -> Self {
> +        self.0.name = ::std::ffi::CStr::as_ptr(name_str);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn unmigratable(mut self) -> Self {
> +        self.0.unmigratable = true;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn early_setup(mut self) -> Self {
> +        self.0.early_setup = true;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn version_id(mut self, version: u8) -> Self {
> +        self.0.version_id = version as c_int;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn minimum_version_id(mut self, min_version: u8) -> Self {
> +        self.0.minimum_version_id = min_version as c_int;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn priority(mut self, priority: MigrationPriority) -> Self {
> +        self.0.priority = priority;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn pre_load<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
> +        self.0.pre_load = Some(vmstate_pre_load_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn post_load<F: for<'a> FnCall<(&'a T, u8), i32>>(mut self, _f: &F) -> Self {
> +        self.0.post_load = Some(vmstate_post_load_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn pre_save<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
> +        self.0.pre_save = Some(vmstate_pre_save_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn post_save<F: for<'a> FnCall<(&'a T,), i32>>(mut self, _f: &F) -> Self {
> +        self.0.post_save = Some(vmstate_post_save_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn needed<F: for<'a> FnCall<(&'a T,), bool>>(mut self, _f: &F) -> Self {
> +        self.0.needed = Some(vmstate_needed_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn unplug_pending<F: for<'a> FnCall<(&'a T,), bool>>(mut self, _f: &F) -> Self {
> +        self.0.dev_unplug_pending = Some(vmstate_dev_unplug_pending_cb::<T, F>);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn fields(mut self, fields: *const VMStateField) -> Self {
> +        self.0.fields = fields;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn subsections(mut self, subs: &'static VMStateSubsectionsWrapper) -> Self {
> +        self.0.subsections = subs.0.as_ptr();
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn build(self) -> VMStateDescription<T> {
> +        VMStateDescription::<T>(self.0, PhantomData)
> +    }
> +
> +    #[must_use]
> +    pub const fn new() -> Self {
> +        Self(RawVMStateDescription::ZERO, PhantomData)
> +    }
> +}
> +
> +impl<T> Default for VMStateDescriptionBuilder<T> {
> +    fn default() -> Self {
> +        Self::new()
> +    }
> +}
> +
> +impl<T> VMStateDescription<T> {
> +    pub const fn get_vmsd(&self) -> RawVMStateDescription {
> +        self.0
> +    }
> +
> +    pub const fn get_vmsd_ptr(&self) -> *const RawVMStateDescription {
> +        &self.0 as *const _ as *const RawVMStateDescription
> +    }
> +}
> +
> +pub trait StaticVMStateDescription {
> +    fn get_vmsd_ptr(&'static self) -> *const RawVMStateDescription;
> +}
> +
> +impl<T> StaticVMStateDescription for VMStateDescription<T> {
> +    fn get_vmsd_ptr(&'static self) -> *const RawVMStateDescription {
> +        self.get_vmsd_ptr()
> +    }
> +}
> 
> 
> 
> 
>
Paolo Bonzini April 15, 2025, 5:43 p.m. UTC | #3
On Tue, Apr 15, 2025 at 4:21 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > An additional difficult case is vmsd(). Passing the raw VMStateDescription
> > looks not good, while passing the VMStateDescription<> wrapper requires
> > bounding DeviceImpl with 'static. Ultimately, I added an extra
> > StaticVMStateDescription trait to successfully compile...
>
> Hmm I cannot fully understand it so I'll check it out later.

So the problem is that, in a "&'a Foo<T>", T must also be "T: 'a".
One solution is for vmsd() to return an
Option<VMStateDescription<Self>>, and do Box::into_raw(Box::new(vmsd))
in the class_init method. Once we have const_refs_static, "fn vmsd()"
can become a const and the Box is not needed anymore.

Also please turn get_vmsd_ptr() into get_vmsd_ref() so that we get
more checks that things are not copied behind our back (leaving behind
a dangling pointer)

I attach the conversion I did of the other devices and tests. I am not
sure if it's possible to avoid having a huge patch to do everything at
once (except HPET since that can be added separately).

Paolo
diff mbox series

Patch

diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 88bdec1eb28f..3cc2841d4d1f 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -153,8 +153,7 @@  QEMU includes four crates:
 
 .. [#issues] The ``pl011`` crate is synchronized with ``hw/char/pl011.c``
    as of commit 02b1f7f61928.  The ``hpet`` crate is synchronized as of
-   commit f32352ff9e.  Both are lacking tracing functionality; ``hpet``
-   is also lacking support for migration.
+   commit 1433e38cc8.  Both are lacking tracing functionality.
 
 This section explains how to work with them.
 
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index dc8a23f29d95..983f3882f8d3 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,6 +4,7 @@ 
 
 use std::{
     ffi::CStr,
+    os::raw::{c_int, c_void},
     pin::Pin,
     ptr::{addr_of_mut, null_mut, NonNull},
     slice::from_ref,
@@ -25,7 +26,10 @@ 
     qom::{ObjectImpl, ObjectType, ParentField},
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
-    timer::{Timer, CLOCK_VIRTUAL},
+    timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
+    vmstate::VMStateDescription,
+    vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_validate,
+    zeroable::Zeroable,
 };
 
 use crate::fw_cfg::HPETFwConfig;
@@ -561,6 +565,7 @@  pub struct HPETState {
     #[doc(alias = "timer")]
     timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS as usize],
     num_timers: BqlCell<u8>,
+    num_timers_save: BqlCell<u8>,
 
     /// Instance id (HPET timer block ID).
     hpet_id: BqlCell<usize>,
@@ -839,6 +844,49 @@  fn write(&self, addr: hwaddr, value: u64, size: u32) {
             }
         }
     }
+
+    fn pre_save(&self) -> i32 {
+        if self.is_hpet_enabled() {
+            self.counter.set(self.get_ticks());
+        }
+
+        /*
+         * The number of timers must match on source and destination, but it was
+         * also added to the migration stream.  Check that it matches the value
+         * that was configured.
+         */
+        self.num_timers_save.set(self.num_timers.get());
+        0
+    }
+
+    fn post_load(&self, _version_id: u8) -> i32 {
+        for timer in self.timers.iter().take(self.get_num_timers()) {
+            let mut t = timer.borrow_mut();
+
+            t.cmp64 = t.calculate_cmp64(t.get_state().counter.get(), t.cmp);
+            t.last = CLOCK_VIRTUAL.get_ns() - NANOSECONDS_PER_SECOND;
+        }
+
+        // Recalculate the offset between the main counter and guest time
+        if !self.hpet_offset_saved {
+            self.hpet_offset
+                .set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
+        }
+
+        0
+    }
+
+    fn is_rtc_irq_level_needed(&self) -> bool {
+        self.rtc_irq_level.get() != 0
+    }
+
+    fn is_offset_needed(&self) -> bool {
+        self.is_hpet_enabled() && self.hpet_offset_saved
+    }
+
+    fn validate_num_timers(&self, _version_id: u8) -> bool {
+        self.num_timers.get() == self.num_timers_save.get()
+    }
 }
 
 qom_isa!(HPETState: SysBusDevice, DeviceState, Object);
@@ -895,11 +943,107 @@  impl ObjectImpl for HPETState {
     ),
 }
 
+unsafe extern "C" fn hpet_rtc_irq_level_needed(opaque: *mut c_void) -> bool {
+    // SAFETY:
+    // the pointer is convertible to a reference
+    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
+    state.is_rtc_irq_level_needed()
+}
+
+unsafe extern "C" fn hpet_offset_needed(opaque: *mut c_void) -> bool {
+    // SAFETY:
+    // the pointer is convertible to a reference
+    let state: &HPETState = unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_ref() };
+    state.is_offset_needed()
+}
+
+unsafe extern "C" fn hpet_pre_save(opaque: *mut c_void) -> c_int {
+    // SAFETY:
+    // the pointer is convertible to a reference
+    let state: &mut HPETState =
+        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
+    state.pre_save() as c_int
+}
+
+unsafe extern "C" fn hpet_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
+    // SAFETY:
+    // the pointer is convertible to a reference
+    let state: &mut HPETState =
+        unsafe { NonNull::new(opaque.cast::<HPETState>()).unwrap().as_mut() };
+    let version: u8 = version_id.try_into().unwrap();
+    state.post_load(version) as c_int
+}
+
+static VMSTATE_HPET_RTC_IRQ_LEVEL: VMStateDescription = VMStateDescription {
+    name: c_str!("hpet/rtc_irq_level").as_ptr(),
+    version_id: 1,
+    minimum_version_id: 1,
+    needed: Some(hpet_rtc_irq_level_needed),
+    fields: vmstate_fields! {
+        vmstate_of!(HPETState, rtc_irq_level),
+    },
+    ..Zeroable::ZERO
+};
+
+static VMSTATE_HPET_OFFSET: VMStateDescription = VMStateDescription {
+    name: c_str!("hpet/offset").as_ptr(),
+    version_id: 1,
+    minimum_version_id: 1,
+    needed: Some(hpet_offset_needed),
+    fields: vmstate_fields! {
+        vmstate_of!(HPETState, hpet_offset),
+    },
+    ..Zeroable::ZERO
+};
+
+static VMSTATE_HPET_TIMER: VMStateDescription = VMStateDescription {
+    name: c_str!("hpet_timer").as_ptr(),
+    version_id: 1,
+    minimum_version_id: 1,
+    fields: vmstate_fields! {
+        vmstate_of!(HPETTimer, index),
+        vmstate_of!(HPETTimer, config),
+        vmstate_of!(HPETTimer, cmp),
+        vmstate_of!(HPETTimer, fsb),
+        vmstate_of!(HPETTimer, period),
+        vmstate_of!(HPETTimer, wrap_flag),
+        vmstate_of!(HPETTimer, qemu_timer),
+    },
+    ..Zeroable::ZERO
+};
+
+const VALIDATE_TIMERS_NAME: &CStr = c_str!("num_timers must match");
+
+static VMSTATE_HPET: VMStateDescription = VMStateDescription {
+    name: c_str!("hpet").as_ptr(),
+    version_id: 2,
+    minimum_version_id: 1,
+    pre_save: Some(hpet_pre_save),
+    post_load: Some(hpet_post_load),
+    fields: vmstate_fields! {
+        vmstate_of!(HPETState, config),
+        vmstate_of!(HPETState, int_status),
+        vmstate_of!(HPETState, counter),
+        vmstate_of!(HPETState, num_timers_save).with_version_id(2),
+        vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
+        vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
+    },
+    subsections: vmstate_subsections! {
+        VMSTATE_HPET_RTC_IRQ_LEVEL,
+        VMSTATE_HPET_OFFSET,
+    },
+    ..Zeroable::ZERO
+};
+
 impl DeviceImpl for HPETState {
     fn properties() -> &'static [Property] {
         &HPET_PROPERTIES
     }
 
+    fn vmsd() -> Option<&'static VMStateDescription> {
+        Some(&VMSTATE_HPET)
+    }
+
     const REALIZE: Option<fn(&Self)> = Some(Self::realize);
 }