* [PATCH 0/9] rust/hpet: Initial support for migration
@ 2025-04-14 14:49 Zhao Liu
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
` (9 more replies)
0 siblings, 10 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
Hi all,
This series add the *initial* support for HPET migration.
This is *initial* because the current migration implementation
introduces multiple *unsafe* callbacks (please refer Patch 8).
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.
Test this series with 3 migration cases:
* q35 (Rust HPET) -> q35 (Rust HPET)
* q35 (Rust HPET) -> q35 (C HPET)
* q35 (C HPET) -> q35 (Rust HPET)
Thanks and Best Regards,
Zhao
---
Zhao Liu (9):
rust/vmstate: Support field_exists check in vmstate_struct macro
rust/vmstate: Support varray's num field wrapped in BqlCell
rust/vmstate_test: Test varray with num field wrapped in BqlCell
rust/vmstate_test: Fix typo in
test_vmstate_macro_array_of_pointer_wrapped()
rust/timer: Define NANOSECONDS_PER_SECOND binding as u64
rust/hpet: convert num_timers to u8 type
rust/hpet: convert HPETTimer index to u8 type
rust/hpet: Support migration
rust/hpet: Fix a clippy error
docs/devel/rust.rst | 3 +-
rust/hw/timer/hpet/src/hpet.rs | 189 ++++++++++++++++++++++++---
rust/qemu-api/src/assertions.rs | 30 ++++-
rust/qemu-api/src/cell.rs | 23 ++++
rust/qemu-api/src/timer.rs | 2 +
rust/qemu-api/src/vmstate.rs | 67 +++++-----
rust/qemu-api/tests/vmstate_tests.rs | 45 +++++--
7 files changed, 294 insertions(+), 65 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-16 14:54 ` Paolo Bonzini
2025-04-14 14:49 ` [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell Zhao Liu
` (8 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
Unfortunately, at present it's not possible to have a const
"with_exist_check" method to append test_fn after vmstate_struct (due
to error on "constant functions cannot evaluate destructors" for `F`).
Before the vmstate builder, the only way to support "test_fn" is to
extend vmstate_struct macro to add the such new optional member (and
fortunately, Rust can still parse the current expansion!).
Abstract the previous callback implementation of vmstate_validate into
a separate macro, and moves it before vmstate_struct for vmstate_struct
to call.
Note that there's no need to add any extra flag for a new test_fn added
in the VMStateField.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/vmstate.rs | 67 +++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 1b2b12eadd6e..7d9f3a2ca6f2 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -435,6 +435,38 @@ macro_rules! vmstate_unused {
}};
}
+pub extern "C" fn rust_vms_test_field_exists<T, F: for<'a> FnCall<(&'a T, u8), bool>>(
+ opaque: *mut c_void,
+ version_id: c_int,
+) -> bool {
+ // 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))
+}
+
+pub type VMSFieldExistCb = unsafe extern "C" fn(
+ opaque: *mut std::os::raw::c_void,
+ version_id: std::os::raw::c_int,
+) -> bool;
+
+#[macro_export]
+macro_rules! vmstate_exist_fn {
+ ($struct_name:ty, $test_fn:expr) => {{
+ const fn test_cb_builder__<T, F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>>(
+ _phantom: ::core::marker::PhantomData<F>,
+ ) -> $crate::vmstate::VMSFieldExistCb {
+ let _: () = F::ASSERT_IS_SOME;
+ $crate::vmstate::rust_vms_test_field_exists::<T, F>
+ }
+
+ const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> {
+ ::core::marker::PhantomData
+ }
+ Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn)))
+ }};
+}
+
// FIXME: including the `vmsd` field in a `const` is not possible without
// the const_refs_static feature (stabilized in Rust 1.83.0). Without it,
// it is not possible to use VMS_STRUCT in a transparent manner using
@@ -445,7 +477,7 @@ macro_rules! vmstate_unused {
#[doc(alias = "VMSTATE_STRUCT")]
#[macro_export]
macro_rules! vmstate_struct {
- ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(,)?) => {
+ ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])?, $vmsd:expr, $type:ty $(, $test_fn:expr)? $(,)?) => {
$crate::bindings::VMStateField {
name: ::core::concat!(::core::stringify!($field_name), "\0")
.as_bytes()
@@ -458,6 +490,7 @@ macro_rules! vmstate_struct {
size: ::core::mem::size_of::<$type>(),
flags: $crate::bindings::VMStateFlags::VMS_STRUCT,
vmsd: $vmsd,
+ $(field_exists: $crate::vmstate_exist_fn!($struct_name, $test_fn),)?
..$crate::zeroable::Zeroable::ZERO
} $(.with_varray_flag_unchecked(
$crate::call_func_with_field!(
@@ -514,43 +547,13 @@ macro_rules! vmstate_fields {
}}
}
-pub extern "C" fn rust_vms_test_field_exists<T, F: for<'a> FnCall<(&'a T, u8), bool>>(
- opaque: *mut c_void,
- version_id: c_int,
-) -> bool {
- let owner: &T = unsafe { &*(opaque.cast::<T>()) };
- let version: u8 = version_id.try_into().unwrap();
- // SAFETY: the opaque was passed as a reference to `T`.
- F::call((owner, version))
-}
-
-pub type VMSFieldExistCb = unsafe extern "C" fn(
- opaque: *mut std::os::raw::c_void,
- version_id: std::os::raw::c_int,
-) -> bool;
-
#[doc(alias = "VMSTATE_VALIDATE")]
#[macro_export]
macro_rules! vmstate_validate {
($struct_name:ty, $test_name:expr, $test_fn:expr $(,)?) => {
$crate::bindings::VMStateField {
name: ::std::ffi::CStr::as_ptr($test_name),
- field_exists: {
- const fn test_cb_builder__<
- T,
- F: for<'a> $crate::callbacks::FnCall<(&'a T, u8), bool>,
- >(
- _phantom: ::core::marker::PhantomData<F>,
- ) -> $crate::vmstate::VMSFieldExistCb {
- let _: () = F::ASSERT_IS_SOME;
- $crate::vmstate::rust_vms_test_field_exists::<T, F>
- }
-
- const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> {
- ::core::marker::PhantomData
- }
- Some(test_cb_builder__::<$struct_name, _>(phantom__(&$test_fn)))
- },
+ field_exists: $crate::vmstate_exist_fn!($struct_name, $test_fn),
flags: $crate::bindings::VMStateFlags(
$crate::bindings::VMStateFlags::VMS_MUST_EXIST.0
| $crate::bindings::VMStateFlags::VMS_ARRAY.0,
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-15 10:54 ` Paolo Bonzini
2025-04-14 14:49 ` [PATCH 3/9] rust/vmstate_test: Test varray with " Zhao Liu
` (7 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
Currently, if the `num` field of a varray is not a numeric type, such as
being placed in a wrapper, the array variant of assert_field_type will
fail the check.
HPET currently wraps num_timers in BqlCell<>. Although BqlCell<> is not
necessary from strictly speaking, it makes sense for vmstate to respect
BqlCell.
The failure of assert_field_type is because it cannot convert BqlCell<T>
into usize for use as the index.
Therefore, first, implement `From` trait for common numeric types on
BqlCell<>. Then, abstract the wrapper and non-wrapper cases uniformly
into a `IntoUsize` trait and make assert_field_type to get usize type
index via `IntoUsize` trait.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/assertions.rs | 30 +++++++++++++++++++++++++++++-
rust/qemu-api/src/cell.rs | 23 +++++++++++++++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs
index eb12e9499a72..232cac5b8dba 100644
--- a/rust/qemu-api/src/assertions.rs
+++ b/rust/qemu-api/src/assertions.rs
@@ -22,6 +22,34 @@ impl<T> EqType for T {
type Itself = T;
}
+pub trait IntoUsize {
+ fn into_usize(v: Self) -> usize;
+}
+
+macro_rules! impl_into_usize {
+ ($type:ty) => {
+ impl IntoUsize for $type {
+ fn into_usize(v: Self) -> usize {
+ v.try_into().unwrap()
+ }
+ }
+
+ impl IntoUsize for crate::cell::BqlCell<$type> {
+ fn into_usize(v: Self) -> usize {
+ let tmp: $type = v.try_into().unwrap();
+ tmp.try_into().unwrap()
+ }
+ }
+ };
+}
+
+// vmstate_n_elems() in C side supports such types.
+impl_into_usize!(u8);
+impl_into_usize!(u16);
+impl_into_usize!(i32);
+impl_into_usize!(u32);
+impl_into_usize!(u64);
+
/// Assert that two types are the same.
///
/// # Examples
@@ -101,7 +129,7 @@ fn types_must_be_equal<T, U>(_: T)
T: $crate::assertions::EqType<Itself = U>,
{
}
- let index: usize = v.$num.try_into().unwrap();
+ let index: usize = $crate::assertions::IntoUsize::into_usize(v.$num);
types_must_be_equal::<_, &$ti>(&v.$i[index]);
}
};
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index ab0785a26928..d31bff093707 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -309,6 +309,29 @@ fn from(t: T) -> BqlCell<T> {
}
}
+// Orphan rules don't like something like `impl<T> From<BqlCell<T>> for T`.
+// It's enough to just implement Into for common types.
+macro_rules! impl_into_inner {
+ ($type:ty) => {
+ impl From<BqlCell<$type>> for $type {
+ fn from(c: BqlCell<$type>) -> $type {
+ c.get()
+ }
+ }
+ };
+}
+
+impl_into_inner!(bool);
+impl_into_inner!(i8);
+impl_into_inner!(i16);
+impl_into_inner!(i32);
+impl_into_inner!(i64);
+impl_into_inner!(u8);
+impl_into_inner!(u16);
+impl_into_inner!(u32);
+impl_into_inner!(u64);
+impl_into_inner!(usize);
+
impl<T: fmt::Debug + Copy> fmt::Debug for BqlCell<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.get().fmt(f)
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/9] rust/vmstate_test: Test varray with num field wrapped in BqlCell
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
2025-04-14 14:49 ` [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 4/9] rust/vmstate_test: Fix typo in test_vmstate_macro_array_of_pointer_wrapped() Zhao Liu
` (6 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/tests/vmstate_tests.rs | 41 ++++++++++++++++++++++------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs
index b8d8b45b19de..d1e37c45eea4 100644
--- a/rust/qemu-api/tests/vmstate_tests.rs
+++ b/rust/qemu-api/tests/vmstate_tests.rs
@@ -28,7 +28,7 @@
// - VMSTATE_VARRAY_UINT16_UNSAFE
// - VMSTATE_VARRAY_MULTIPLY
#[repr(C)]
-#[derive(qemu_api_macros::offsets)]
+#[derive(Default, qemu_api_macros::offsets)]
struct FooA {
arr: [u8; FOO_ARRAY_MAX],
num: u16,
@@ -147,8 +147,9 @@ fn test_vmstate_varray_multiply() {
// - VMSTATE_STRUCT_VARRAY_UINT8
// - (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32
// - VMSTATE_ARRAY
+// - VMSTATE_STRUCT_VARRAY_UINT8 with BqlCell wrapper & test_fn
#[repr(C)]
-#[derive(qemu_api_macros::offsets)]
+#[derive(Default, qemu_api_macros::offsets)]
struct FooB {
arr_a: [FooA; FOO_ARRAY_MAX],
num_a: u8,
@@ -158,6 +159,12 @@ struct FooB {
val: bool,
// FIXME: Use Timer array. Now we can't since it's hard to link savevm.c to test.
arr_i64: [i64; FOO_ARRAY_MAX],
+ arr_a_wrap: [FooA; FOO_ARRAY_MAX],
+ num_a_wrap: BqlCell<u32>,
+}
+
+fn validate_foob(_state: &FooB, _version_id: u8) -> bool {
+ true
}
static VMSTATE_FOOB: VMStateDescription = VMStateDescription {
@@ -170,13 +177,14 @@ struct FooB {
vmstate_struct!(FooB, arr_a[0 .. num_a], &VMSTATE_FOOA, FooA).with_version_id(1),
vmstate_struct!(FooB, arr_a_mul[0 .. num_a_mul * 32], &VMSTATE_FOOA, FooA).with_version_id(2),
vmstate_of!(FooB, arr_i64),
+ vmstate_struct!(FooB, arr_a_wrap[0 .. num_a_wrap], &VMSTATE_FOOA, FooA, validate_foob),
},
..Zeroable::ZERO
};
#[test]
fn test_vmstate_bool_v() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
// 1st VMStateField ("val") in VMSTATE_FOOB (corresponding to VMSTATE_BOOL_V)
assert_eq!(
@@ -196,7 +204,7 @@ fn test_vmstate_bool_v() {
#[test]
fn test_vmstate_uint64() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
// 2nd VMStateField ("wrap") in VMSTATE_FOOB (corresponding to VMSTATE_U64)
assert_eq!(
@@ -216,7 +224,7 @@ fn test_vmstate_uint64() {
#[test]
fn test_vmstate_struct_varray_uint8() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
// 3rd VMStateField ("arr_a") in VMSTATE_FOOB (corresponding to
// VMSTATE_STRUCT_VARRAY_UINT8)
@@ -240,7 +248,7 @@ fn test_vmstate_struct_varray_uint8() {
#[test]
fn test_vmstate_struct_varray_uint32_multiply() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
// 4th VMStateField ("arr_a_mul") in VMSTATE_FOOB (corresponding to
// (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32)
@@ -266,7 +274,7 @@ fn test_vmstate_struct_varray_uint32_multiply() {
#[test]
fn test_vmstate_macro_array() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
// 5th VMStateField ("arr_i64") in VMSTATE_FOOB (corresponding to
// VMSTATE_ARRAY)
@@ -283,9 +291,26 @@ fn test_vmstate_macro_array() {
assert_eq!(foo_fields[4].flags, VMStateFlags::VMS_ARRAY);
assert!(foo_fields[4].vmsd.is_null());
assert!(foo_fields[4].field_exists.is_none());
+}
+
+#[test]
+fn test_vmstate_struct_varray_uint8_wrapper() {
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let mut foo_b: FooB = Default::default();
+ let foo_b_p = std::ptr::addr_of_mut!(foo_b).cast::<c_void>();
+
+ // 6th VMStateField ("arr_a_wrap") in VMSTATE_FOOB (corresponding to
+ // VMSTATE_STRUCT_VARRAY_UINT8). Other fields are checked in
+ // test_vmstate_struct_varray_uint8.
+ assert_eq!(
+ unsafe { CStr::from_ptr(foo_fields[5].name) }.to_bytes_with_nul(),
+ b"arr_a_wrap\0"
+ );
+ assert_eq!(foo_fields[5].num_offset, 228);
+ assert!(unsafe { foo_fields[5].field_exists.unwrap()(foo_b_p, 0) });
// The last VMStateField in VMSTATE_FOOB.
- assert_eq!(foo_fields[5].flags, VMStateFlags::VMS_END);
+ assert_eq!(foo_fields[6].flags, VMStateFlags::VMS_END);
}
// =========================== Test VMSTATE_FOOC ===========================
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/9] rust/vmstate_test: Fix typo in test_vmstate_macro_array_of_pointer_wrapped()
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (2 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 3/9] rust/vmstate_test: Test varray with " Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 5/9] rust/timer: Define NANOSECONDS_PER_SECOND binding as u64 Zhao Liu
` (5 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
test_vmstate_macro_array_of_pointer_wrapped() tests the 3rd element, so
fix the index.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/tests/vmstate_tests.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs
index d1e37c45eea4..f7a93117e1a0 100644
--- a/rust/qemu-api/tests/vmstate_tests.rs
+++ b/rust/qemu-api/tests/vmstate_tests.rs
@@ -408,12 +408,12 @@ fn test_vmstate_macro_array_of_pointer_wrapped() {
);
assert_eq!(foo_fields[3].offset, (FOO_ARRAY_MAX + 2) * PTR_SIZE);
assert_eq!(foo_fields[3].num_offset, 0);
- assert_eq!(foo_fields[2].info, unsafe { &vmstate_info_uint8 });
+ assert_eq!(foo_fields[3].info, unsafe { &vmstate_info_uint8 });
assert_eq!(foo_fields[3].version_id, 0);
assert_eq!(foo_fields[3].size, PTR_SIZE);
assert_eq!(foo_fields[3].num, FOO_ARRAY_MAX as i32);
assert_eq!(
- foo_fields[2].flags.0,
+ foo_fields[3].flags.0,
VMStateFlags::VMS_ARRAY.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0
);
assert!(foo_fields[3].vmsd.is_null());
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/9] rust/timer: Define NANOSECONDS_PER_SECOND binding as u64
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (3 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 4/9] rust/vmstate_test: Fix typo in test_vmstate_macro_array_of_pointer_wrapped() Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 6/9] rust/hpet: convert num_timers to u8 type Zhao Liu
` (4 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
NANOSECONDS_PER_SECOND is often used in operations with get_ns(), which
currently returns a u64.
Therefore, define a new NANOSECONDS_PER_SECOND binding is with u64 type
to eliminate unnecessary type conversions (from u32 to u64).
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/timer.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index f0b04ef95d7e..e769f8bc910d 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -121,3 +121,5 @@ pub fn get_ns(&self) -> u64 {
pub const CLOCK_VIRTUAL: ClockType = ClockType {
id: QEMUClockType::QEMU_CLOCK_VIRTUAL,
};
+
+pub const NANOSECONDS_PER_SECOND: u64 = 1000000000;
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/9] rust/hpet: convert num_timers to u8 type
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (4 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 5/9] rust/timer: Define NANOSECONDS_PER_SECOND binding as u64 Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 7/9] rust/hpet: convert HPETTimer index " Zhao Liu
` (3 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
The C version of HPET uses the uint8_t type for num_timers, and usize
type in Rust version will break migration between the C and Rust
versions.
So convert num_timers' type to u8 (consistent with the C version of
HPET) to make it friendly for vmstate support.
Note the commit 7bda68e8e2b0 ("qdev, rust/hpet: fix type of HPET
'timers property") supports the usize type property, but the uint8
property has to be re-supported now.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/hw/timer/hpet/src/hpet.rs | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 3ae3ec25f17a..1afa891362fa 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -12,7 +12,7 @@
use qemu_api::{
bindings::{
address_space_memory, address_space_stl_le, qdev_prop_bit, qdev_prop_bool,
- qdev_prop_uint32, qdev_prop_usize,
+ qdev_prop_uint32, qdev_prop_uint8,
},
c_str,
cell::{BqlCell, BqlRefCell},
@@ -34,9 +34,9 @@
const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
/// Minimum recommended hardware implementation.
-const HPET_MIN_TIMERS: usize = 3;
+const HPET_MIN_TIMERS: u8 = 3;
/// Maximum timers in each timer block.
-const HPET_MAX_TIMERS: usize = 32;
+const HPET_MAX_TIMERS: u8 = 32;
/// Flags that HPETState.flags supports.
const HPET_FLAG_MSI_SUPPORT_SHIFT: usize = 0;
@@ -559,14 +559,19 @@ pub struct HPETState {
/// HPET timer array managed by this timer block.
#[doc(alias = "timer")]
- timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS],
- num_timers: BqlCell<usize>,
+ timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS as usize],
+ num_timers: BqlCell<u8>,
/// Instance id (HPET timer block ID).
hpet_id: BqlCell<usize>,
}
impl HPETState {
+ // Get num_timers with `usize` type, which is useful to play with array index.
+ fn get_num_timers(&self) -> usize {
+ self.num_timers.get().into()
+ }
+
const fn has_msi_flag(&self) -> bool {
self.flags & (1 << HPET_FLAG_MSI_SUPPORT_SHIFT) != 0
}
@@ -628,7 +633,7 @@ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
self.hpet_offset
.set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
- for timer in self.timers.iter().take(self.num_timers.get()) {
+ for timer in self.timers.iter().take(self.get_num_timers()) {
let mut t = timer.borrow_mut();
if t.is_int_enabled() && t.is_int_active() {
@@ -640,7 +645,7 @@ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
// Halt main counter and disable interrupt generation.
self.counter.set(self.get_ticks());
- for timer in self.timers.iter().take(self.num_timers.get()) {
+ for timer in self.timers.iter().take(self.get_num_timers()) {
timer.borrow_mut().del_timer();
}
}
@@ -663,7 +668,7 @@ fn set_int_status_reg(&self, shift: u32, _len: u32, val: u64) {
let new_val = val << shift;
let cleared = new_val & self.int_status.get();
- for (index, timer) in self.timers.iter().take(self.num_timers.get()).enumerate() {
+ for (index, timer) in self.timers.iter().take(self.get_num_timers()).enumerate() {
if cleared & (1 << index) != 0 {
timer.borrow_mut().update_irq(false);
}
@@ -737,7 +742,7 @@ fn realize(&self) {
1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
1 << HPET_CAP_LEG_RT_CAP_SHIFT |
HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
- ((self.num_timers.get() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
+ ((self.get_num_timers() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
(HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
);
@@ -746,7 +751,7 @@ fn realize(&self) {
}
fn reset_hold(&self, _type: ResetType) {
- for timer in self.timers.iter().take(self.num_timers.get()) {
+ for timer in self.timers.iter().take(self.get_num_timers()) {
timer.borrow_mut().reset();
}
@@ -774,7 +779,7 @@ fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode {
GlobalRegister::try_from(addr).map(HPETRegister::Global)
} else {
let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
- if timer_id <= self.num_timers.get() {
+ if timer_id <= self.get_num_timers() {
// TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id)
TimerRegister::try_from(addr & 0x18)
.map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
@@ -859,8 +864,8 @@ impl ObjectImpl for HPETState {
c_str!("timers"),
HPETState,
num_timers,
- unsafe { &qdev_prop_usize },
- usize,
+ unsafe { &qdev_prop_uint8 },
+ u8,
default = HPET_MIN_TIMERS
),
qemu_api::define_property!(
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/9] rust/hpet: convert HPETTimer index to u8 type
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (5 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 6/9] rust/hpet: convert num_timers to u8 type Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 8/9] rust/hpet: Support migration Zhao Liu
` (2 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
The C version of HPET uses the uint8_t type for timer index ("tn"), and
usize type in Rust version will break migration between the C and Rust
versions.
So convert HPETTimer index' type to u8 (consistent with the C version of
HPET) to make it friendly for vmstate support.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/hw/timer/hpet/src/hpet.rs | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 1afa891362fa..dc8a23f29d95 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -184,7 +184,7 @@ fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
- index: usize,
+ index: u8,
qemu_timer: Timer,
/// timer block abstraction containing this timer
state: NonNull<HPETState>,
@@ -210,7 +210,7 @@ pub struct HPETTimer {
}
impl HPETTimer {
- fn init(&mut self, index: usize, state: &HPETState) {
+ fn init(&mut self, index: u8, state: &HPETState) {
*self = HPETTimer {
index,
// SAFETY: the HPETTimer will only be used after the timer
@@ -235,7 +235,7 @@ fn init(&mut self, index: usize, state: &HPETState) {
Timer::NS,
0,
timer_handler,
- &state.timers[self.index],
+ &state.timers[self.index as usize],
)
}
@@ -246,7 +246,7 @@ fn get_state(&self) -> &HPETState {
}
fn is_int_active(&self) -> bool {
- self.get_state().is_timer_int_active(self.index)
+ self.get_state().is_timer_int_active(self.index.into())
}
const fn is_fsb_route_enabled(&self) -> bool {
@@ -611,7 +611,7 @@ fn handle_legacy_irq(&self, irq: u32, level: u32) {
fn init_timer(&self) {
for (index, timer) in self.timers.iter().enumerate() {
- timer.borrow_mut().init(index, self);
+ timer.borrow_mut().init(index.try_into().unwrap(), self);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 8/9] rust/hpet: Support migration
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (6 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 7/9] rust/hpet: convert HPETTimer index " Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-15 12:01 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 9/9] rust/hpet: Fix a clippy error Zhao Liu
2025-04-15 10:24 ` [PATCH 0/9] rust/hpet: Initial support for migration Paolo Bonzini
9 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
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.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 9/9] rust/hpet: Fix a clippy error
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (7 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 8/9] rust/hpet: Support migration Zhao Liu
@ 2025-04-14 14:49 ` Zhao Liu
2025-04-15 10:24 ` [PATCH 0/9] rust/hpet: Initial support for migration Paolo Bonzini
9 siblings, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-14 14:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi, Zhao Liu
Carge clippy complained about:
error: casts from `u8` to `u32` can be expressed infallibly using `From`
So use `From` to convert `u8` to `u32`.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/hw/timer/hpet/src/hpet.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 983f3882f8d3..a3538af14b48 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -357,7 +357,7 @@ fn update_irq(&mut self, set: bool) {
// still operate and generate appropriate status bits, but
// will not cause an interrupt"
self.get_state()
- .update_int_status(self.index as u32, set && self.is_int_level_triggered());
+ .update_int_status(u32::from(self.index), set && self.is_int_level_triggered());
self.set_irq(set);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/9] rust/hpet: Initial support for migration
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
` (8 preceding siblings ...)
2025-04-14 14:49 ` [PATCH 9/9] rust/hpet: Fix a clippy error Zhao Liu
@ 2025-04-15 10:24 ` Paolo Bonzini
9 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-15 10:24 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
On 4/14/25 16:49, Zhao Liu wrote:
> Hi all,
>
> This series add the *initial* support for HPET migration.
>
> This is *initial* because the current migration implementation
> introduces multiple *unsafe* callbacks (please refer Patch 8).
>
> 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.
Merged 4/6/7/9 for now, thanks! I'll reply to patch 2 with a review.
Paolo
> Test this series with 3 migration cases:
> * q35 (Rust HPET) -> q35 (Rust HPET)
> * q35 (Rust HPET) -> q35 (C HPET)
> * q35 (C HPET) -> q35 (Rust HPET)
>
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (9):
> rust/vmstate: Support field_exists check in vmstate_struct macro
> rust/vmstate: Support varray's num field wrapped in BqlCell
> rust/vmstate_test: Test varray with num field wrapped in BqlCell
> rust/vmstate_test: Fix typo in
> test_vmstate_macro_array_of_pointer_wrapped()
> rust/timer: Define NANOSECONDS_PER_SECOND binding as u64
> rust/hpet: convert num_timers to u8 type
> rust/hpet: convert HPETTimer index to u8 type
> rust/hpet: Support migration
> rust/hpet: Fix a clippy error
>
> docs/devel/rust.rst | 3 +-
> rust/hw/timer/hpet/src/hpet.rs | 189 ++++++++++++++++++++++++---
> rust/qemu-api/src/assertions.rs | 30 ++++-
> rust/qemu-api/src/cell.rs | 23 ++++
> rust/qemu-api/src/timer.rs | 2 +
> rust/qemu-api/src/vmstate.rs | 67 +++++-----
> rust/qemu-api/tests/vmstate_tests.rs | 45 +++++--
> 7 files changed, 294 insertions(+), 65 deletions(-)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-14 14:49 ` [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell Zhao Liu
@ 2025-04-15 10:54 ` Paolo Bonzini
2025-04-16 9:43 ` Zhao Liu
2025-05-16 8:25 ` Zhao Liu
0 siblings, 2 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-15 10:54 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
On 4/14/25 16:49, Zhao Liu wrote:
> Currently, if the `num` field of a varray is not a numeric type, such as
> being placed in a wrapper, the array variant of assert_field_type will
> fail the check.
>
> HPET currently wraps num_timers in BqlCell<>. Although BqlCell<> is not
> necessary from strictly speaking, it makes sense for vmstate to respect
> BqlCell.
Dropping BqlCell<> from num_timers is indeed possible. But I agree that
getting BqlCell<> varrays to work is a good thing anyway; then you can
separately decide whether to drop BqlCell<> from num_timers.
> The failure of assert_field_type is because it cannot convert BqlCell<T>
> into usize for use as the index.
>
> Therefore, first, implement `From` trait for common numeric types on
> BqlCell<>. Then, abstract the wrapper and non-wrapper cases uniformly
> into a `IntoUsize` trait and make assert_field_type to get usize type
> index via `IntoUsize` trait.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> rust/qemu-api/src/assertions.rs | 30 +++++++++++++++++++++++++++++-
> rust/qemu-api/src/cell.rs | 23 +++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 1 deletion(-)
I think you can drop the "num=" case of assert_field_type!, and use
something like this macro:
/// Drop everything up to the colon, with the intention that
/// `if_present!` is called inside an optional macro substitution
/// (such as `$(... $arg ...)?` or `$(... $arg ...)*`). This allows
/// expanding `$result` depending on the presence of an argument,
/// even if the argument itself is not included in `$result`.
///
/// # Examples
///
/// ```
/// # use qemu_api::if_present;
/// macro_rules! is_present {
/// ($($cond:expr)?) => {
/// loop {
/// $(if_present!([$cond]: break true;);)?
/// #[allow(unreachable_code)]
/// break false;
/// }
/// }
/// }
///
/// assert!(!is_present!());
/// assert!(is_present!("abc"));
/// ```
#[macro_export]
macro_rules! if_present {
([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
}
to expand the array part of the access:
assert_field_type!(...
$($crate::if_present!([$num]: [0]))?;
);
With this change, assert_field_type! is nicer and at least the trait
you're introducing in assertions.rs goes away...
> +// Orphan rules don't like something like `impl<T> From<BqlCell<T>> for T`.
> +// It's enough to just implement Into for common types.
> +macro_rules! impl_into_inner {
> + ($type:ty) => {
> + impl From<BqlCell<$type>> for $type {
> + fn from(c: BqlCell<$type>) -> $type {
> + c.get()
> + }
> + }
> + };
> +}
... and it's not clear to me whether this is needed with the change
above? Would impl_vmstate_transparent!'s definition of VARRAY_FLAG be
enough?
If not, I *think* you can do a blanket implementation of Into<T> for
BqlCell<T>. Maybe that's nicer, you can decide.
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-14 14:49 ` [PATCH 8/9] rust/hpet: Support migration Zhao Liu
@ 2025-04-15 12:01 ` Zhao Liu
2025-04-15 14:21 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-15 12:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
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()
+ }
+}
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-15 12:01 ` Zhao Liu
@ 2025-04-15 14:21 ` Paolo Bonzini
2025-04-15 17:43 ` Paolo Bonzini
2025-04-16 10:33 ` Zhao Liu
0 siblings, 2 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-15 14:21 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
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()
> + }
> +}
>
>
>
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-15 14:21 ` Paolo Bonzini
@ 2025-04-15 17:43 ` Paolo Bonzini
2025-04-16 10:20 ` Zhao Liu
2025-04-16 10:33 ` Zhao Liu
1 sibling, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-15 17:43 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]
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
[-- Attachment #2: 0001-convert-everything-to-VMStateBuilder.patch --]
[-- Type: text/x-patch, Size: 26517 bytes --]
From d67d96e4da714b1ce6676c642dba28ff589c8902 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 15 Apr 2025 17:29:24 +0200
Subject: [PATCH] convert everything to VMStateBuilder
---
rust/hw/char/pl011/src/device.rs | 17 ++-
rust/hw/char/pl011/src/device_class.rs | 117 ++++++++-----------
rust/hw/timer/hpet/src/hpet.rs | 149 ++++++++++---------------
rust/qemu-api/tests/tests.rs | 17 ++-
rust/qemu-api/tests/vmstate_tests.rs | 139 +++++++++++------------
5 files changed, 197 insertions(+), 242 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index bb2a0f207a5..2f1550708b9 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -172,8 +172,8 @@ impl DeviceImpl for PL011State {
fn properties() -> &'static [Property] {
&device_class::PL011_PROPERTIES
}
- fn vmsd() -> Option<&'static VMStateDescription> {
- Some(&device_class::VMSTATE_PL011)
+ fn vmsd() -> Option<VMStateDescription<Self>> {
+ Some(device_class::VMSTATE_PL011)
}
const REALIZE: Option<fn(&Self)> = Some(Self::realize);
}
@@ -524,6 +524,10 @@ const fn clock_update(&self, _event: ClockEvent) {
/* pl011_trace_baudrate_change(s); */
}
+ pub fn clock_needed(&self) -> bool {
+ self.migrate_clock
+ }
+
fn post_init(&self) {
self.init_mmio(&self.iomem);
for irq in self.interrupts.iter() {
@@ -629,8 +633,13 @@ fn update(&self) {
}
}
- pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
- self.regs.borrow_mut().post_load()
+ pub fn post_load(&self, _version_id: u8) -> i32 {
+ let result = self.regs.borrow_mut().post_load();
+ if result.is_err() {
+ -1
+ } else {
+ 0
+ }
}
}
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index b4d4a7eb432..ab60947813c 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -2,87 +2,66 @@
// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{
- os::raw::{c_int, c_void},
- ptr::NonNull,
-};
-
use qemu_api::{
bindings::{qdev_prop_bool, qdev_prop_chr},
c_str,
prelude::*,
- vmstate::VMStateDescription,
+ vmstate::{VMStateDescription, VMStateDescriptionBuilder},
vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_unused,
- zeroable::Zeroable,
};
use crate::device::{PL011Registers, PL011State};
-extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
- let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
- unsafe { state.as_ref().migrate_clock }
-}
-
/// Migration subsection for [`PL011State`] clock.
-static VMSTATE_PL011_CLOCK: VMStateDescription = VMStateDescription {
- name: c_str!("pl011/clock").as_ptr(),
- version_id: 1,
- minimum_version_id: 1,
- needed: Some(pl011_clock_needed),
- fields: vmstate_fields! {
- vmstate_clock!(PL011State, clock),
- },
- ..Zeroable::ZERO
-};
+static VMSTATE_PL011_CLOCK: VMStateDescription<PL011State> =
+ VMStateDescriptionBuilder::<PL011State>::new()
+ .name(c_str!("pl011/clock"))
+ .version_id(1)
+ .minimum_version_id(1)
+ .needed(&PL011State::clock_needed)
+ .fields(vmstate_fields! {
+ vmstate_clock!(PL011State, clock),
+ })
+ .build();
-extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
- let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
- let result = unsafe { state.as_ref().post_load(version_id as u32) };
- if result.is_err() {
- -1
- } else {
- 0
- }
-}
+static VMSTATE_PL011_REGS: VMStateDescription<PL011Registers> =
+ VMStateDescriptionBuilder::<PL011Registers>::new()
+ .name(c_str!("pl011/regs"))
+ .version_id(2)
+ .minimum_version_id(2)
+ .fields(vmstate_fields! {
+ vmstate_of!(PL011Registers, flags),
+ vmstate_of!(PL011Registers, line_control),
+ vmstate_of!(PL011Registers, receive_status_error_clear),
+ vmstate_of!(PL011Registers, control),
+ vmstate_of!(PL011Registers, dmacr),
+ vmstate_of!(PL011Registers, int_enabled),
+ vmstate_of!(PL011Registers, int_level),
+ vmstate_of!(PL011Registers, read_fifo),
+ vmstate_of!(PL011Registers, ilpr),
+ vmstate_of!(PL011Registers, ibrd),
+ vmstate_of!(PL011Registers, fbrd),
+ vmstate_of!(PL011Registers, ifl),
+ vmstate_of!(PL011Registers, read_pos),
+ vmstate_of!(PL011Registers, read_count),
+ vmstate_of!(PL011Registers, read_trigger),
+ })
+ .build();
-static VMSTATE_PL011_REGS: VMStateDescription = VMStateDescription {
- name: c_str!("pl011/regs").as_ptr(),
- version_id: 2,
- minimum_version_id: 2,
- fields: vmstate_fields! {
- vmstate_of!(PL011Registers, flags),
- vmstate_of!(PL011Registers, line_control),
- vmstate_of!(PL011Registers, receive_status_error_clear),
- vmstate_of!(PL011Registers, control),
- vmstate_of!(PL011Registers, dmacr),
- vmstate_of!(PL011Registers, int_enabled),
- vmstate_of!(PL011Registers, int_level),
- vmstate_of!(PL011Registers, read_fifo),
- vmstate_of!(PL011Registers, ilpr),
- vmstate_of!(PL011Registers, ibrd),
- vmstate_of!(PL011Registers, fbrd),
- vmstate_of!(PL011Registers, ifl),
- vmstate_of!(PL011Registers, read_pos),
- vmstate_of!(PL011Registers, read_count),
- vmstate_of!(PL011Registers, read_trigger),
- },
- ..Zeroable::ZERO
-};
-
-pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
- name: c_str!("pl011").as_ptr(),
- version_id: 2,
- minimum_version_id: 2,
- post_load: Some(pl011_post_load),
- fields: vmstate_fields! {
- vmstate_unused!(core::mem::size_of::<u32>()),
- vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
- },
- subsections: vmstate_subsections! {
- VMSTATE_PL011_CLOCK
- },
- ..Zeroable::ZERO
-};
+pub const VMSTATE_PL011: VMStateDescription<PL011State> =
+ VMStateDescriptionBuilder::<PL011State>::new()
+ .name(c_str!("pl011"))
+ .version_id(2)
+ .minimum_version_id(2)
+ .post_load(&PL011State::post_load)
+ .fields(vmstate_fields! {
+ vmstate_unused!(core::mem::size_of::<u32>()),
+ vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
+ })
+ .subsections(vmstate_subsections! {
+ VMSTATE_PL011_CLOCK
+ })
+ .build();
qemu_api::declare_properties! {
PL011_PROPERTIES,
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 12de2ba59a1..70b6fd620db 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::{VMStateDescription, VMStateDescriptionBuilder},
vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_validate,
- zeroable::Zeroable,
};
use crate::fw_cfg::HPETFwConfig;
@@ -943,105 +941,74 @@ 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()
-}
+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();
-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()
-}
+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();
-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_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
-};
+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),
+ })
+ .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> {
- Some(&VMSTATE_HPET)
+ fn vmsd() -> Option<VMStateDescription<Self>> {
+ Some(VMSTATE_HPET)
}
const REALIZE: Option<fn(&Self)> = Some(Self::realize);
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 99a7aab6fed..f4ff3641c25 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -13,18 +13,17 @@
qdev::{DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
qom::{ObjectImpl, ParentField},
sysbus::SysBusDevice,
- vmstate::VMStateDescription,
- zeroable::Zeroable,
+ vmstate::{VMStateDescription, VMStateDescriptionBuilder},
};
mod vmstate_tests;
// Test that macros can compile.
-pub static VMSTATE: VMStateDescription = VMStateDescription {
- name: c_str!("name").as_ptr(),
- unmigratable: true,
- ..Zeroable::ZERO
-};
+pub const VMSTATE: VMStateDescription<DummyState> =
+ VMStateDescriptionBuilder::<DummyState>::new()
+ .name(c_str!("name"))
+ .unmigratable()
+ .build();
#[derive(qemu_api_macros::offsets)]
#[repr(C)]
@@ -74,8 +73,8 @@ impl DeviceImpl for DummyState {
fn properties() -> &'static [Property] {
&DUMMY_PROPERTIES
}
- fn vmsd() -> Option<&'static VMStateDescription> {
- Some(&VMSTATE)
+ fn vmsd() -> Option<VMStateDescription<Self>> {
+ Some(VMSTATE)
}
}
diff --git a/rust/qemu-api/tests/vmstate_tests.rs b/rust/qemu-api/tests/vmstate_tests.rs
index f7a93117e1a..fa35b9addec 100644
--- a/rust/qemu-api/tests/vmstate_tests.rs
+++ b/rust/qemu-api/tests/vmstate_tests.rs
@@ -12,9 +12,8 @@
c_str,
cell::{BqlCell, Opaque},
impl_vmstate_forward,
- vmstate::{VMStateDescription, VMStateField},
+ vmstate::{VMStateDescription, VMStateDescriptionBuilder, VMStateField},
vmstate_fields, vmstate_of, vmstate_struct, vmstate_unused, vmstate_validate,
- zeroable::Zeroable,
};
const FOO_ARRAY_MAX: usize = 3;
@@ -37,22 +36,22 @@ struct FooA {
elem: i8,
}
-static VMSTATE_FOOA: VMStateDescription = VMStateDescription {
- name: c_str!("foo_a").as_ptr(),
- version_id: 1,
- minimum_version_id: 1,
- fields: vmstate_fields! {
- vmstate_of!(FooA, elem),
- vmstate_unused!(size_of::<i64>()),
- vmstate_of!(FooA, arr[0 .. num]).with_version_id(0),
- vmstate_of!(FooA, arr_mul[0 .. num_mul * 16]),
- },
- ..Zeroable::ZERO
-};
+static VMSTATE_FOOA: VMStateDescription<FooA> =
+ VMStateDescriptionBuilder::<FooA>::new()
+ .name(c_str!("foo_a"))
+ .version_id(1)
+ .minimum_version_id(1)
+ .fields(vmstate_fields! {
+ vmstate_of!(FooA, elem),
+ vmstate_unused!(size_of::<i64>()),
+ vmstate_of!(FooA, arr[0 .. num]).with_version_id(0),
+ vmstate_of!(FooA, arr_mul[0 .. num_mul * 16]),
+ })
+ .build();
#[test]
fn test_vmstate_uint16() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.get_vmsd_ref().fields, 5) };
// 1st VMStateField ("elem") in VMSTATE_FOOA (corresponding to VMSTATE_UINT16)
assert_eq!(
@@ -72,7 +71,7 @@ fn test_vmstate_uint16() {
#[test]
fn test_vmstate_unused() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.get_vmsd_ref().fields, 5) };
// 2nd VMStateField ("unused") in VMSTATE_FOOA (corresponding to VMSTATE_UNUSED)
assert_eq!(
@@ -92,7 +91,7 @@ fn test_vmstate_unused() {
#[test]
fn test_vmstate_varray_uint16_unsafe() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.get_vmsd_ref().fields, 5) };
// 3rd VMStateField ("arr") in VMSTATE_FOOA (corresponding to
// VMSTATE_VARRAY_UINT16_UNSAFE)
@@ -113,7 +112,7 @@ fn test_vmstate_varray_uint16_unsafe() {
#[test]
fn test_vmstate_varray_multiply() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.fields, 5) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOA.get_vmsd_ref().fields, 5) };
// 4th VMStateField ("arr_mul") in VMSTATE_FOOA (corresponding to
// VMSTATE_VARRAY_MULTIPLY)
@@ -167,24 +166,24 @@ fn validate_foob(_state: &FooB, _version_id: u8) -> bool {
true
}
-static VMSTATE_FOOB: VMStateDescription = VMStateDescription {
- name: c_str!("foo_b").as_ptr(),
- version_id: 2,
- minimum_version_id: 1,
- fields: vmstate_fields! {
- vmstate_of!(FooB, val).with_version_id(2),
- vmstate_of!(FooB, wrap),
- vmstate_struct!(FooB, arr_a[0 .. num_a], &VMSTATE_FOOA, FooA).with_version_id(1),
- vmstate_struct!(FooB, arr_a_mul[0 .. num_a_mul * 32], &VMSTATE_FOOA, FooA).with_version_id(2),
- vmstate_of!(FooB, arr_i64),
- vmstate_struct!(FooB, arr_a_wrap[0 .. num_a_wrap], &VMSTATE_FOOA, FooA, validate_foob),
- },
- ..Zeroable::ZERO
-};
+static VMSTATE_FOOB: VMStateDescription<FooB> =
+ VMStateDescriptionBuilder::<FooB>::new()
+ .name(c_str!("foo_b"))
+ .version_id(2)
+ .minimum_version_id(1)
+ .fields(vmstate_fields! {
+ vmstate_of!(FooB, val).with_version_id(2),
+ vmstate_of!(FooB, wrap),
+ vmstate_struct!(FooB, arr_a[0 .. num_a], &VMSTATE_FOOA, FooA).with_version_id(1),
+ vmstate_struct!(FooB, arr_a_mul[0 .. num_a_mul * 32], &VMSTATE_FOOA, FooA).with_version_id(2),
+ vmstate_of!(FooB, arr_i64),
+ vmstate_struct!(FooB, arr_a_wrap[0 .. num_a_wrap], &VMSTATE_FOOA, FooA, validate_foob),
+ })
+ .build();
#[test]
fn test_vmstate_bool_v() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
// 1st VMStateField ("val") in VMSTATE_FOOB (corresponding to VMSTATE_BOOL_V)
assert_eq!(
@@ -204,7 +203,7 @@ fn test_vmstate_bool_v() {
#[test]
fn test_vmstate_uint64() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
// 2nd VMStateField ("wrap") in VMSTATE_FOOB (corresponding to VMSTATE_U64)
assert_eq!(
@@ -224,7 +223,7 @@ fn test_vmstate_uint64() {
#[test]
fn test_vmstate_struct_varray_uint8() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
// 3rd VMStateField ("arr_a") in VMSTATE_FOOB (corresponding to
// VMSTATE_STRUCT_VARRAY_UINT8)
@@ -242,13 +241,13 @@ fn test_vmstate_struct_varray_uint8() {
foo_fields[2].flags.0,
VMStateFlags::VMS_STRUCT.0 | VMStateFlags::VMS_VARRAY_UINT8.0
);
- assert_eq!(foo_fields[2].vmsd, &VMSTATE_FOOA);
+ assert_eq!(foo_fields[2].vmsd, VMSTATE_FOOA.get_vmsd_ref());
assert!(foo_fields[2].field_exists.is_none());
}
#[test]
fn test_vmstate_struct_varray_uint32_multiply() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
// 4th VMStateField ("arr_a_mul") in VMSTATE_FOOB (corresponding to
// (no C version) MULTIPLY variant of VMSTATE_STRUCT_VARRAY_UINT32)
@@ -268,13 +267,13 @@ fn test_vmstate_struct_varray_uint32_multiply() {
| VMStateFlags::VMS_VARRAY_UINT32.0
| VMStateFlags::VMS_MULTIPLY_ELEMENTS.0
);
- assert_eq!(foo_fields[3].vmsd, &VMSTATE_FOOA);
+ assert_eq!(foo_fields[3].vmsd, VMSTATE_FOOA.get_vmsd_ref());
assert!(foo_fields[3].field_exists.is_none());
}
#[test]
fn test_vmstate_macro_array() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
// 5th VMStateField ("arr_i64") in VMSTATE_FOOB (corresponding to
// VMSTATE_ARRAY)
@@ -295,7 +294,7 @@ fn test_vmstate_macro_array() {
#[test]
fn test_vmstate_struct_varray_uint8_wrapper() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.fields, 7) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOB.get_vmsd_ref().fields, 7) };
let mut foo_b: FooB = Default::default();
let foo_b_p = std::ptr::addr_of_mut!(foo_b).cast::<c_void>();
@@ -332,26 +331,28 @@ struct FooC {
arr_ptr_wrap: FooCWrapper,
}
-static VMSTATE_FOOC: VMStateDescription = VMStateDescription {
- name: c_str!("foo_c").as_ptr(),
- version_id: 3,
- minimum_version_id: 1,
- fields: vmstate_fields! {
- vmstate_of!(FooC, ptr).with_version_id(2),
- // FIXME: Currently vmstate_struct doesn't support the pointer to structure.
- // VMSTATE_STRUCT_POINTER: vmstate_struct!(FooC, ptr_a, VMSTATE_FOOA, NonNull<FooA>)
- vmstate_unused!(size_of::<NonNull<FooA>>()),
- vmstate_of!(FooC, arr_ptr),
- vmstate_of!(FooC, arr_ptr_wrap),
- },
- ..Zeroable::ZERO
-};
+unsafe impl Sync for FooC {}
+
+static VMSTATE_FOOC: VMStateDescription<FooC> =
+ VMStateDescriptionBuilder::<FooC>::new()
+ .name(c_str!("foo_c"))
+ .version_id(3)
+ .minimum_version_id(1)
+ .fields(vmstate_fields! {
+ vmstate_of!(FooC, ptr).with_version_id(2),
+ // FIXME: Currently vmstate_struct doesn't support the pointer to structure.
+ // VMSTATE_STRUCT_POINTER: vmstate_struct!(FooC, ptr_a, VMSTATE_FOOA, NonNull<FooA>)
+ vmstate_unused!(size_of::<NonNull<FooA>>()),
+ vmstate_of!(FooC, arr_ptr),
+ vmstate_of!(FooC, arr_ptr_wrap),
+ })
+ .build();
const PTR_SIZE: usize = size_of::<*mut ()>();
#[test]
fn test_vmstate_pointer() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.get_vmsd_ref().fields, 6) };
// 1st VMStateField ("ptr") in VMSTATE_FOOC (corresponding to VMSTATE_POINTER)
assert_eq!(
@@ -374,7 +375,7 @@ fn test_vmstate_pointer() {
#[test]
fn test_vmstate_macro_array_of_pointer() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.get_vmsd_ref().fields, 6) };
// 3rd VMStateField ("arr_ptr") in VMSTATE_FOOC (corresponding to
// VMSTATE_ARRAY_OF_POINTER)
@@ -398,7 +399,7 @@ fn test_vmstate_macro_array_of_pointer() {
#[test]
fn test_vmstate_macro_array_of_pointer_wrapped() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.fields, 6) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOC.get_vmsd_ref().fields, 6) };
// 4th VMStateField ("arr_ptr_wrap") in VMSTATE_FOOC (corresponding to
// VMSTATE_ARRAY_OF_POINTER)
@@ -447,21 +448,21 @@ fn validate_food_2(_state: &FooD, _version_id: u8) -> bool {
true
}
-static VMSTATE_FOOD: VMStateDescription = VMStateDescription {
- name: c_str!("foo_d").as_ptr(),
- version_id: 3,
- minimum_version_id: 1,
- fields: vmstate_fields! {
- vmstate_validate!(FooD, c_str!("foo_d_0"), FooD::validate_food_0),
- vmstate_validate!(FooD, c_str!("foo_d_1"), FooD::validate_food_1),
- vmstate_validate!(FooD, c_str!("foo_d_2"), validate_food_2),
- },
- ..Zeroable::ZERO
-};
+static VMSTATE_FOOD: VMStateDescription<FooD> =
+ VMStateDescriptionBuilder::<FooD>::new()
+ .name(c_str!("foo_d"))
+ .version_id(3)
+ .minimum_version_id(1)
+ .fields(vmstate_fields! {
+ vmstate_validate!(FooD, c_str!("foo_d_0"), FooD::validate_food_0),
+ vmstate_validate!(FooD, c_str!("foo_d_1"), FooD::validate_food_1),
+ vmstate_validate!(FooD, c_str!("foo_d_2"), validate_food_2),
+ })
+ .build();
#[test]
fn test_vmstate_validate() {
- let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOD.fields, 4) };
+ let foo_fields: &[VMStateField] = unsafe { slice::from_raw_parts(VMSTATE_FOOD.get_vmsd_ref().fields, 4) };
let mut foo_d = FooD;
let foo_d_p = std::ptr::addr_of_mut!(foo_d).cast::<c_void>();
--
2.49.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-15 10:54 ` Paolo Bonzini
@ 2025-04-16 9:43 ` Zhao Liu
2025-04-16 12:34 ` Zhao Liu
2025-05-16 8:25 ` Zhao Liu
1 sibling, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-16 9:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
On Tue, Apr 15, 2025 at 12:54:51PM +0200, Paolo Bonzini wrote:
> Date: Tue, 15 Apr 2025 12:54:51 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped
> in BqlCell
>
> On 4/14/25 16:49, Zhao Liu wrote:
> > Currently, if the `num` field of a varray is not a numeric type, such as
> > being placed in a wrapper, the array variant of assert_field_type will
> > fail the check.
> >
> > HPET currently wraps num_timers in BqlCell<>. Although BqlCell<> is not
> > necessary from strictly speaking, it makes sense for vmstate to respect
> > BqlCell.
>
> Dropping BqlCell<> from num_timers is indeed possible.
I can move the num_timers adjustment from realize() into init().
> But I agree that getting BqlCell<> varrays to work is a good thing anyway;
While num_timers can get out of BqlCell<>, num_timers_save is still
needed to stay in BqlCell<>, since I understand pre_save - as a callback
of the vmstate - still needs the bql protection.
So this patch is still necessary to support migration for HPET.
> then you can separately decide whether to drop BqlCell<> from num_timers.
Yes. In the next version, I can drop BqlCell<> and adjust the C version
as well. But then I can't update the document at the same time, because
the document needs to list the synchronized commit ID. I can update the
document after the HPET migration is able to be merged.
> > The failure of assert_field_type is because it cannot convert BqlCell<T>
> > into usize for use as the index.
> >
> > Therefore, first, implement `From` trait for common numeric types on
> > BqlCell<>. Then, abstract the wrapper and non-wrapper cases uniformly
> > into a `IntoUsize` trait and make assert_field_type to get usize type
> > index via `IntoUsize` trait.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > rust/qemu-api/src/assertions.rs | 30 +++++++++++++++++++++++++++++-
> > rust/qemu-api/src/cell.rs | 23 +++++++++++++++++++++++
> > 2 files changed, 52 insertions(+), 1 deletion(-)
>
> I think you can drop the "num=" case of assert_field_type!, and use
> something like this macro:
>
> /// Drop everything up to the colon, with the intention that
> /// `if_present!` is called inside an optional macro substitution
> /// (such as `$(... $arg ...)?` or `$(... $arg ...)*`). This allows
> /// expanding `$result` depending on the presence of an argument,
> /// even if the argument itself is not included in `$result`.
> ///
> /// # Examples
> ///
> /// ```
> /// # use qemu_api::if_present;
> /// macro_rules! is_present {
I understand this is_present could have another name to avoid confusion
with our real is_present macro.
> /// ($($cond:expr)?) => {
> /// loop {
> /// $(if_present!([$cond]: break true;);)?
> /// #[allow(unreachable_code)]
> /// break false;
> /// }
> /// }
> /// }
> ///
> /// assert!(!is_present!());
> /// assert!(is_present!("abc"));
> /// ```
> #[macro_export]
> macro_rules! if_present {
> ([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
> }
>
> to expand the array part of the access:
>
> assert_field_type!(...
> $($crate::if_present!([$num]: [0]))?;
This example remind me that I introduced a bug into array part:
let index: usize = v.$num.try_into().unwrap();
types_must_be_equal::<_, &$ti>(&v.$i[index]);
In the current code, actually it accesses v[num], but when num
stores the length of the whole array, it will cause index out of bounds.
So for current code, at least it should access `v.i[num - 1]`:
let index: usize = v.$num.try_into().unwrap() - 1; // access the last element.
types_must_be_equal::<_, &$ti>(&v.$i[index]);
> );
>
> With this change, assert_field_type! is nicer and at least the trait you're
> introducing in assertions.rs goes away...
Yes! Great idea.
Then with your help, we could integrate the array part like:
#[macro_export]
macro_rules! if_present {
([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
}
...
#[macro_export]
macro_rules! assert_field_type {
($t:ty, $i:tt, $ti:ty $(, $num:ident)?) => {
const _: () = {
#[allow(unused)]
fn assert_field_type(v: $t) {
fn types_must_be_equal<T, U>(_: T)
where
T: $crate::assertions::EqType<Itself = U>,
{
}
let access = v.$i$($crate::if_present!([$num]: [v.$num - 1])])?;
types_must_be_equal::<_, $ti>(access);
}
};
};
}
> > +// Orphan rules don't like something like `impl<T> From<BqlCell<T>> for T`.
> > +// It's enough to just implement Into for common types.
> > +macro_rules! impl_into_inner {
> > + ($type:ty) => {
> > + impl From<BqlCell<$type>> for $type {
> > + fn from(c: BqlCell<$type>) -> $type {
> > + c.get()
> > + }
> > + }
> > + };
> > +}
>
> ... and it's not clear to me whether this is needed with the change above?
> Would impl_vmstate_transparent!'s definition of VARRAY_FLAG be enough?
If I change the array part like (the change is needed because: cannot
subtract `{integer}` from `BqlCell<u8>`):
- let access = v.$i$([$crate::if_present!([$num]: v.$num) - 1])?;
+ let access = v.$i$([$crate::if_present!([$num]: v.$num).into() - 1])?;
Then there'll be an error:
85 | macro_rules! assert_field_type {
| ------------------------------ in this expansion of `$crate::assert_field_type!` (#2)
...
96 | let access = v.$i$([$crate::if_present!([$num]: v.$num).into() - 1])?;
| ^^^^ cannot infer type
This is because I want to also check whether "num" would cause index out
of bounds. If we just check the [0] element, then everything is OK...
> If not, I *think* you can do a blanket implementation of Into<T> for
> BqlCell<T>. Maybe that's nicer, you can decide.
I tired this way, but there's 2 difficulities:
* Into<T> for BqlCell<T> will violate coherence rules:
error[E0119]: conflicting implementations of trait `Into<_>` for type `cell::BqlCell<_>`
--> qemu-api/src/cell.rs:312:1
|
312 | impl<T> Into<T> for BqlCell<T> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `core`:
- impl<T, U> Into<U> for T
where U: From<T>;
* As index, we need to convert BqlCell<T> into T and then convert T
into usize. :-(
Do you think there is a better way to check array[num -1]? (array's
len() method also returns usize).
Or do you think whether it's necessary to check array[num -1]?
(referring to C version, for example, VMSTATE_STRUCT_VARRAY_UINT8, it
doesn't check the array's out of bounds case.)
Thanks,
Zhao
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-15 17:43 ` Paolo Bonzini
@ 2025-04-16 10:20 ` Zhao Liu
2025-04-16 14:40 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-16 10:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-15 14:21 ` Paolo Bonzini
2025-04-15 17:43 ` Paolo Bonzini
@ 2025-04-16 10:33 ` Zhao Liu
1 sibling, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-04-16 10:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-16 9:43 ` Zhao Liu
@ 2025-04-16 12:34 ` Zhao Liu
2025-04-16 14:24 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Zhao Liu @ 2025-04-16 12:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
> > #[macro_export]
> > macro_rules! if_present {
> > ([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
> > }
> >
> > to expand the array part of the access:
> >
> > assert_field_type!(...
> > $($crate::if_present!([$num]: [0]))?;
>
> This example remind me that I introduced a bug into array part:
>
> let index: usize = v.$num.try_into().unwrap();
> types_must_be_equal::<_, &$ti>(&v.$i[index]);
>
> In the current code, actually it accesses v[num], but when num
> stores the length of the whole array, it will cause index out of bounds.
>
> So for current code, at least it should access `v.i[num - 1]`:
>
> let index: usize = v.$num.try_into().unwrap() - 1; // access the last element.
> types_must_be_equal::<_, &$ti>(&v.$i[index]);
I realize that my thinking was wrong here! The `v` (with specific type)
isn't a valid instance, and the variable `num` being passed isn't
correctly initialized. Therefore, checking `num`'s value here is
meaningless; it's enough to just check if the type matches!
> > );
> >
> > With this change, assert_field_type! is nicer and at least the trait you're
> > introducing in assertions.rs goes away...
>
> Yes! Great idea.
>
> Then with your help, we could integrate the array part like:
>
> #[macro_export]
> macro_rules! if_present {
> ([$($cond:tt)*]: $($result:tt)*) => { $($result)* };
> }
>
> ...
>
> #[macro_export]
> macro_rules! assert_field_type {
> ($t:ty, $i:tt, $ti:ty $(, $num:ident)?) => {
> const _: () = {
> #[allow(unused)]
> fn assert_field_type(v: $t) {
> fn types_must_be_equal<T, U>(_: T)
> where
> T: $crate::assertions::EqType<Itself = U>,
> {
> }
>
> let access = v.$i$($crate::if_present!([$num]: [v.$num - 1])])?;
So, the correct code should just check array[0] as you said:
let access = v.$i$($crate::if_present!([$num]: [0])])?;
Based on this, there's no need for anything else such as `Into`.
> types_must_be_equal::<_, $ti>(access);
> }
> };
> };
> }
Thanks,
Zhao
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-16 12:34 ` Zhao Liu
@ 2025-04-16 14:24 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-16 14:24 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
Il mer 16 apr 2025, 14:14 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > let access = v.$i$($crate::if_present!([$num]: [v.$num -
> 1])])?;
>
> So, the correct code should just check array[0] as you said:
>
> let access = v.$i$($crate::if_present!([$num]: [0])])?;
>
Except I think the if_present! call would not be in assert_field_type; it
would be in the caller, i.e. vmstate.rs.
Paolo
Based on this, there's no need for anything else such as `Into`.
>
> > types_must_be_equal::<_, $ti>(access);
> > }
> > };
> > };
> > }
>
> Thanks,
> Zhao
>
>
[-- Attachment #2: Type: text/html, Size: 1394 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/9] rust/hpet: Support migration
2025-04-16 10:20 ` Zhao Liu
@ 2025-04-16 14:40 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-16 14:40 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]
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
[-- Attachment #2: Type: text/html, Size: 5038 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
@ 2025-04-16 14:54 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2025-04-16 14:54 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust, Dapeng Mi
On Mon, Apr 14, 2025 at 4:29 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Unfortunately, at present it's not possible to have a const
> "with_exist_check" method to append test_fn after vmstate_struct (due
> to error on "constant functions cannot evaluate destructors" for `F`).
Nothing that std::mem::forget() can't fix. ;) We know that the
destructor is a no-op because FnCall is only implemented on zero-sized
functions.
Another problem with such a method, however, is that you need
VMStateField<T> for it to be type-safe. Implementing it on
bindings::VMStateField gives no guarantee that the type of the
argument to the function is the correct one. So this patch is okay
too, but perhaps add a comment (not necessarily a doc comment)
mentioning that VMStateField<T> is planned in order to remove the
extra argument?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell
2025-04-15 10:54 ` Paolo Bonzini
2025-04-16 9:43 ` Zhao Liu
@ 2025-05-16 8:25 ` Zhao Liu
1 sibling, 0 replies; 23+ messages in thread
From: Zhao Liu @ 2025-05-16 8:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Dapeng Mi
> > HPET currently wraps num_timers in BqlCell<>. Although BqlCell<> is not
> > necessary from strictly speaking, it makes sense for vmstate to respect
> > BqlCell.
>
> Dropping BqlCell<> from num_timers is indeed possible.
Hi Paolo,
I would like to further discuss whether there's any safe issues.
num_timers is a property:
qemu_api::define_property!(
c"timers",
HPETState,
num_timers,
unsafe { &qdev_prop_uint8 },
u8,
default = HPET_MIN_TIMERS
),
Then this means someone could set this property in C side or Rust side
by:
DeviceState *hpet = qdev_new(TYPE_HPET);
qdev_prop_set_uint8(hpet, "timers", 8);
(Though we haven't provide safe interface at Rust side to set property.)
Whatever this happens at C side or Rust side, this depends on QOM core
code (in C) to overwrite the HPETState::num_timers directly.
Then after the call to qdev_prop_set_uint8() starts, all subsequent
processes happen on the C side, so even though the rewriting of num_timers
is runtime, there are no additional safety considerations because it
doesn't cross FFI boundaries. Am I understanding this correctly?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-05-16 8:04 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-14 14:49 [PATCH 0/9] rust/hpet: Initial support for migration Zhao Liu
2025-04-14 14:49 ` [PATCH 1/9] rust/vmstate: Support field_exists check in vmstate_struct macro Zhao Liu
2025-04-16 14:54 ` Paolo Bonzini
2025-04-14 14:49 ` [PATCH 2/9] rust/vmstate: Support varray's num field wrapped in BqlCell Zhao Liu
2025-04-15 10:54 ` Paolo Bonzini
2025-04-16 9:43 ` Zhao Liu
2025-04-16 12:34 ` Zhao Liu
2025-04-16 14:24 ` Paolo Bonzini
2025-05-16 8:25 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 3/9] rust/vmstate_test: Test varray with " Zhao Liu
2025-04-14 14:49 ` [PATCH 4/9] rust/vmstate_test: Fix typo in test_vmstate_macro_array_of_pointer_wrapped() Zhao Liu
2025-04-14 14:49 ` [PATCH 5/9] rust/timer: Define NANOSECONDS_PER_SECOND binding as u64 Zhao Liu
2025-04-14 14:49 ` [PATCH 6/9] rust/hpet: convert num_timers to u8 type Zhao Liu
2025-04-14 14:49 ` [PATCH 7/9] rust/hpet: convert HPETTimer index " Zhao Liu
2025-04-14 14:49 ` [PATCH 8/9] rust/hpet: Support migration Zhao Liu
2025-04-15 12:01 ` Zhao Liu
2025-04-15 14:21 ` Paolo Bonzini
2025-04-15 17:43 ` Paolo Bonzini
2025-04-16 10:20 ` Zhao Liu
2025-04-16 14:40 ` Paolo Bonzini
2025-04-16 10:33 ` Zhao Liu
2025-04-14 14:49 ` [PATCH 9/9] rust/hpet: Fix a clippy error Zhao Liu
2025-04-15 10:24 ` [PATCH 0/9] rust/hpet: Initial support for migration Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).