Re: [PATCH 05/10] rust: vmstate: implement VMState for scalar types

[Zhao Liu <zhao1.liu@intel.com>]

On Fri, Jan 17, 2025 at 10:00:41AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:00:41 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 05/10] rust: vmstate: implement VMState for scalar types
> X-Mailer: git-send-email 2.47.1
> 
> 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 | 128 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 2 deletions(-)


>  /// 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 {
> @@ -99,6 +178,15 @@ 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`

I just found your rust-next has already updated and removed `Option` :-)

> +/// * an array of any of the above
> +///
>  /// In order to support other types, the trait `VMState` must be implemented
>  /// for them.
>  #[macro_export]
> @@ -109,8 +197,14 @@ macro_rules! vmstate_of {
>                  .as_bytes()
>                  .as_ptr() as *const ::std::os::raw::c_char,
>              offset: $crate::offset_of!($struct_name, $field_name),
> -            // Compute most of the VMStateField from the type of the field.

Rebase mistake? This comment seems no need to be deleted.

>              $(.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
> +            )),
>

Only a nit above,

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