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(-)
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);
}
--
2.34.1Hi 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()
+ }
+}
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()
> + }
> +}
>
>
>
>
>
> > 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.
Yes, I have to use c_str! in name().
> > 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 agree, additional consideration is needed here. Currently it is
vmstate_fields! that limits changes to vmstate_of!.
> 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.
Sure, will fix.
> > + 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!).
I see and will fix.
> (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).
I didn't have a deep understanding of this previously :-(. I'll take a
closer look at this.
Thanks,
Zhao
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
On Tue, Apr 15, 2025 at 07:43:00PM +0200, Paolo Bonzini wrote:
> Date: Tue, 15 Apr 2025 19:43:00 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 8/9] rust/hpet: Support migration
>
> 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.
Thanks so much, that's a good idea!
About `Box::into_raw(Box::new(vmsd))`, do you think it's necessary to use
Box::leak(Box::new(*))? (though the Box<> isn't actively dropped during
the class's existence)
pub fn class_init<T: DeviceImpl>(&mut self) {
...
if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
let static_vmsd: &'static mut bindings::VMStateDescription = Box::leak(Box::new(vmsd.get_vmsd()));
self.vmsd = static_vmsd;
}
}
> 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)
Sure!
> 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).
Thank you again! From my initial thoughts: Splitting is also possible,
but it requires first renaming VMStateDescription<T> to
VMStateDescriptionWrapper<T>, then replacing it in pl011 and test (and
hpet) one by one, and finally renaming it back to VMStateDescription<T>.
If you prefer this approach, I can help you split your patch below.
> +const 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),
And it seems like you don't oppose the hack in patch 1? ;-)
> + })
> + .subsections(vmstate_subsections!(
> + VMSTATE_HPET_RTC_IRQ_LEVEL,
> + VMSTATE_HPET_OFFSET,
> + ))
> + .build();
Thanks,
Zhao
Il mer 16 apr 2025, 12:00 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> 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.
Thanks so much, that's a good idea!
About `Box::into_raw(Box::new(vmsd))`, do you think it's necessary
to use
Box::leak(Box::new(*))? (though the Box<> isn't actively dropped during
the class's existence)
It's the same; leak and into_raw only differ in the return type. You can
use leak if you prefer.
> 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).
Thank you again! From my initial thoughts: Splitting is also possible,
but it requires first renaming VMStateDescription<T> to
VMStateDescriptionWrapper<T>, then replacing it in pl011 and test (and
hpet) one by one, and finally renaming it back to VMStateDescription<T>.
If you prefer this approach, I can help you split your patch below.
Or maybe first you keepvmsd() return
Option<&bindings::VMStateDescription> and stick a get_vmsd_ref() in "fn
vmsd()", then in a final patch you make it return
Option<VMStateDescription<T>> and introduce the Box::leak(Box::new(...))
trick.
> > + vmstate_struct!(HPETState, timers[0 .. num_timers],
> &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>,
> HPETState::validate_num_timers).with_version_id(0),
>
> And it seems like you don't oppose the hack in patch 1? ;-)
It's not bad, but I haven't looked at why it's needed yet (i.e. why a
constant function would be evaluating a destructor).
Paolo
© 2016 - 2025 Red Hat, Inc.