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

[Paolo Bonzini <pbonzini@redhat.com>]

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>
> 
> 
>