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.
+ #[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?
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)
+#[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!(),
>
... 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
+ ..::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
>
> --
> γαῖα πυρί μιχθήτω
>
>
