Message ID | 20250414144943.1112885-9-zhao1.liu@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | rust/hpet: Initial support for migration | expand |
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() + } +}
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() > + } > +} > > > > >
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 --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); }
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(-)