diff mbox series

[RFC,3/9] rust: vmstate: add varray support to vmstate_of!

Message ID 20241231002336.25931-4-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series rust: (mostly) type safe VMState | expand

Commit Message

Paolo Bonzini Dec. 31, 2024, 12:23 a.m. UTC
Untested...

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

Comments

Zhao Liu Jan. 8, 2025, 3:28 a.m. UTC | #1
On Tue, Dec 31, 2024 at 01:23:30AM +0100, Paolo Bonzini wrote:
> Date: Tue, 31 Dec 2024 01:23:30 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [RFC PATCH 3/9] rust: vmstate: add varray support to vmstate_of!
> X-Mailer: git-send-email 2.47.1
> 
> Untested...
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/vmstate.rs | 45 +++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index e20f27b172b..079c19c687b 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -69,6 +69,15 @@ pub unsafe trait VMState {
>      /// The base contents of a `VMStateField` (minus the name and offset) for
>      /// the type that is implementing the trait.
>      const BASE: VMStateField;
> +
> +    /// A flag that is added to another field's `VMStateField` to specify the
> +    /// length's type in a variable-sized array.  If this is not a supported
> +    /// type for the length (i.e. if it is not `u8`, `u16`, `u32`), using it
> +    /// in a call to [`vmstate_of!`](crate::vmstate_of) will cause a
> +    /// compile-time error.
> +    const VARRAY_FLAG: VMStateFlags = {
> +        panic!("invalid type for variable-sized array");
> +    };
>  }
>  
>  /// Internal utility function to retrieve a type's `VMStateField`;
> @@ -77,6 +86,13 @@ pub const fn vmstate_base<T: VMState>(_: PhantomData<T>) -> VMStateField {
>      T::BASE
>  }
>  
> +/// Internal utility function to retrieve a type's `VMStateFlags` when it
> +/// is used as the element count of a `VMSTATE_VARRAY`; used by
> +/// [`vmstate_of!`](crate::vmstate_of).
> +pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateField {
> +    T::BASE
> +}

a copy issue:

pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateFlags {
    T::VARRAY_FLAG
}

> +
>  /// Return the `VMStateField` for a field of a struct.  The field must be
>  /// visible in the current scope.
>  ///
> @@ -84,18 +100,24 @@ pub const fn vmstate_base<T: VMState>(_: PhantomData<T>) -> VMStateField {
>  /// for them.
>  #[macro_export]
>  macro_rules! vmstate_of {
> -    ($struct_name:ty, $field_name:ident $(,)?) => {
> +    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? $(,)?) => {

Compared to something like "[num]", the format "[0 .. num]" is more
elegant, so I also support this style.

>          $crate::bindings::VMStateField {
>              name: ::core::concat!(::core::stringify!($field_name), "\0")
>                  .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.
> +            $(.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.
>              ..$crate::call_func_with_field!(
>                  $crate::vmstate::vmstate_base,
>                  $struct_name,
>                  $field_name
> -            )
> +            )$(.with_varray_flag($crate::call_func_with_field!(
> +                    $crate::vmstate::vmstate_varray_flag,
> +                    $struct_name,
> +                    $num))
> +               $(.with_varray_multiply($factor))?)?
>          }
>      };
>  }
> @@ -130,6 +152,22 @@ pub const fn with_pointer_flag(mut self) -> Self {
>          self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_POINTER.0);
>          self
>      }
> +
> +    #[must_use]
> +    pub const fn with_varray_flag<T: VMState>(mut self, flag: VMStateFlags) -> VMStateField {
> +        assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0);

I understand you checked VMS_ARRAY here since [T; N] has this array
flag.

What if a Rust device just store a pointer to the array? If we allow
this use, then it seems also possible to set varray flags...Then what
about dropping this limitation?

However, I also doube that pointer usage is bad; we should always use
Vec.

> +        self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_ARRAY.0);
> +        self.flags = VMStateFlags(self.flags.0 | flag.0);
> +        self
> +    }
> +
> +    #[must_use]
> +    pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField {
> +        assert!(num <= 0x7FFF_FFFFu32);

Similarly, what about using "assert!(num <= i32::MAX as u32);"?

> +        self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_MULTIPLY_ELEMENTS.0);
> +        self.num = num as i32;
> +        self
> +    }
>  }
>  
>  // Transparent wrappers: just use the internal type
> @@ -141,6 +179,7 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
>                  size: mem::size_of::<$type>(),
>                  ..<$base as VMState>::BASE
>              };
> +            const VARRAY_FLAG: VMStateFlags = <$base as VMState>::VARRAY_FLAG;
>          }
>      };
>  }
> -- 
> 2.47.1
>
Paolo Bonzini Jan. 15, 2025, 10:14 a.m. UTC | #2
On 1/8/25 04:28, Zhao Liu wrote:
>> +    #[must_use]
>> +    pub const fn with_varray_flag<T: VMState>(mut self, flag: VMStateFlags) -> VMStateField {
>> +        assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0);
> 
> I understand you checked VMS_ARRAY here since [T; N] has this array
> flag.
> 
> What if a Rust device just store a pointer to the array? If we allow
> this use, then it seems also possible to set varray flags...Then what
> about dropping this limitation?

Box can be added for that purpose:

impl_vmstate_pointer!(Box<T> where T: VMState);

Also I need to drop Option<NonNull<>> because of

             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
                 assert(first_elem || !n_elems || !size);
             }

in migration/vmstate.c.

> However, I also doube that pointer usage is bad; we should always use
> Vec.

Vec support is a bit tricky because the number of elements is not 
accessible from C.  But unbounded arrays are rare in devices.  We can 
think about it later.

Paolo
diff mbox series

Patch

diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index e20f27b172b..079c19c687b 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -69,6 +69,15 @@  pub unsafe trait VMState {
     /// The base contents of a `VMStateField` (minus the name and offset) for
     /// the type that is implementing the trait.
     const BASE: VMStateField;
+
+    /// A flag that is added to another field's `VMStateField` to specify the
+    /// length's type in a variable-sized array.  If this is not a supported
+    /// type for the length (i.e. if it is not `u8`, `u16`, `u32`), using it
+    /// in a call to [`vmstate_of!`](crate::vmstate_of) will cause a
+    /// compile-time error.
+    const VARRAY_FLAG: VMStateFlags = {
+        panic!("invalid type for variable-sized array");
+    };
 }
 
 /// Internal utility function to retrieve a type's `VMStateField`;
@@ -77,6 +86,13 @@  pub const fn vmstate_base<T: VMState>(_: PhantomData<T>) -> VMStateField {
     T::BASE
 }
 
+/// Internal utility function to retrieve a type's `VMStateFlags` when it
+/// is used as the element count of a `VMSTATE_VARRAY`; used by
+/// [`vmstate_of!`](crate::vmstate_of).
+pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateField {
+    T::BASE
+}
+
 /// Return the `VMStateField` for a field of a struct.  The field must be
 /// visible in the current scope.
 ///
@@ -84,18 +100,24 @@  pub const fn vmstate_base<T: VMState>(_: PhantomData<T>) -> VMStateField {
 /// for them.
 #[macro_export]
 macro_rules! vmstate_of {
-    ($struct_name:ty, $field_name:ident $(,)?) => {
+    ($struct_name:ty, $field_name:ident $([0 .. $num:ident $(* $factor:expr)?])? $(,)?) => {
         $crate::bindings::VMStateField {
             name: ::core::concat!(::core::stringify!($field_name), "\0")
                 .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.
+            $(.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.
             ..$crate::call_func_with_field!(
                 $crate::vmstate::vmstate_base,
                 $struct_name,
                 $field_name
-            )
+            )$(.with_varray_flag($crate::call_func_with_field!(
+                    $crate::vmstate::vmstate_varray_flag,
+                    $struct_name,
+                    $num))
+               $(.with_varray_multiply($factor))?)?
         }
     };
 }
@@ -130,6 +152,22 @@  pub const fn with_pointer_flag(mut self) -> Self {
         self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_POINTER.0);
         self
     }
+
+    #[must_use]
+    pub const fn with_varray_flag<T: VMState>(mut self, flag: VMStateFlags) -> VMStateField {
+        assert!((self.flags.0 & VMStateFlags::VMS_ARRAY.0) != 0);
+        self.flags = VMStateFlags(self.flags.0 & !VMStateFlags::VMS_ARRAY.0);
+        self.flags = VMStateFlags(self.flags.0 | flag.0);
+        self
+    }
+
+    #[must_use]
+    pub const fn with_varray_multiply(mut self, num: u32) -> VMStateField {
+        assert!(num <= 0x7FFF_FFFFu32);
+        self.flags = VMStateFlags(self.flags.0 | VMStateFlags::VMS_MULTIPLY_ELEMENTS.0);
+        self.num = num as i32;
+        self
+    }
 }
 
 // Transparent wrappers: just use the internal type
@@ -141,6 +179,7 @@  unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
                 size: mem::size_of::<$type>(),
                 ..<$base as VMState>::BASE
             };
+            const VARRAY_FLAG: VMStateFlags = <$base as VMState>::VARRAY_FLAG;
         }
     };
 }