Scalar types are those that have their own VMStateInfo.  This poses
a problem in that references to VMStateInfo can only be included in
associated consts starting with Rust 1.83.0, when the const_refs_static
was stabilized.  Removing the requirement is done by placing a limited
list of VMStateInfos in an enum, and going from enum to &VMStateInfo
only when building the VMStateField.

The same thing cannot be done with VMS_STRUCT because the set of
VMStateDescriptions extends to structs defined by the devices.
Therefore, structs and cells cannot yet use vmstate_of!.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/vmstate.rs | 125 ++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 2 deletions(-)

diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 49d0a3c31d4..edd0cbff162 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -21,8 +21,11 @@
 
 use core::{marker::PhantomData, mem, ptr::NonNull};
 
-pub use crate::bindings::{VMStateDescription, VMStateField};
 use crate::bindings::VMStateFlags;
+pub use crate::{
+    bindings::{self, VMStateDescription, VMStateField},
+    zeroable::Zeroable,
+};
 
 /// This macro is used to call a function with a generic argument bound
 /// to the type of a field.  The function must take a
@@ -58,6 +61,70 @@ const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> { ::core::marker:
     };
 }
 
+/// Workaround for lack of `const_refs_static`: references to global variables
+/// can be included in a `static`, but not in a `const`; unfortunately, this
+/// is exactly what would go in the `VMStateField`'s `info` member.
+///
+/// This enum contains the contents of the `VMStateField`'s `info` member,
+/// but as an `enum` instead of a pointer.
+#[allow(non_camel_case_types)]
+pub enum VMStateFieldType {
+    null,
+    vmstate_info_bool,
+    vmstate_info_int8,
+    vmstate_info_int16,
+    vmstate_info_int32,
+    vmstate_info_int64,
+    vmstate_info_uint8,
+    vmstate_info_uint16,
+    vmstate_info_uint32,
+    vmstate_info_uint64,
+    vmstate_info_timer,
+}
+
+/// Workaround for lack of `const_refs_static`.  Converts a `VMStateFieldType`
+/// to a `*const VMStateInfo`, for inclusion in a `VMStateField`.
+#[macro_export]
+macro_rules! info_enum_to_ref {
+    ($e:expr) => {
+        unsafe {
+            match $e {
+                $crate::vmstate::VMStateFieldType::null => ::core::ptr::null(),
+                $crate::vmstate::VMStateFieldType::vmstate_info_bool => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_bool)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_int8 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_int8)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_int16 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_int16)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_int32 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_int64 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_int64)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_uint8 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint8)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_uint16 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint16)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_uint32 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint32)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_uint64 => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_uint64)
+                }
+                $crate::vmstate::VMStateFieldType::vmstate_info_timer => {
+                    ::core::ptr::addr_of!($crate::bindings::vmstate_info_timer)
+                }
+            }
+        }
+    };
+}
+
 /// A trait for types that can be included in a device's migration stream.  It
 /// provides the base contents of a `VMStateField` (minus the name and offset).
 ///
@@ -66,6 +133,12 @@ const fn phantom__<T>(_: &T) -> ::core::marker::PhantomData<T> { ::core::marker:
 /// The contents of this trait go straight into structs that are parsed by C
 /// code and used to introspect into other structs.  Be careful.
 pub unsafe trait VMState {
+    /// The `info` member of a `VMStateField` is a pointer and as such cannot
+    /// yet be included in the [`BASE`](VMState::BASE) associated constant;
+    /// this is only allowed by Rust 1.83.0 and newer.  For now, include the
+    /// member as an enum which is stored in a separate constant.
+    const SCALAR_TYPE: VMStateFieldType = VMStateFieldType::null;
+
     /// The base contents of a `VMStateField` (minus the name and offset) for
     /// the type that is implementing the trait.
     const BASE: VMStateField;
@@ -80,6 +153,12 @@ pub unsafe trait VMState {
     };
 }
 
+/// Internal utility function to retrieve a type's `VMStateFieldType`;
+/// used by [`vmstate_of!`](crate::vmstate_of).
+pub const fn vmstate_scalar_type<T: VMState>(_: PhantomData<T>) -> VMStateFieldType {
+    T::SCALAR_TYPE
+}
+
 /// Internal utility function to retrieve a type's `VMStateField`;
 /// used by [`vmstate_of!`](crate::vmstate_of).
 pub const fn vmstate_base<T: VMState>(_: PhantomData<T>) -> VMStateField {
@@ -96,11 +175,20 @@ pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateField
 /// Return the `VMStateField` for a field of a struct.  The field must be
 /// visible in the current scope.
 ///
+/// Only a limited set of types is supported out of the box:
+/// * scalar types (integer and `bool`)
+/// * the C struct `QEMUTimer`
+/// * a transparent wrapper for any of the above (`Cell`, `UnsafeCell`,
+///   [`BqlCell`](crate::cell::BqlCell), [`BqlRefCell`](crate::cell::BqlRefCell)
+/// * a raw pointer to any of the above
+/// * a `NonNull` pointer to any of the above, possibly wrapped with `Option`
+/// * an array of any of the above
+///
 /// In order to support other types, the trait `VMState` must be implemented
 /// for them.
 #[macro_export]
 macro_rules! vmstate_of {
-    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? $(,)?) => {
+    ($struct_name:ty, $field_name:ident $([0 .. $num:tt $(* $factor:expr)?])? $(,)?) => {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), "\0")
                 .as_bytes()
@@ -109,6 +197,11 @@ macro_rules! vmstate_of {
             $(.num_offset: $crate::offset_of!($struct_name, $num),)?
             // The calls to `call_func_with_field!` are the magic that
             // computes most of the VMStateField from the type of the field.
+            info: $crate::info_enum_to_ref!($crate::call_func_with_field!(
+                $crate::vmstate::vmstate_scalar_type,
+                $struct_name,
+                $field_name
+            )),
             ..$crate::call_func_with_field!(
                 $crate::vmstate::vmstate_base,
                 $struct_name,
@@ -175,6 +268,7 @@ pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField {
 macro_rules! impl_vmstate_transparent {
     ($type:ty where $base:tt: VMState $($where:tt)*) => {
         unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
+            const SCALAR_TYPE: VMStateFieldType = <$base as VMState>::SCALAR_TYPE;
             const BASE: VMStateField = VMStateField {
                 size: mem::size_of::<$type>(),
                 ..<$base as VMState>::BASE
@@ -189,6 +283,33 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
 impl_vmstate_transparent!(crate::cell::BqlCell<T> where T: VMState);
 impl_vmstate_transparent!(crate::cell::BqlRefCell<T> where T: VMState);
 
+// Scalar types using predefined VMStateInfos
+
+macro_rules! impl_vmstate_scalar {
+    ($info:ident, $type:ty$(, $varray_flag:ident)?) => {
+        unsafe impl VMState for $type {
+            const SCALAR_TYPE: VMStateFieldType = VMStateFieldType::$info;
+            const BASE: VMStateField = VMStateField {
+                size: mem::size_of::<$type>(),
+                flags: VMStateFlags::VMS_SINGLE,
+                ..Zeroable::ZERO
+            };
+            $(const VARRAY_FLAG: VMStateFlags = VMStateFlags::$varray_flag;)?
+        }
+    };
+}
+
+impl_vmstate_scalar!(vmstate_info_bool, bool);
+impl_vmstate_scalar!(vmstate_info_int8, i8);
+impl_vmstate_scalar!(vmstate_info_int16, i16);
+impl_vmstate_scalar!(vmstate_info_int32, i32);
+impl_vmstate_scalar!(vmstate_info_int64, i64);
+impl_vmstate_scalar!(vmstate_info_uint8, u8, VMS_VARRAY_UINT8);
+impl_vmstate_scalar!(vmstate_info_uint16, u16, VMS_VARRAY_UINT16);
+impl_vmstate_scalar!(vmstate_info_uint32, u32, VMS_VARRAY_UINT32);
+impl_vmstate_scalar!(vmstate_info_uint64, u64);
+impl_vmstate_scalar!(vmstate_info_timer, bindings::QEMUTimer);
+
 // Pointer types using the underlying type's VMState plus VMS_POINTER
 
 macro_rules! impl_vmstate_pointer {
-- 
2.47.1

>  #[macro_export]
>  macro_rules! vmstate_of {
> -    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? $(,)?) => {
> +    ($struct_name:ty, $field_name:ident $([0 .. $num:tt $(* $factor:expr)?])? $(,)?) => {

Why change ident to tt? 

>          $crate::bindings::VMStateField {
>              name: ::core::concat!(::core::stringify!($field_name), "\0")
>                  .as_bytes()
> @@ -109,6 +197,11 @@ macro_rules! vmstate_of {
>              $(.num_offset: $crate::offset_of!($struct_name, $num),)?
>              // The calls to `call_func_with_field!` are the magic that
>              // computes most of the VMStateField from the type of the field.
> +            info: $crate::info_enum_to_ref!($crate::call_func_with_field!(
> +                $crate::vmstate::vmstate_scalar_type,
> +                $struct_name,
> +                $field_name
> +            )),
>              ..$crate::call_func_with_field!(
>                  $crate::vmstate::vmstate_base,
>                  $struct_name,

...

> +impl_vmstate_scalar!(vmstate_info_bool, bool);
> +impl_vmstate_scalar!(vmstate_info_int8, i8);
> +impl_vmstate_scalar!(vmstate_info_int16, i16);
> +impl_vmstate_scalar!(vmstate_info_int32, i32);

missed VMS_VARRAY_INT32 :-)

> +impl_vmstate_scalar!(vmstate_info_int64, i64);
> +impl_vmstate_scalar!(vmstate_info_uint8, u8, VMS_VARRAY_UINT8);
> +impl_vmstate_scalar!(vmstate_info_uint16, u16, VMS_VARRAY_UINT16);
> +impl_vmstate_scalar!(vmstate_info_uint32, u32, VMS_VARRAY_UINT32);

If we want to expand in the future (e.g., support vmstate_info_int32_equal
and vmstate_info_int32_le), then introducing new macro variants will be
straightforward. So, fair enough.

> +impl_vmstate_scalar!(vmstate_info_uint64, u64);

What about applying this to "usize" with vmstate_info_uint64?

For array length, I think usize is also used wildly. Maybe we can add
VMS_VARRAY_UINT64 and just treat usize as u64.

> +impl_vmstate_scalar!(vmstate_info_timer, bindings::QEMUTimer);
> +
>  // Pointer types using the underlying type's VMState plus VMS_POINTER
>  
>  macro_rules! impl_vmstate_pointer {
> -- 
> 2.47.1

Overall, I think it's good; the design idea is coherent.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

On 1/8/25 07:45, Zhao Liu wrote:
>>   #[macro_export]
>>   macro_rules! vmstate_of {
>> -    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? $(,)?) => {
>> +    ($struct_name:ty, $field_name:ident $([0 .. $num:tt $(* $factor:expr)?])? $(,)?) => {
> 
> Why change ident to tt?

Rebase mistake.  Initially I had $num:tt, however that becomes unclear 
if you have [0 .. 0] where the second 0 is a field name.

>> +impl_vmstate_scalar!(vmstate_info_bool, bool);
>> +impl_vmstate_scalar!(vmstate_info_int8, i8);
>> +impl_vmstate_scalar!(vmstate_info_int16, i16);
>> +impl_vmstate_scalar!(vmstate_info_int32, i32);
> 
> missed VMS_VARRAY_INT32 :-)

I left that out intentionally, as Rust is probably going to use 
IndexMut<uNN> instead of i32.

>> +impl_vmstate_scalar!(vmstate_info_int64, i64);
>> +impl_vmstate_scalar!(vmstate_info_uint8, u8, VMS_VARRAY_UINT8);
>> +impl_vmstate_scalar!(vmstate_info_uint16, u16, VMS_VARRAY_UINT16);
>> +impl_vmstate_scalar!(vmstate_info_uint32, u32, VMS_VARRAY_UINT32);
> 
> If we want to expand in the future (e.g., support vmstate_info_int32_equal
> and vmstate_info_int32_le), then introducing new macro variants will be
> straightforward. So, fair enough.
> 
>> +impl_vmstate_scalar!(vmstate_info_uint64, u64);
> 
> What about applying this to "usize" with vmstate_info_uint64?

There's 32-bit hosts too...  So one would have to add vmstate_info_ulong 
which is serialized as 64-bit.

We can add it later, but perhaps we could also create a derive(Index, 
IndexMut) macro that makes it possible to specify the type of the index. 
  While Rust uses usize instead of uNN for array indices, that does not 
have to be universal; using uNN is a lot better if it means you can get 
rid of casts from register values to array indices and back.  See for 
example commit 6b4f7b0705b ("rust: pl011: fix migration stream", 
2024-12-19).

That is indeed also an issue for HPET, but in that case it can be 
isolated to a couple lines,

             let timer_id: usize = ((addr - 0x100) / 0x20) as usize;

and it could even be wrapped further

     fn timer_and_addr(&self, addr: hwaddr) -> 
Option<&BqlRefCell<HPETTimer>, hwaddr> {
         let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
         if timer_id > self.num_timers.get() {
             // TODO: Add trace point - 
trace_hpet_timer_id_out_of_range(timer_id)
             None
         } else {
             Some((self.get_timer(timer_id), addr & 0x18))
         }
     }

     ...

     match self.timer_and_addr(addr) {
         None => 0 // Reserved,
         Some(timer, addr) => timer.borrow_mut().read(addr, size)
     }


So for HPET you didn't reach the threshold of having to create "pub 
struct HPETTimers([BqlRefCell<HPETTimer>; MAX_HPET_TIMERS])" and 
implement Index<>.

Paolo

 
> > > +impl_vmstate_scalar!(vmstate_info_uint64, u64);
> > 
> > What about applying this to "usize" with vmstate_info_uint64?
> 
> There's 32-bit hosts too...  So one would have to add vmstate_info_ulong
> which is serialized as 64-bit.
> 
> We can add it later, but perhaps we could also create a derive(Index,
> IndexMut) macro that makes it possible to specify the type of the index.
> While Rust uses usize instead of uNN for array indices, that does not have
> to be universal; using uNN is a lot better if it means you can get rid of
> casts from register values to array indices and back.  See for example
> commit 6b4f7b0705b ("rust: pl011: fix migration stream", 2024-12-19).

Yes, I agree!

> That is indeed also an issue for HPET, but in that case it can be isolated
> to a couple lines,
> 
>             let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
> 
> and it could even be wrapped further
> 
>     fn timer_and_addr(&self, addr: hwaddr) -> Option<&BqlRefCell<HPETTimer>,
> hwaddr> {
>         let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
>         if timer_id > self.num_timers.get() {
>             // TODO: Add trace point -
> trace_hpet_timer_id_out_of_range(timer_id)
>             None
>         } else {
>             Some((self.get_timer(timer_id), addr & 0x18))
>         }
>     }
> 
>     ...
> 
>     match self.timer_and_addr(addr) {
>         None => 0 // Reserved,
>         Some(timer, addr) => timer.borrow_mut().read(addr, size)
>     }
> 
> 
> So for HPET you didn't reach the threshold of having to create "pub struct
> HPETTimers([BqlRefCell<HPETTimer>; MAX_HPET_TIMERS])" and implement Index<>.
> 

Thank you Paolo! Will apply your wrapping suggestion!

Regards,
Zhao