Re: [RFC PATCH 2/9] rust: vmstate: implement VMState for non-leaf types

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

On Tue, Dec 31, 2024 at 01:23:29AM +0100, Paolo Bonzini wrote:
> Date: Tue, 31 Dec 2024 01:23:29 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [RFC PATCH 2/9] rust: vmstate: implement VMState for non-leaf types
> X-Mailer: git-send-email 2.47.1
> 
> Arrays, pointers and cells use a VMStateField that is based on that
> for the inner type.  The implementation therefore delegates to the
> VMState implementation of the inner type.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/vmstate.rs | 66 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index bfcf06e8f1d..e20f27b172b 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -19,8 +19,9 @@
>  //!   `include/migration/vmstate.h`. These are not type-safe and should not be
>  //!   used if the equivalent functionality is available with `vmstate_of!`.
>  
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, mem, ptr::NonNull};
>  
> +use crate::bindings::VMStateFlags;
>  pub use crate::bindings::{VMStateDescription, VMStateField};
>  
>  /// This macro is used to call a function with a generic argument bound
> @@ -108,6 +109,69 @@ pub const fn with_version_id(mut self, version_id: i32) -> Self {
>          self.version_id = version_id;
>          self
>      }
> +
> +    #[must_use]
> +    pub const fn with_array_flag(mut self, num: usize) -> Self { 
> +        assert!(num <= 0x7FFF_FFFFusize);

I see, this is the similar case to check overflow like your comment for
patch 1. I think we can use i32::MAX to avoid such magic number.

> +        assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) == 0);

Do we need to also check varray flags? e.g.,

But the doc (in vmstate.h) of VMS_ARRAY said "May not be combined with
VMS_VARRAY*", I understand VMS_ARRAY shouldn't be combined with
VMS_VARRAY*, am I right?

If so, we can add a simple const variable that combines all VARRY flags,
e.g,

const VMS_VARRAY_FLAGS: VMStateFlags = VMStateFlags(
    VMStateFlags::VMS_VARRAY_INT32.0 |
    VMStateFlags::VMS_VARRAY_UINT8.0 |
    VMStateFlags::VMS_VARRAY_UINT16.0 |
    VMStateFlags::VMS_VARRAY_UINT32.0
);

...

assert!((self.flags.0 & VMS_VARRAY_FLAGS.0) == 0);

> +        if (self.flags.0 & VMStateFlags::VMS_POINTER.0) != 0 {
> +            self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_POINTER.0);
> +            self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_ARRAY_OF_POINTER.0);
> +        }
> +        self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_SINGLE.0);
> +        self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_ARRAY.0);
> +        self.num = num as i32;
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn with_pointer_flag(mut self) -> Self {
> +        assert!((self.flags.0 & VMStateFlags::VMS_POINTER.0) == 0);

Maybe VMS_ARRAY_OF_POINTER should be checked here as well?

> +        self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_POINTER.0);
> +        self
> +    }
> +}
> +
> +// Transparent wrappers: just use the internal type
> +
> +macro_rules! impl_vmstate_transparent {
> +    ($type:ty where $base:tt: VMState $($where:tt)*) => {
> +        unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
> +            const BASE: VMStateField = VMStateField {
> +                size: mem::size_of::<$type>(),
> +                ..<$base as VMState>::BASE
> +            };
> +        }
> +    };
> +}
> +
> +impl_vmstate_transparent!(std::cell::Cell<T> where T: VMState);
> +impl_vmstate_transparent!(std::cell::UnsafeCell<T> where T: VMState);
> +impl_vmstate_transparent!(crate::cell::BqlCell<T> where T: VMState);
> +impl_vmstate_transparent!(crate::cell::BqlRefCell<T> where T: VMState);
> +
> +// Pointer types using the underlying type's VMState plus VMS_POINTER
> +
> +macro_rules! impl_vmstate_pointer {
> +    ($type:ty where $base:tt: VMState $($where:tt)*) => {
> +        unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
> +            const SCALAR_TYPE: VMStateFieldType = <T as VMState>::SCALAR_TYPE;

Rebase issue: SCALAR_TYPE is introduced in patch 5.

> +            const BASE: VMStateField = <$base as VMState>::BASE.with_pointer_flag();
> +        }
> +    };
> +}
> +
> +impl_vmstate_pointer!(*const T where T: VMState);
> +impl_vmstate_pointer!(*mut T where T: VMState);
> +impl_vmstate_pointer!(NonNull<T> where T: VMState);
> +impl_vmstate_pointer!(Option<NonNull<T>> where T: VMState);
> +
> +// Arrays using the underlying type's VMState plus
> +// VMS_ARRAY/VMS_ARRAY_OF_POINTER
> +
> +unsafe impl<T: VMState, const N: usize> VMState for [T; N] {
> +    const SCALAR_TYPE: VMStateFieldType = <T as VMState>::SCALAR_TYPE;

Ditto.

> +    const BASE: VMStateField = <T as VMState>::BASE.with_array_flag(N);
>  }
>  
>  #[doc(alias = "VMSTATE_UNUSED_BUFFER")]
> -- 
> 2.47.1
>

Overall, I think this patch handles the array/pointer/cell cases very
well. So (with some nits fixed :-)),

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