Re: [PATCH WIP RFC] rust: add qdev DeviceProperties derive macro

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Fri, May 23, 2025 at 4:06 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il gio 22 mag 2025, 10:12 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> ha scritto:
>>
>> This is unnecessary though, because once we have the
>> const_refs_to_static feature we can introduce a QdevProp trait that
>> returns a reference to a type's qdev_prop_* global variable. We cannot
>> do this now because for our minimum Rust version we cannot refer to
>> statics from const values.
>
>
> Why don't you already write it assuming const_refs_to_static? If needed we can add a hack to make the VALUE an enum, similar to what is already in vmstate.rs.
>
>> So, this patch is not for merging yet, I will wait until we upgrade the
>> Rust version and re-spin it with a proper trait-based implementation (and
>> also split it into individual steps/patches).
>
>
> It's not too large, overall.
>
>>  #[repr(C)]
>> -#[derive(qemu_api_macros::Object)]
>> +#[derive(qemu_api_macros::Object, qemu_api_macros::DeviceProperties)]
>
>
> Is there more to be derived that is useful for Devices? Maybe the macro can be DeviceState or Device.

VMStateDescription and the realize callback, I think.

>
>>  +    #[property(name = c"chardev", qdev_prop = qemu_api::bindings::qdev_prop_chr)]
>
>
> Can you change the name to be a "normal" string and change it to a C literal in the macro?

That'd be neat, it should be possible to create a cstr literal token
and error out if the input str literal cannot be represented as a
cstr.

>
>> diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
>> index d328d846323f6080a9573053767e51481eb32941..83d70d7d82aac4a3252a0b4cb24af705b01d3635 100644
>> --- a/rust/hw/char/pl011/src/device_class.rs
>> +++ b/rust/hw/char/pl011/src/device_class.rs
>> @@ -8,11 +8,8 @@
>>  };
>>
>>  use qemu_api::{
>> -    bindings::{qdev_prop_bool, qdev_prop_chr},
>> -    prelude::*,
>> -    vmstate::VMStateDescription,
>> -    vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_unused,
>> -    zeroable::Zeroable,
>> +    prelude::*, vmstate::VMStateDescription, vmstate_clock, vmstate_fields, vmstate_of,
>> +    vmstate_struct, vmstate_subsections, vmstate_unused, zeroable::Zeroable,
>>  };
>
>
> I would also merge the files at this point, but no hurry.
>
>> +#[derive(Debug)]
>> +struct DeviceProperty {
>> +    name: Option<syn::LitCStr>,
>> +    qdev_prop: Option<syn::Path>,
>> +    assert_type: Option<proc_macro2::TokenStream>,
>> +    bitnr: Option<syn::Expr>,
>> +    defval: Option<syn::Expr>,
>> +}
>> +
>> +impl Parse for DeviceProperty {
>
>
> Can you look into using https://docs.rs/crate/attrs/latest for parsing?
>
> (attrs doesn't support LitCStr btw)

I think we can do without it for now, even if this patch is not
cleaned up (for example it has unwraps instead of proper panic
messages or error handling) it does not end up very complex as far as
attribute parsing is concerned.

I'm fine with either approach though.

>
>> +#[proc_macro_derive(DeviceProperties, attributes(property, bool_property))]
>> +pub fn derive_device_properties(input: TokenStream) -> TokenStream {
>
>
> Do you need to handle errors in the parsing of attributes?...
>
>> +        _other => unreachable!(),
>

This should be literally unreachable IIUC, because only property names
declared in the attributes part of `#[proc_macro_derive(...
attributes(__))]` would get accepted by the compiler and passed to the
derive macro.

>
> ... Yes, you do—there is code already in qemu_macros, used by derive_object_or_error(), to get the fields of a struct with proper error handling.
>
>> let prop_name = prop.name.as_ref().unwrap();
>>
>> + let field_name = field.ident.as_ref().unwrap();
>>
>> + let qdev_prop = prop.qdev_prop.as_ref().unwrap();
>>
>> + let bitnr = prop.bitnr.as_ref().unwrap_or(&zero);
>>
>> + let set_default = prop.defval.is_some();
>>
>> + let defval = prop.defval.as_ref().unwrap_or(&zero);+                bitnr: #bitnr,
>> +
>
>
> You also need to use let...else here instead of unwrap(), since panicking provides worse error messages.
>
>>               set_default: #set_default,
>> +                defval: ::qemu_api::bindings::Property__bindgen_ty_1 { u: #defval as u64 },
>
>
> If you like it, you can write this also as
>
>   #(bitnr: #bitnr,)?
>   #(set_default: true, defval: ...)?
>
> and keep bitnr and defval as Options, since the None case is handled by the default below.
>
> Paolo

Thanks I will take another look!

>
>
>> +                ..::qemu_api::zeroable::Zeroable::ZERO
>> +            }
>> +        });
>> +    }
>> +    let properties_expanded = quote_spanned! {span.into()=>
>> +        #(#assertions)*
>> +
>> +        impl ::qemu_api::qdev::DevicePropertiesImpl for #name {
>> +            fn properties() -> &'static [::qemu_api::bindings::Property] {
>> +                static PROPERTIES: &'static [::qemu_api::bindings::Property] = &[#(#properties_expanded),*];
>> +
>> +                PROPERTIES
>> +            }
>> +        }
>> +    };
>> +
>> +    TokenStream::from(properties_expanded)
>> +}
>> +
>>  #[proc_macro_derive(Wrapper)]
>>  pub fn derive_opaque(input: TokenStream) -> TokenStream {
>>      let input = parse_macro_input!(input as DeriveInput);
>> diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
>> index 1279d7a58d50e1bf6c8d2e6f00d7229bbb19e003..2fd8b2750ffabcaa1065558d38a700e35fbc9136 100644
>> --- a/rust/qemu-api/src/qdev.rs
>> +++ b/rust/qemu-api/src/qdev.rs
>> @@ -100,8 +100,19 @@ pub trait ResettablePhasesImpl {
>>      T::EXIT.unwrap()(unsafe { state.as_ref() }, typ);
>>  }
>>
>> +pub trait DevicePropertiesImpl {
>> +    /// An array providing the properties that the user can set on the
>> +    /// device.  Not a `const` because referencing statics in constants
>> +    /// is unstable until Rust 1.83.0.
>> +    fn properties() -> &'static [Property] {
>> +        &[]
>> +    }
>> +}
>> +
>>  /// Trait providing the contents of [`DeviceClass`].
>> -pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl + IsA<DeviceState> {
>> +pub trait DeviceImpl:
>> +    ObjectImpl + ResettablePhasesImpl + DevicePropertiesImpl + IsA<DeviceState>
>> +{
>>      /// _Realization_ is the second stage of device creation. It contains
>>      /// all operations that depend on device properties and can fail (note:
>>      /// this is not yet supported for Rust devices).
>> @@ -110,13 +121,6 @@ pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl + IsA<DeviceState> {
>>      /// with the function pointed to by `REALIZE`.
>>      const REALIZE: Option<fn(&Self)> = None;
>>
>> -    /// An array providing the properties that the user can set on the
>> -    /// device.  Not a `const` because referencing statics in constants
>> -    /// is unstable until Rust 1.83.0.
>> -    fn properties() -> &'static [Property] {
>> -        &[]
>> -    }
>> -
>>      /// A `VMStateDescription` providing the migration format for the device
>>      /// Not a `const` because referencing statics in constants is unstable
>>      /// until Rust 1.83.0.
>> @@ -171,7 +175,7 @@ pub fn class_init<T: DeviceImpl>(&mut self) {
>>          if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
>>              self.vmsd = vmsd;
>>          }
>> -        let prop = <T as DeviceImpl>::properties();
>> +        let prop = <T as DevicePropertiesImpl>::properties();
>>          if !prop.is_empty() {
>>              unsafe {
>>                  bindings::device_class_set_props_n(self, prop.as_ptr(), prop.len());
>> diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
>> index a658a49fcfdda8fa4b9d139c10afb6ff3243790b..e8eadfd6e9add385ffc97de015b84aae825c18ee 100644
>> --- a/rust/qemu-api/tests/tests.rs
>> +++ b/rust/qemu-api/tests/tests.rs
>> @@ -9,7 +9,7 @@
>>      cell::{self, BqlCell},
>>      declare_properties, define_property,
>>      prelude::*,
>> -    qdev::{DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
>> +    qdev::{DeviceImpl, DevicePropertiesImpl, DeviceState, Property, ResettablePhasesImpl},
>>      qom::{ObjectImpl, ParentField},
>>      sysbus::SysBusDevice,
>>      vmstate::VMStateDescription,
>> @@ -68,10 +68,13 @@ impl ObjectImpl for DummyState {
>>
>>  impl ResettablePhasesImpl for DummyState {}
>>
>> -impl DeviceImpl for DummyState {
>> +impl DevicePropertiesImpl for DummyState {
>>      fn properties() -> &'static [Property] {
>>          &DUMMY_PROPERTIES
>>      }
>> +}
>> +
>> +impl DeviceImpl for DummyState {
>>      fn vmsd() -> Option<&'static VMStateDescription> {
>>          Some(&VMSTATE)
>>      }
>> @@ -85,6 +88,8 @@ pub struct DummyChildState {
>>
>>  qom_isa!(DummyChildState: Object, DeviceState, DummyState);
>>
>> +impl DevicePropertiesImpl for DummyChildState {}
>> +
>>  pub struct DummyChildClass {
>>      parent_class: <DummyState as ObjectType>::Class,
>>  }
>>
>> ---
>> base-commit: 2af4a82ab2cce3412ffc92cd4c96bd870e33bc8e
>> change-id: 20250522-rust-qdev-properties-728e8f6a468e
>>
>> --
>> γαῖα πυρί μιχθήτω
>>