On Fri, May 23, 2025 at 4:06 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
>
>
> Il gio 22 mag 2025, 10:12 Manos Pitsidianakis
> <manos.pitsidiana...@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
>>
>> --
>> γαῖα πυρί μιχθήτω
>>
