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

[Paolo Bonzini]

On 1/22/25 13:33, Zhao Liu wrote:
> 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.

It's moved below because there's now more than one call_func_with_field 
call.  I can make it so that the phrasing and placement remains the same 
throughout the series.

Paolo

> >               $(.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>