| Message ID | 20241024-rust-round-2-v1-3-051e7a25b978@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust device model patches and misc cleanups | expand |
Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > Add a new derive procedural macro to declare device models. Add > corresponding DeviceImpl trait after already existing ObjectImpl trait. > At the same time, add instance_init, instance_post_init, > instance_finalize methods to the ObjectImpl trait and call them from the > ObjectImplUnsafe trait, which is generated by the procedural macro. > > This allows all the boilerplate device model registration to be handled > by macros, and all pertinent details to be declared through proc macro > attributes or trait associated constants and methods. > > The device class can now be generated automatically and the name can be > optionally overridden: > > ------------------------ >8 ------------------------ > #[repr(C)] > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > #[device(class_name_override = PL011Class)] > /// PL011 Device Model in QEMU > pub struct PL011State { > ------------------------ >8 ------------------------ > > Properties are now marked as field attributes: > > ------------------------ >8 ------------------------ > #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] > pub char_backend: CharBackend, > ------------------------ >8 ------------------------ > > Object methods (instance_init, etc) methods are now trait methods: > > ------------------------ >8 ------------------------ > /// Trait a type must implement to be registered with QEMU. > pub trait ObjectImpl { > type Class: ClassImpl; > const TYPE_NAME: &'static CStr; > const PARENT_TYPE_NAME: Option<&'static CStr>; > const ABSTRACT: bool; > > unsafe fn instance_init(&mut self) {} > fn instance_post_init(&mut self) {} > fn instance_finalize(&mut self) {} > } > ------------------------ >8 ------------------------ > > Device methods (realize/reset etc) are now safe idiomatic trait methods: > > ------------------------ >8 ------------------------ > /// Implementation methods for device types. > pub trait DeviceImpl: ObjectImpl { > fn realize(&mut self) {} > fn reset(&mut self) {} > } > ------------------------ >8 ------------------------ > > The derive Device macro is responsible for creating all the extern "C" FFI > functions that QEMU needs to call these methods. > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > rust/hw/char/pl011/src/device.rs | 124 +++----- > rust/hw/char/pl011/src/device_class.rs | 70 ----- > rust/hw/char/pl011/src/lib.rs | 1 - > rust/qemu-api-macros/src/device.rs | 433 ++++++++++++++++++++++++++ > rust/qemu-api-macros/src/lib.rs | 45 +-- > rust/qemu-api-macros/src/object.rs | 107 +++++++ > rust/qemu-api-macros/src/symbols.rs | 55 ++++ > rust/qemu-api-macros/src/utilities.rs | 152 +++++++++ > rust/qemu-api/meson.build | 3 +- > rust/qemu-api/src/definitions.rs | 97 ------ > rust/qemu-api/src/device_class.rs | 128 -------- > rust/qemu-api/src/lib.rs | 6 +- > rust/qemu-api/src/objects.rs | 90 ++++++ > rust/qemu-api/src/tests.rs | 49 --- > subprojects/packagefiles/syn-2-rs/meson.build | 1 + > 15 files changed, 902 insertions(+), 459 deletions(-) My initial reaction is these patches are getting quite large because we are doing several things at once. Is it possible to split them so: - we bring in one macro at a time (with unit tests) - we then convert pl011 to the new idiom ?
On Thu, 24 Oct 2024 at 18:14, Alex Bennée <alex.bennee@linaro.org> wrote: > > Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > > > Add a new derive procedural macro to declare device models. Add > > corresponding DeviceImpl trait after already existing ObjectImpl trait. > > At the same time, add instance_init, instance_post_init, > > instance_finalize methods to the ObjectImpl trait and call them from the > > ObjectImplUnsafe trait, which is generated by the procedural macro. > > > > This allows all the boilerplate device model registration to be handled > > by macros, and all pertinent details to be declared through proc macro > > attributes or trait associated constants and methods. > > > > The device class can now be generated automatically and the name can be > > optionally overridden: > > > > ------------------------ >8 ------------------------ > > #[repr(C)] > > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > > #[device(class_name_override = PL011Class)] > > /// PL011 Device Model in QEMU > > pub struct PL011State { > > ------------------------ >8 ------------------------ > > > > Properties are now marked as field attributes: > > > > ------------------------ >8 ------------------------ > > #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] > > pub char_backend: CharBackend, > > ------------------------ >8 ------------------------ > > > > Object methods (instance_init, etc) methods are now trait methods: > > > > ------------------------ >8 ------------------------ > > /// Trait a type must implement to be registered with QEMU. > > pub trait ObjectImpl { > > type Class: ClassImpl; > > const TYPE_NAME: &'static CStr; > > const PARENT_TYPE_NAME: Option<&'static CStr>; > > const ABSTRACT: bool; > > > > unsafe fn instance_init(&mut self) {} > > fn instance_post_init(&mut self) {} > > fn instance_finalize(&mut self) {} > > } > > ------------------------ >8 ------------------------ > > > > Device methods (realize/reset etc) are now safe idiomatic trait methods: > > > > ------------------------ >8 ------------------------ > > /// Implementation methods for device types. > > pub trait DeviceImpl: ObjectImpl { > > fn realize(&mut self) {} > > fn reset(&mut self) {} > > } > > ------------------------ >8 ------------------------ > > > > The derive Device macro is responsible for creating all the extern "C" FFI > > functions that QEMU needs to call these methods. > > > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > --- > > rust/hw/char/pl011/src/device.rs | 124 +++----- > > rust/hw/char/pl011/src/device_class.rs | 70 ----- > > rust/hw/char/pl011/src/lib.rs | 1 - > > rust/qemu-api-macros/src/device.rs | 433 ++++++++++++++++++++++++++ > > rust/qemu-api-macros/src/lib.rs | 45 +-- > > rust/qemu-api-macros/src/object.rs | 107 +++++++ > > rust/qemu-api-macros/src/symbols.rs | 55 ++++ > > rust/qemu-api-macros/src/utilities.rs | 152 +++++++++ > > rust/qemu-api/meson.build | 3 +- > > rust/qemu-api/src/definitions.rs | 97 ------ > > rust/qemu-api/src/device_class.rs | 128 -------- > > rust/qemu-api/src/lib.rs | 6 +- > > rust/qemu-api/src/objects.rs | 90 ++++++ > > rust/qemu-api/src/tests.rs | 49 --- > > subprojects/packagefiles/syn-2-rs/meson.build | 1 + > > 15 files changed, 902 insertions(+), 459 deletions(-) > > My initial reaction is these patches are getting quite large because we > are doing several things at once. Is it possible to split them so: > > - we bring in one macro at a time (with unit tests) > - we then convert pl011 to the new idiom Sure, I think it's possible to split them on some level. (Make changes on the existing Object derive macro and then introduce the Device macro) We can't unfortunately do changes without pl011 not being updated though, since it uses the code. As for unit tests, the only way to test proc macros is to match generated code output to expected output. Otherwise they are indirectly tested by the device not breaking other qemu tests. What do you think should be our unit testing approach for this instance?
On 10/24/24 16:03, Manos Pitsidianakis wrote: > Add a new derive procedural macro to declare device models. Add > corresponding DeviceImpl trait after already existing ObjectImpl trait. > At the same time, add instance_init, instance_post_init, > instance_finalize methods to the ObjectImpl trait and call them from the > ObjectImplUnsafe trait, which is generated by the procedural macro. > > This allows all the boilerplate device model registration to be handled > by macros, and all pertinent details to be declared through proc macro > attributes or trait associated constants and methods. > The device class can now be generated automatically and the name can be > optionally overridden: > > ------------------------ >8 ------------------------ > #[repr(C)] > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > #[device(class_name_override = PL011Class)] > /// PL011 Device Model in QEMU > pub struct PL011State { > ------------------------ >8 ------------------------ > > Properties are now marked as field attributes: > > ------------------------ >8 ------------------------ > #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] > pub char_backend: CharBackend, > ------------------------ >8 ------------------------ Let's look at this from the point of view of separating the code into multiple patches. This part is probably the easiest to extract. We can start the device impl trait with just the properties: pub trait DeviceImpl { const PROPERTIES: &[Property] = &[]; } and then the device_class_init! macro can use <$type as DeviceImpl>::PROPERTIES instead of $props. qemu_api::device_class_init! { - pl011_class_init, - props => PL011_PROPERTIES, + PL011State, pl011_class_init, realize_fn => Some(pl011_realize), legacy_reset_fn => Some(pl011_reset), vmsd => VMSTATE_PL011, } > Object methods (instance_init, etc) methods are now trait methods: > > ------------------------ >8 ------------------------ > /// Trait a type must implement to be registered with QEMU. > pub trait ObjectImpl { > type Class: ClassImpl; > const TYPE_NAME: &'static CStr; > const PARENT_TYPE_NAME: Option<&'static CStr>; > const ABSTRACT: bool; > > unsafe fn instance_init(&mut self) {} > fn instance_post_init(&mut self) {} > fn instance_finalize(&mut self) {} > } > ------------------------ >8 ------------------------ Some comments here: - instance_init should take either "instance: *mut self" or even better "instance: &mut MaybeUninit<self>". - for all Rust objects the implementation of instance_finalize() should just be ptr::drop_in_place(obj), so this need not be included in the trait. TYPE_NAME/PARENT_TYPE_NAME/ABSTRACT could be passed to the #[derive(Object)] macro, for example: #[derive(Object)] // abstract can be optional #[object(type_name="pl011", parent_type=DeviceState, abstract=false, class_type=PL011Class)] while the "fn instance_init()" remains as a trait implementation in pl011/src/device.rs. Because a trait can be implemented only once, this suggests separating the trait(s) in two: one for the constants that can be generated from the macro, one for the functions that form the vtable. This is true for both objects and devices: pub trait ObjectInfo { type Class: ClassImpl; const TYPE_NAME: &'static CStr; const PARENT_TYPE_NAME: Option<&'static CStr>; const ABSTRACT: bool; } pub trait ObjectImpl: ObjectInfo { unsafe fn instance_init(instance: &mut MaybeUninit<self>) {} } and likewise: > Device methods (realize/reset etc) are now safe idiomatic trait methods: > > ------------------------ >8 ------------------------ > /// Implementation methods for device types. > pub trait DeviceImpl: ObjectImpl { > fn realize(&mut self) {} > fn reset(&mut self) {} > } > ------------------------ >8 ------------------------ pub trait DeviceInfo { const PROPERTIES: &[Property] = &[]; } pub trait DeviceImpl: ObjectImpl + DeviceInfo { fn realize(&mut self) {} fn reset(&mut self) {} const VMSTATE: Option<VMStateDescription> = None; } (Generally, don't read too much in the code - the syntax might have issues but you get the idea). Anyhow, going forward to the property attribute: > + #[property(name = c"migrate-clk", qdev_prop = qdev_prop_bool)] There are two issues here: First, the default is true, so 1) this has to be fixed in QEMU (will do), 2) it is important to support it in #[property()]. Second, this provides a false sense of safety, because I could specify qdev_prop_chr here. Instead, the qdev_prop type should be derived by the field type. Third, since we are at it there's no need to use c"" in the attribute. The c_str!() macro that I am adding for backwards compatibility to old versions of Rust might actually come in handy here. The part where I have most comments, and some ideas of how to make your work a little more maintainable, is the implementation of class_init (and all that depends on it). Let's start with these generated functions: > + pub unsafe extern "C" fn #realize_fn( > + dev: *mut ::qemu_api::bindings::DeviceState, > + errp: *mut *mut ::qemu_api::bindings::Error, > + ) { > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > + unsafe { > + ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); > + } > + } > + > + #[no_mangle] > + pub unsafe extern "C" fn #reset_fn( > + dev: *mut ::qemu_api::bindings::DeviceState, > + ) { > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > + unsafe { > + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); > + } > + } This can be handled entirely in Rust code, outside the macro. If you add: unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>( dev: *mut DeviceState, errp: *mut *mut Error, ) { let mut instance = NonNull::new(dev.cast::<T>()). expect("Expected dev to be a non-null pointer"); unsafe { ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); } } unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>( dev: *mut ::qemu_api::bindings::DeviceState, ) { let mut instance = NonNull::new(dev.cast::<T>()). expect("Expected dev to be a non-null pointer"); unsafe { ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); } } then the functions can be used directly instead of #realize_fn and #reset_fn with a blanket implementation of DeviceImplUnsafe: unsafe impl DeviceImplUnsafe for T: DeviceImpl { const REALIZE: ... = Some(realize_fn_unsafe::<T>); const RESET: ... = Some(realize_fn_unsafe::<T>); } Going on to the implementation of the safe functions: > +impl DeviceImpl for PL011State { > + fn realize(&mut self) { These are not quite traits. First, you can implement only some of the functions. Second, if you don't implement them they are not overwritten by the class_init method. So this points to a different implementation as an attribute macro, which is able to rewrite everything in the body of the impl. For example: #[qom_class_init] impl DeviceImpl for PL011State { fn realize(&mut self) { ... } fn reset(&mut self) { ... } const VMSTATE: ... = {} const CATEGORY: ... = {} } can be transformed into: #[qom_class_init] impl DeviceImpl for PL011State { // fns are wrapped and transformed into consts const REALIZE: Option<fn(&mut self)> = { fn realize(&mut self) { ... } Some(realize) }; const RESET: Option<fn(&mut self)> = { fn reset(&mut self) { ... } Some(reset) }; // while associated consts (and perhaps types?) remain as is const VMSTATE: ... = {} const CATEGORY: ... = {} } The above blanket implementation of DeviceImplUnsafe is easily adjusted to support non-overridden methods: unsafe impl DeviceImplUnsafe for T: DeviceImpl { const REALIZE: ... = <T as DeviceImpl>::REALIZE::map( || realize_fn_unsafe::<T>); const RESET: ... = <T as DeviceImpl>::RESET::map( || realize_fn_unsafe::<T>); } You can also keep out of the macro the class_init method itself. Here I'm adding it to DeviceImplUnsafe: pub trait DeviceImplUnsafe { unsafe fn class_init(klass: *mut ObjectClass, data: *mut c_void) { let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap(); unsafe { dc.as_mut().realize = Self::REALIZE; bindings::device_class_set_legacy_reset( dc.as_mut(), Self::RESET); device_class_set_props(dc.as_mut(), <Self as DeviceInfo>::PROPS); if let Some(vmsd) = <Self as DeviceInfo>::VMSTATE { dc.as_mut().vmsd = vmsd; } if let Some(cat) = <Self as DeviceInfo>::CATEGORY { dc.as_mut().category = cat; } // maybe in the future for unparent // <Self as ObjectImplUnsafe>::class_init(klass, data) } } } And with all this machinery in place the device_class_init! macro becomes simply device_class_init!(PL011State); (because the arguments have moved to DeviceInfo or DeviceImpl). Why is this important? Because the only code transformation is the generation of properties and vtables, and the bindings can be developed almost entirely in qemu_api instead of qemu_api_macros. This has several advantages: 1) it's much easier: simpler error messages, no macro indirection, no super long global crate paths 2) it allows simplifying the pl011 code piecewise, even before introducing procedural macro code 3) it's easier to add comments It also becomes much easier to separate the work in separate patches, or even separate series. Replacing the arguments to device_class_init!() with DeviceImpl + DeviceImplUnsafe can be introduced first: posted, reviewed, merged. All the remaining tasks are pretty much independent: 1) splitting out ObjectInfo and introducing #[object] to automate it (i.e. extending derive(Object)) 2) splitting out DeviceInfo and introducing #[property] to automate it (i.e. derive(Device)) 3) the #[qom_class_init] macro A final word: I absolutely don't want you to think that your work is of no value. It's totally okay to throw away the first version of something. You showed that it _is_ possible to have idiomatic code with the help of procedural macros. Even if the implementation can be improved, the _idea_ remains yours. Paolo
On Fri, 25 Oct 2024 at 15:01, Paolo Bonzini <pbonzini@redhat.com> wrote: > > (Generally, don't read too much in the code - the syntax might have > issues but you get the idea). > > > Anyhow, going forward to the property attribute: > > > + #[property(name = c"migrate-clk", qdev_prop = qdev_prop_bool)] > > There are two issues here: > > First, the default is true, so 1) this has to be fixed in QEMU (will > do), 2) it is important to support it in #[property()]. TODO, it was not ignored just planned as next > > Second, this provides a false sense of safety, because I could specify > qdev_prop_chr here. Instead, the qdev_prop type should be derived by > the field type. TODO, if you recall we had that discussion about external statics, that was what I was looking into back then. > Third, since we are at it there's no need to use c"" in the attribute. > The c_str!() macro that I am adding for backwards compatibility to old > versions of Rust might actually come in handy here. TODO, you can you use both LitStr and LitCStr in the macro > > The part where I have most comments, and some ideas of how to make your > work a little more maintainable, is the implementation of class_init > (and all that depends on it). > > Let's start with these generated functions: > > > + pub unsafe extern "C" fn #realize_fn( > > + dev: *mut ::qemu_api::bindings::DeviceState, > > + errp: *mut *mut ::qemu_api::bindings::Error, > > + ) { > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > + unsafe { > > + ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); > > + } > > + } > > + > > + #[no_mangle] > > + pub unsafe extern "C" fn #reset_fn( > > + dev: *mut ::qemu_api::bindings::DeviceState, > > + ) { > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > + unsafe { > > + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); > > + } > > + } > > This can be handled entirely in Rust code, outside the macro. If you add: Why? I don't understand what this solves. These are *just* trampoline functions to call the Rust-abi code. > > unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>( > dev: *mut DeviceState, > errp: *mut *mut Error, > ) { > let mut instance = NonNull::new(dev.cast::<T>()). > expect("Expected dev to be a non-null pointer"); > unsafe { > ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); > } > } > > unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>( > dev: *mut ::qemu_api::bindings::DeviceState, > ) { > let mut instance = NonNull::new(dev.cast::<T>()). > expect("Expected dev to be a non-null pointer"); > unsafe { > ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); > } > } > > then the functions can be used directly instead of #realize_fn and > #reset_fn with a blanket implementation of DeviceImplUnsafe: > So just rename them and put a generic argument. Still not seeing any gain here. > > unsafe impl DeviceImplUnsafe for T: DeviceImpl { > const REALIZE: ... = Some(realize_fn_unsafe::<T>); > const RESET: ... = Some(realize_fn_unsafe::<T>); > } > > Going on to the implementation of the safe functions: > > > +impl DeviceImpl for PL011State { > > + fn realize(&mut self) { > > These are not quite traits. First, you can implement only some of the > functions. This is called "default implementations" in Rust > Second, if you don't implement them they are not overwritten > by the class_init method. WYM overwritten? That we hook up the empty stub instead of a NULL function pointer? > So this points to a different implementation as an attribute macro, > which is able to rewrite everything in the body of the impl. For example: > > #[qom_class_init] > impl DeviceImpl for PL011State { > fn realize(&mut self) { ... } > fn reset(&mut self) { ... } > > const VMSTATE: ... = {} > const CATEGORY: ... = {} > } > > can be transformed into: > > #[qom_class_init] > impl DeviceImpl for PL011State { > // fns are wrapped and transformed into consts > const REALIZE: Option<fn(&mut self)> = { > fn realize(&mut self) { ... } > Some(realize) > }; > const RESET: Option<fn(&mut self)> = { > fn reset(&mut self) { ... } > Some(reset) > }; > > // while associated consts (and perhaps types?) remain as is > const VMSTATE: ... = {} > const CATEGORY: ... = {} > } > > The above blanket implementation of DeviceImplUnsafe is easily adjusted > to support non-overridden methods: > > unsafe impl DeviceImplUnsafe for T: DeviceImpl { > const REALIZE: ... = <T as DeviceImpl>::REALIZE::map( > || realize_fn_unsafe::<T>); > const RESET: ... = <T as DeviceImpl>::RESET::map( > || realize_fn_unsafe::<T>); > } > > > You can also keep out of the macro the class_init method itself. Here > I'm adding it to DeviceImplUnsafe: > > pub trait DeviceImplUnsafe { > unsafe fn class_init(klass: *mut ObjectClass, data: *mut c_void) { > let mut dc = NonNull::new(klass.cast::<DeviceClass>()).unwrap(); > unsafe { > dc.as_mut().realize = Self::REALIZE; > bindings::device_class_set_legacy_reset( > dc.as_mut(), Self::RESET); > device_class_set_props(dc.as_mut(), > <Self as DeviceInfo>::PROPS); > if let Some(vmsd) = <Self as DeviceInfo>::VMSTATE { > dc.as_mut().vmsd = vmsd; > } > if let Some(cat) = <Self as DeviceInfo>::CATEGORY { > dc.as_mut().category = cat; > } > > // maybe in the future for unparent > // <Self as ObjectImplUnsafe>::class_init(klass, data) > } > } > } > > And with all this machinery in place the device_class_init! macro > becomes simply > > device_class_init!(PL011State); > > (because the arguments have moved to DeviceInfo or DeviceImpl). > > > Why is this important? Because the only code transformation is the > generation of properties and vtables, and the bindings can be developed > almost entirely in qemu_api instead of qemu_api_macros. This has > several advantages: > > 1) it's much easier: simpler error messages, no macro indirection, no > super long global crate paths Hard no, sorry. Error messages can definitely be generated from proc macros. Long crate paths; that's just a matter of using imports or changing names. > > 2) it allows simplifying the pl011 code piecewise, even before > introducing procedural macro code Not sure how that is relevant can you explain? > > 3) it's easier to add comments > > > It also becomes much easier to separate the work in separate patches, or > even separate series. Replacing the arguments to device_class_init!() > with DeviceImpl + DeviceImplUnsafe can be introduced first: posted, > reviewed, merged. All the remaining tasks are pretty much independent: > > 1) splitting out ObjectInfo and introducing #[object] to automate it > (i.e. extending derive(Object)) > > 2) splitting out DeviceInfo and introducing #[property] to automate it > (i.e. derive(Device)) > > 3) the #[qom_class_init] macro I disagree with this approach at the moment. This patch is in an acceptable state albeit a bit longish while all these suggestions would merely cause more running around making changes for no real gain while also delaying review and merging. > > A final word: I absolutely don't want you to think that your work is of > no value. It's totally okay to throw away the first version of > something. You showed that it _is_ possible to have idiomatic code with > the help of procedural macros. Even if the implementation can be > improved, the _idea_ remains yours. I don't know what this is in reference to :P What work and throwing away are you talking about? > Paolo >
[snipping to concentrate on the QOM code generation] On Fri, Oct 25, 2024 at 4:05 PM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > The part where I have most comments, and some ideas of how to make your > > work a little more maintainable, is the implementation of class_init > > (and all that depends on it). > > > > Let's start with these generated functions: > > > > > + pub unsafe extern "C" fn #realize_fn( > > > + dev: *mut ::qemu_api::bindings::DeviceState, > > > + errp: *mut *mut ::qemu_api::bindings::Error, > > > + ) { > > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > > + unsafe { > > > + DeviceImpl::realize(instance.as_mut()); > > > + } > > > + } > > > + > > > + #[no_mangle] > > > + pub unsafe extern "C" fn #reset_fn( > > > + dev: *mut ::qemu_api::bindings::DeviceState, > > > + ) { > > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > > + unsafe { > > > + DeviceImpl::reset(instance.as_mut()); > > > + } > > > + } > > > > This can be handled entirely in Rust code, outside the macro. If you add: > > Why? I don't understand what this solves. These are *just* trampoline > functions to call the Rust-abi code. Yes, and there is no need to place them in the procedural macros. Generally it's a good rule to do as little as possible in procedural macros, and move as much code as possible outside; this often means to generic functions or a declarative macros. For some examples you can look at Linux; in rust/macros/zeroable.rs for example the only "quote!" is quote! { ::kernel::__derive_zeroable!( parse_input: @sig(#(#rest)*), @impl_generics(#(#new_impl_generics)*), @ty_generics(#(#ty_generics)*), @body(#last), ); } For more information, see https://www.youtube.com/watch?v=DMLBBZBlKis > > unsafe extern "C" fn realize_fn_unsafe<T: DeviceImpl>(...) {} > > unsafe extern "C" fn reset_fn_unsafe<T: DeviceImpl>(...) {} > > So just rename them and put a generic argument. Still not seeing any gain here. The gain is that you have a much shorter macro implementation, a shorter expansion which makes it easier to debug the macro, and an implementation of realize_fn_unsafe<>() and reset_fn_unsafe<>() that is isolated. > > Going on to the implementation of the safe functions: > > > > > +impl DeviceImpl for PL011State { > > > + fn realize(&mut self) { > > > > These are not quite traits. First, you can implement only some of the > > functions. > > This is called "default implementations" in Rust Sort of. Yes, this is related to default implementations, but the default implementation of a QOM method is not "call whatever is declared in the trait". It's "call whatever is defined by the superclass", which is represented by None. > > Second, if you don't implement them they are not overwritten > > by the class_init method. > > WYM overwritten? That we hook up the empty stub instead of a NULL > function pointer? Not really the NULL function pointer: the value left by the superclass. In other words, the class_init function should not have // realize is a function in a trait dc.as_mut().realize = Self::realize; but rather // realize is a constant Option<fn(...)> in a trait if let Some(realize_fn) = Self::REALIZE { dc.as_mut().realize = Self::realize_fn; } > > Why is this important? Because the only code transformation is the > > generation of properties and vtables, and the bindings can be developed > > almost entirely in qemu_api instead of qemu_api_macros. This has > > several advantages: > > > > 1) it's much easier: simpler error messages, no macro indirection, no > > super long global crate paths > > Hard no, sorry. Error messages can definitely be generated from proc > macros. Long crate paths; that's just a matter of using imports or > changing names. I don't mean error messages from proc macros. I mean that debugging macros that are this complex (does not matter if procedural or declarative) is hell. If you move code outside the macro, to generic functions and blanket trait implementations, error messages from the compiler are simpler. One additional benefit is that the code is compiled _always_, not just if the macro hits a specific code path. It simplifies the testing noticeably. Finally, without -Zmacro-backtrace (nightly only) debugging procedural macros is a tedious matter of using "cargo expand". With -Zmacro-backtrace it's better but we don't want developers to install nightly to develop QEMU. Note that this code is not write-once; it will be extended for example as soon as we have a device that can fail realization. > Long crate paths; that's just a matter of using imports or > changing names. Only to some extent. See how many ::core::ffi:: or ::core::mem:: are there in the current macros. > > 2) it allows simplifying the pl011 code piecewise, even before > > introducing procedural macro code > > Not sure how that is relevant can you explain? Alex asked for a way to validate the expansion of the macro. If the procedural macro is simple and the code is outside, then reviewers can easily compare the pl011 code and the qemu-api-macros code, and see that they expand to the same thing. Try to put yourself into the mindset of the reviewer. If I see a patch like +#[qom_class_init] impl DeviceImpl for PL011State { - const REALIZE: Option<fn(&mut self)> = { fn realize(&mut self) { ... } - Some(realize) - }; - const RESET: Option<fn(&mut self)> = { fn reset(&mut self) { ... } - Some(reset) - }; const VMSTATE: ... = ...; const CATEGORY: ... = ...; } ... then it's quite obvious what to expect from the expansion of the #[qom_class_init] attribute macro. > > It also becomes much easier to separate the work in separate patches, or > > even separate series. Replacing the arguments to device_class_init!() > > with DeviceImpl + DeviceImplUnsafe can be introduced first: posted, > > reviewed, merged. All the remaining tasks are pretty much independent: > > > > 1) splitting out ObjectInfo and introducing #[object] to automate it > > (i.e. extending derive(Object)) > > > > 2) splitting out DeviceInfo and introducing #[property] to automate it > > (i.e. derive(Device)) > > > > 3) the #[qom_class_init] macro > > I disagree with this approach at the moment. This patch is in an > acceptable state albeit a bit longish while all these suggestions > would merely cause more running around making changes for no real > gain while also delaying review and merging. I will be very clear: no, this patch is very far from an acceptable state. It's a wonderful prototype, just like hw/char/pl011, but at this point we're not accepting prototype-quality Rust code anymore: we're turning the existing prototype, which has a manageable but still large size, into the real thing. To sum up the basic points underlying the suggestions, my review comments are: - the syntax of #[device(...)] is arbitrary. You are keeping things that obviously refer to the QOM object (type name, parent type name) far from #[derive(Object)]. You are putting in #[device()] things that apply to the QOM object class (e.g. class_name_override) rather than the QOM device class. Finally, you are putting in #[device()] the vmstate, which is not self-contained (due to subsections). - the split in modules needs more work. For example, why is Migrateable under qemu_api::objects instead of qemu_api::device_class? - the part that would require the fewest changes is probably #[property], but we need to be careful about not introducing "fake safety". - to simplify splitting the patches, start by moving code out of the existing declarative macros and into generic code or declarative macros - to simply reviewing the patches, only use the procedural macros as a syntactic sugar, and do as little code generation as possible in the procedural macros themselves - splitting the ObjectImpl/DeviceImpl traits in two (one for code generated by derive-macros; one for stuff that is definde outside the struct declaration) could be a way to work incrementally, adding more and more parts of the class definition to the procedural macro but without doing everything at once - it must be possible to specify non-overridden DeviceClass or ObjectClass functions The suggestions are just a way to achieve these objectives. In particular, the suggestions are about how to achieve the bullets in the above list starting from the third. Also, this series *must* come after the other cleanups and CI integration that have been posted, for several reasons. As to the cleanups, right now we *already* have in the tree code that is duplicate, dead or untested. Removing that, and ensuring that it does not regress, is a priority over everything else. As to CI integration, first, changes like this one must be fully bisectable, which requires having working CI before. Second, if we want to advertise 9.2 as supporting --enable-rust (even if experimental), we need a way to run automated tests on at least _some_ of the platforms that we support. So my plan right now is to post a pull request for part 1 next week, and to include part 2 (including your work on migration and Luminary, but without the procedural macro) in -rc1. This work is for 10.0. > > A final word: I absolutely don't want you to think that your work is of > > no value. It's totally okay to throw away the first version of > > something. You showed that it _is_ possible to have idiomatic code with > > the help of procedural macros. Even if the implementation can be > > improved, the _idea_ remains yours. > > I don't know what this is in reference to :P What work and throwing > away are you talking about? I was talking in general. When working in Rust I often found that the first thing I came up with was crap, and the main value of it was in learning. Sometimes I came up with the second version on my own, sometimes it was pointed out during review. But even if it's pointed out during review, the reviewer doesn't in any way think bad of you or your work. Paolo
I disagree with your points, Paolo. A patch review should not be "Fear Uncertainty and Doubt" but point out actual problems with the code. Please do not proceed with this conversation since it's not productive. If this series misses softfreeze because of code problems or by people delaying the review/merge, we can figure out what to do when we get to it.
Hello, here is my second attempt to review this, this time placing the remarks as close as possible to the code that is affected. However, the meat is the same as in my previous replies to the 03/11 thread. I hope this shows that I have practical concerns about the patch and it's not just FUD that it's not acceptable. On 10/24/24 16:03, Manos Pitsidianakis wrote: > Add a new derive procedural macro to declare device models. Add > corresponding DeviceImpl trait after already existing ObjectImpl trait. > At the same time, add instance_init, instance_post_init, > instance_finalize methods to the ObjectImpl trait and call them from the > ObjectImplUnsafe trait, which is generated by the procedural macro. > > This allows all the boilerplate device model registration to be handled > by macros, and all pertinent details to be declared through proc macro > attributes or trait associated constants and methods. > > The device class can now be generated automatically and the name can be > optionally overridden: > > ------------------------ >8 ------------------------ > #[repr(C)] > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > #[device(class_name_override = PL011Class)] > /// PL011 Device Model in QEMU > pub struct PL011State { The first design issue is already visible here in this example. I could place the same comment when the code appears in rust/hw/char/pl011, but it's easier to do it here. You have two derive macros, Object and Device. Object is derived by all objects (even if right now we have only devices), Device is derived by devices only. The class name is a property of any object, not just devices. It should not be part of the #[device()] attribute. #[derive(Device)] and #[device()] instead should take care of properties and categories (and possibly vmstate, but I'm not sure about that and there's already enough to say about this patch). You also have no documentation, which means that users will have no idea of what are the other sub-attributes of #[device()], including the difference between class_name and class_name_override, or how categories are defined. Even if we don't have support for rustdoc yet in tree, we should have all the needed documentation as soon as the API moves from "ad hoc usage of C symbols" to idiomatic. > Object methods (instance_init, etc) methods are now trait methods: > > ------------------------ >8 ------------------------ > /// Trait a type must implement to be registered with QEMU. > pub trait ObjectImpl { > type Class: ClassImpl; > const TYPE_NAME: &'static CStr; > const PARENT_TYPE_NAME: Option<&'static CStr>; > const ABSTRACT: bool; Class, TYPE_NAME, PARENT_TYPE_NAME, ABSTRACT should be defined via #[object()]. But actually, there is already room for defining a separate trait: /// # Safety /// /// - the first field of the struct must be of `Object` type, /// or derived from it /// /// - `TYPE` must match the type name used in the `TypeInfo` (no matter /// if it is defined in C or Rust). /// /// - the struct must be `#[repr(C)]` pub unsafe trait ObjectType { type Class: ClassImpl; const TYPE_NAME: &'static CStr; } ... because you can implement it even for classes that are defined in C code. Then #[derive(Object)] can find the TYPE_NAME directly from the first field of the struct, i.e. parent_obj: SysBusDevice; becomes const PARENT_TYPE_NAME: Option<&'static CStr> = Some(<SysBusDevice as TypeImpl>::TYPE_NAME); while #[object()] would be just #[object(class_type = PL011Class, type_name = "pl011")] Accessing the type of the first field is easy using the get_fields() function that Junjie added at https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/ This shows another reason why I prefer to get CI to work first. Having to do simple, but still non-trivial work, often provides code that can be reused in more complex setups. > unsafe fn instance_init(&mut self) {} > fn instance_post_init(&mut self) {} > fn instance_finalize(&mut self) {} > } In the trait, having a default implementation that is empty works (unlike for realize/reset, as we'll see later). So this is a bit simpler. However, instance_finalize should have a non-empty default implementation: std::ptr::drop_in_place(self); which should be okay for most devices. Alternatively, leave out instance_post_init() and instance_finalize() until we need them, and put the drop_in_place() call directly in the unsafe function that goes in the TypeInfo. > ------------------------ >8 ------------------------ > > Device methods (realize/reset etc) are now safe idiomatic trait methods: > > ------------------------ >8 ------------------------ > /// Implementation methods for device types. > pub trait DeviceImpl: ObjectImpl { > fn realize(&mut self) {} > fn reset(&mut self) {} > } > ------------------------ >8 ------------------------ This is an incorrect definition of the trait. The default definition of device methods is not "empty", it's "just reuse the superclass implementation". In particular, this means that PL011LuminaryState right now cannot use #[derive(Device)]. > The derive Device macro is responsible for creating all the extern "C" FFI > functions that QEMU needs to call these methods. This is unnecessary. It is perfectly possible to write the extern "C" functions (class_init, realize, reset) just once as either type-generic functions, or functions in a trait. More on this later. > diff --git a/rust/qemu-api/src/objects.rs b/rust/qemu-api/src/objects.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..5c6762023ed6914f9c6b7dd16a5e07f778c2d4fa > --- /dev/null > +++ b/rust/qemu-api/src/objects.rs > @@ -0,0 +1,90 @@ > +// Copyright 2024, Linaro Limited > +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +//! Implementation traits for QEMU objects, devices. > + > +use ::core::ffi::{c_int, c_void, CStr}; > + > +use crate::bindings::{DeviceState, Error, MigrationPriority, Object, ObjectClass, TypeInfo}; > + > +/// Trait a type must implement to be registered with QEMU. > +pub trait ObjectImpl { > + type Class: ClassImpl; > + const TYPE_NAME: &'static CStr; > + const PARENT_TYPE_NAME: Option<&'static CStr>; > + const ABSTRACT: bool; These consts should entirely be derived from the #[object()] attribute. You can facilitate the split by having two traits, one for things derived from the attribute (the above four), and one for the vtable. > + unsafe fn instance_init(&mut self) {} > + fn instance_post_init(&mut self) {} > + fn instance_finalize(&mut self) {} > +} See above remark on the default implementation of instance_finalize. > +/// The `extern`/`unsafe` analogue of [`ObjectImpl`]; it is used internally by `#[derive(Object)]` > +/// and should not be implemented manually. > +pub unsafe trait ObjectImplUnsafe { > + const TYPE_INFO: TypeInfo; > + > + const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; > + const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; > + const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)>; > +} > + This trait is not needed at all, because it really has juts one implementation. The fact that there is just one implementation is hidden by the fact that you are generating the code instead of relying on the type system. All you need is a single function, which will be called via the module_init mechanism: fn rust_type_register<T: ObjectImpl>() { let TypeInfo ti = ...; unsafe { type_register(&ti); } } > > +/// Methods for QOM class types. > +pub trait ClassImpl { > + type Object: ObjectImpl; > + > + unsafe fn class_init(&mut self, _data: *mut core::ffi::c_void) {} > + unsafe fn class_base_init(&mut self, _data: *mut core::ffi::c_void) {} > +} > + This trait (or more precisely class_init and class_base_init) is not needed. class_base_init is only needed in very special cases, we can just decide they won't be available in Rust for now and possible for ever. As to class_init device XYZ would only need a non-empty class_init method if we added support for the _data argument. But then we would need a way to provide the type of _data, and to cast _data to the appropriate type; we would also need a way to provide a mapping from multiple data objects to multiple type names, which is hard to do because right now each Rust struct has a single type name associated. So, let's just keep only the auto-generated class_init for simplicity. If we can just decide that, if device XYZ has superclass FooDevice, it implements FooDeviceImpl and class_init is provided by the FooDevice bindings. I can't really say if the "type Object" part is needed. I couldn't offhand find anything that uses it, but I may have missed it. If so, it can be in ClassImplUnsafe. > +/// The `extern`/`unsafe` analogue of [`ClassImpl`]; it is used internally by `#[derive(Object)]` > +/// and should not be implemented manually. > +pub unsafe trait ClassImplUnsafe { > + const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)>; > + const CLASS_BASE_INIT: Option< > + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), > + >; > +} Again, no need to have CLASS_BASE_INIT here. > +/// Implementation methods for device types. > +pub trait DeviceImpl: ObjectImpl { > + fn realize(&mut self) {} > + fn reset(&mut self) {} > +} These unfortunately cannot be functions. Doing so forces the class_init method to assign both dc->reset and dc->realize for _all_ classes, whereas for example PL011LuminaryClass would not override either. Therefore, the definition must be pub trait DeviceImpl: ObjectImpl { const REALIZE: Option<fn realize(&mut self)> = None; const RESET: Option<fn realize(&mut self)> = None; } Yes, it's uglier, but we cannot escape the fact that we're implementing something that Rust doesn't have natively (inheritance). :( So we cannot use language features meant for a completely different kind of polymorphism. > +/// The `extern`/`unsafe` analogue of [`DeviceImpl`]; it is used internally by `#[derive(Device)]` > +/// and should not be implemented manually. > +pub unsafe trait DeviceImplUnsafe { > + const REALIZE: Option<unsafe extern "C" fn(dev: *mut DeviceState, _errp: *mut *mut Error)>; > + const RESET: Option<unsafe extern "C" fn(dev: *mut DeviceState)>; > +} This trait is also unnecessary, because all that you need is a single function: fn rust_device_class_init<T: DeviceImpl>( klass: *mut ObjectClass, _data: *mut c_void) defined outside the procedural macro. #[derive(Device)] can define ClassImplUnsafe to point CLASS_INIT to rust_device_class_init. (Later, rust_device_class_init() can be moved into a trait so that it's possible to define other classes of devices, for example PCI devices. Note that such an extension would be much easier, than if it was _required_ to touch the procedural macro). > > +/// Constant metadata and implementation methods for types with device migration state. > +pub trait Migrateable: DeviceImplUnsafe { > + const NAME: Option<&'static CStr> = None; > + const UNMIGRATABLE: bool = true; > + const EARLY_SETUP: bool = false; > + const VERSION_ID: c_int = 1; > + const MINIMUM_VERSION_ID: c_int = 1; > + const PRIORITY: MigrationPriority = MigrationPriority::MIG_PRI_DEFAULT; > + > + unsafe fn pre_load(&mut self) -> c_int { > + 0 > + } > + unsafe fn post_load(&mut self, _version_id: c_int) -> c_int { > + 0 > + } > + unsafe fn pre_save(&mut self) -> c_int { > + 0 > + } > + unsafe fn post_save(&mut self) -> c_int { > + 0 > + } > + unsafe fn needed(&mut self) -> bool { > + false > + } > + unsafe fn dev_unplug_pending(&mut self) -> bool { > + false > + } > +} Premature. No need to add this trait until you add support for migration. > diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs > deleted file mode 100644 > index df54edbd4e27e7d2aafc243355d1826d52497c21..0000000000000000000000000000000000000000 > --- a/rust/qemu-api/src/tests.rs > +++ /dev/null > @@ -1,49 +0,0 @@ Nope. Fix the test, don't remove it. > -#[derive(Debug, qemu_api_macros::Object)] > +#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > +#[device(class_name_override = PL011Class)] > /// PL011 Device Model in QEMU > pub struct PL011State { > pub parent_obj: SysBusDevice, > @@ -51,6 +52,7 @@ pub struct PL011State { > pub read_count: usize, > pub read_trigger: usize, > #[doc(alias = "chr")] > + #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] (See earlier comments on accepting only a LitStr and deriving qdev_prop from the type). > +impl DeviceImpl for PL011State { > + fn realize(&mut self) { > + ... > + } > + > + fn reset(&mut self) { > + ... > + } This extractions of code into DeviceImpl is good. However, as I said above, I'm not sure about the trait itself. I'll remark later when I encounter the definition. > +impl qemu_api::objects::Migrateable for PL011State {} Premature. Before moving on to the procedural macro code, my proposal to split the patches is: 1) introduce the trait ObjectType, define it for Object, DeviceState and SysBusDevice. 2) introduce the traits ObjectImpl, DeviceImpl and ClassImplUnsafe. Define the first two for PL011State. 3) add to common code the wrappers that call into DeviceImpl, removing them from PL011State 4) introduce the functions rust_type_register and rust_device_class_init that use the traits. 5) remove most arguments of device_class_init!(), using the infrastructure introduced in the previous two steps 6) split ObjectImpl into a part that will be covered by #[object()], let's call it ObjectInfo 7) add implementation of #[object()], replace PL011State's implementation of ObjectInfo with #[object()] 8) split DeviceImpl into a part that will be covered by #[device()] (properties and categories), let's call it DeviceInfo 9) add #[derive(Device) and implementation of #[device()], replace PL011State's implementation of DeviceInfo with #[device()] Where 1-5 should be submitted as a separate series, one that does not touch procedural macros *at all* and just generalizes the pl011 code that defines QOM types. Anyhow, I'll continue reviewing the procedural macro code. > +#[derive(Debug, Default)] > +struct DeriveContainer { > + category: Option<syn::Path>, > + class_name: Option<syn::Ident>, > + class_name_override: Option<syn::Ident>, > +} Rename to DeviceAttribute. > +impl Parse for DeriveContainer { > + fn parse(input: ParseStream) -> Result<Self> { syn::Result represents a parse error, not an error in the allowed syntax of the attribute. Below, you're using panic! and unwrap(), but probably instead of syn::Result we need to have something like pub enum Error { CompileError(syn::Span, String), ParseError(syn::Error) } which extends the CompileError enum of https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/ and is amenable to use with "?". In particular, note the idiom used by the root derive_offsets() functions: let input = parse_macro_input!(input as DeriveInput); let expanded = derive_offsets_or_error(input). unwrap_or_else(Into::into); TokenStream::from(expanded) which works via an "impl From<CompileError> for proc_macro2::TokenStream". I believe that most of the benefit of this series (basically, all except the #[property] attribute) can be obtained without the procedural macro. Therefore, once we do add the procedural macro, we should not have it panic on errors. > + let _: syn::Token![#] = input.parse()?; > + let bracketed; > + _ = syn::bracketed!(bracketed in input); > + assert_eq!(DEVICE, bracketed.parse::<syn::Ident>()?); > + let mut retval = Self { > + category: None, > + class_name: None, > + class_name_override: None, > + }; > + let content; > + _ = syn::parenthesized!(content in bracketed); > + while !content.is_empty() { > + let value: syn::Ident = content.parse()?; > + if value == CLASS_NAME { > + let _: syn::Token![=] = content.parse()?; > + if retval.class_name.is_some() { > + panic!("{} can only be used at most once", CLASS_NAME); > + } No panic!, instead we need to return a compile_error!() TokenStream, or as above a Result<> that can be converted to compile_error!() up in the chain. > + retval.class_name = Some(content.parse()?); Please make this function a separate trait in utilities: trait AttributeParsing { const NAME: Symbol; fn set(&mut self, key: &syn::Ident, input: &ParseStream) -> Result<()>; fn parse(input: ParseStream) -> Result<Self> { ... } } Then the "if" can move to the struct-specific implementation of AttributeParsing::set, while the rest can move to the default implementation of AttributeParsing::parse. #[property()] and #[device()] (and also the proposed #[object()]) can then share the implementation of AttributeParsing::parse. > + } else if value == CLASS_NAME_OVERRIDE { > + let _: syn::Token![=] = content.parse()?; > + if retval.class_name_override.is_some() { > + panic!("{} can only be used at most once", CLASS_NAME_OVERRIDE); > + }> + retval.class_name_override = Some(content.parse()?); > + } else if value == CATEGORY { > + let _: syn::Token![=] = content.parse()?; > + if retval.category.is_some() { > + panic!("{} can only be used at most once", CATEGORY); > + } > + let lit: syn::LitStr = content.parse()?; > + let path: syn::Path = lit.parse()?; Do I understand that this would be category = "foo::bar::Baz" ? If so, why the extra quotes? There can actually be more than one category, so at least add a TODO here. > +#[derive(Debug)] > +struct QdevProperty { > + name: Option<syn::LitCStr>, Just LitStr. Convert it to CString in the macro. You can reuse the c_str!() macro that I'm adding in the series to fix CI and support old rustc, i.e. quote! { ::qemu_api::c_str!(#name) } or something like that. > + qdev_prop: Option<syn::Path>, > +} > + > +impl Parse for QdevProperty { > + fn parse(input: ParseStream) -> Result<Self> { > + let _: syn::Token![#] = input.parse()?; > + let bracketed; > + _ = syn::bracketed!(bracketed in input); > + assert_eq!(PROPERTY, bracketed.parse::<syn::Ident>()?); > + let mut retval = Self { > + name: None, > + qdev_prop: None, > + }; > + let content; > + _ = syn::parenthesized!(content in bracketed); > + while !content.is_empty() { > + let value: syn::Ident = content.parse()?; > + if value == NAME { > + let _: syn::Token![=] = content.parse()?; > + if retval.name.is_some() { > + panic!("{} can only be used at most once", NAME); > + } > + retval.name = Some(content.parse()?); > + } else if value == QDEV_PROP { > + let _: syn::Token![=] = content.parse()?; > + if retval.qdev_prop.is_some() { > + panic!("{} can only be used at most once", QDEV_PROP); > + } > + retval.qdev_prop = Some(content.parse()?); > + } else { > + panic!("unrecognized token `{}`", value); > + } > + > + if !content.is_empty() { > + let _: syn::Token![,] = content.parse()?; > + } > + } > + Ok(retval) See above with respect to the duplicated code with #[device()]. > + let derive_container: DeriveContainer = input > + .attrs > + .iter() > + .find(|a| a.path() == DEVICE) > + .map(|a| syn::parse(a.to_token_stream().into()).expect("could not parse device attr")) > + .unwrap_or_default(); > + let (qdev_properties_static, qdev_properties_expanded) = make_qdev_properties(&input); Please put functions before their callers. > + let realize_fn = format_ident!("__{}_realize_generated", name); > + let reset_fn = format_ident!("__{}_reset_generated", name); > + > + let expanded = quote! { > + unsafe impl ::qemu_api::objects::DeviceImplUnsafe for #name { > + const REALIZE: ::core::option::Option< > + unsafe extern "C" fn( > + dev: *mut ::qemu_api::bindings::DeviceState, > + errp: *mut *mut ::qemu_api::bindings::Error, > + ), > + > = Some(#realize_fn); > + const RESET: ::core::option::Option< > + unsafe extern "C" fn(dev: *mut ::qemu_api::bindings::DeviceState), > + > = Some(#reset_fn); > + } > + > + #[no_mangle] Not needed. > + pub unsafe extern "C" fn #realize_fn( > + dev: *mut ::qemu_api::bindings::DeviceState, > + errp: *mut *mut ::qemu_api::bindings::Error, > + ) { > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > + unsafe { > + ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); > + } > + } > + > + #[no_mangle] Not needed. > + pub unsafe extern "C" fn #reset_fn( > + dev: *mut ::qemu_api::bindings::DeviceState, > + ) { > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > + unsafe { > + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); > + } > + } All this code depends on nothing but #name. This is not the C preprocessor; the way to do it in Rust is monomorphization as described above. > +fn gen_device_class( > + derive_container: DeriveContainer, > + qdev_properties_static: syn::Ident, > + name: &syn::Ident, > +) -> proc_macro2::TokenStream { > + let (class_name, class_def) = match ( > + derive_container.class_name_override, > + derive_container.class_name, > + ) { > + (Some(class_name), _) => { > + let class_expanded = quote! { > + #[repr(C)] > + pub struct #class_name { > + _inner: [u8; 0], > + } > + }; > + (class_name, class_expanded) > + } > + (None, Some(class_name)) => (class_name, quote! {}), > + (None, None) => { > + let class_name = format_ident!("{}Class", name); > + let class_expanded = quote! { > + #[repr(C)] > + pub struct #class_name { > + _inner: [u8; 0], > + } This should have a DeviceClass member, it should not be a dummy 0-byte type. Also, this should be generated by #[derive(Object)]. > + }; > + (class_name, class_expanded) > + } > + }; > + let class_init_fn = format_ident!("__{}_class_init_generated", class_name); > + let class_base_init_fn = format_ident!("__{}_class_base_init_generated", class_name); > + > + let (vmsd, vmsd_impl) = { > + let (i, vmsd) = make_vmstate(name); > + (quote! { &#i }, vmsd) > + }; > + let category = if let Some(category) = derive_container.category { > + quote! { > + const BITS_PER_LONG: u32 = ::core::ffi::c_ulong::BITS; > + let _: ::qemu_api::bindings::DeviceCategory = #category; > + let nr: ::core::ffi::c_ulong = #category as _; > + let mask = 1 << (nr as u32 % BITS_PER_LONG); > + let p = ::core::ptr::addr_of_mut!(dc.as_mut().categories).offset((nr as u32 / BITS_PER_LONG) as isize); > + let p: *mut ::core::ffi::c_ulong = p.cast(); > + let categories = p.read_unaligned(); > + p.write_unaligned(categories | mask); What's wrong with const BITS_PER_ELEMENT: u32 = ::core::mem::sizeof(dc.categories) / dc.categories.len() * 8; dc.categories[((nr as u32) / BITS_PER_ELEMENT) as usize] |= 1 << ((nr as u32) % BITS_PER_ELEMENT); ? > + #[no_mangle] > + pub unsafe extern "C" fn #class_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { > + { > + { > + let mut dc = > + ::core::ptr::NonNull::new(klass.cast::<::qemu_api::bindings::DeviceClass>()).unwrap(); And then "let mut dc = dc.as_mut()". Just write the conversion once. > + unsafe { > + dc.as_mut().realize = > + <#name as ::qemu_api::objects::DeviceImplUnsafe>::REALIZE; > + ::qemu_api::bindings::device_class_set_legacy_reset( > + dc.as_mut(), > + <#name as ::qemu_api::objects::DeviceImplUnsafe>::RESET > + ); As written elsewhere, these should be conditional. > + dc.as_mut().vmsd = #vmsd; > + #props > + #category > + }> + } All this code should be outside the macro, and should use trait consts instead of quoting. > + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); > + unsafe { > + ::qemu_api::objects::ClassImpl::class_init(klass.as_mut(), data); > + } > + } > + } > + #[no_mangle] > + pub unsafe extern "C" fn #class_base_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { > + { > + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); > + unsafe { > + ::qemu_api::objects::ClassImpl::class_base_init(klass.as_mut(), data); > + } > + } > + } > + > + #vmsd_impl > + } > +} > + > +fn make_vmstate(name: &syn::Ident) -> (syn::Ident, proc_macro2::TokenStream) { Not needed. Just let the user provide a VMStateDescription in DeviceImpl. I'm not sure if it's possible to make it a const; if not, it can be a function returning a &'static VMStateDescription. > + let vmstate_description_ident = format_ident!("__VMSTATE_{}", name); > + > + let pre_load = format_ident!("__{}_pre_load_generated", name); > + let post_load = format_ident!("__{}_post_load_generated", name); > + let pre_save = format_ident!("__{}_pre_save_generated", name); > + let post_save = format_ident!("__{}_post_save_generated", name); > + let needed = format_ident!("__{}_needed_generated", name); > + let dev_unplug_pending = format_ident!("__{}_dev_unplug_pending_generated", name); > + > + let migrateable_fish = quote! {<#name as ::qemu_api::objects::Migrateable>}; > + let vmstate_description = quote! { > + #[used] Attribute not needed. > + #[allow(non_upper_case_globals)] > + pub static #vmstate_description_ident: ::qemu_api::bindings::VMStateDescription = ::qemu_api::bindings::VMStateDescription { > + name: if let Some(name) = #migrateable_fish::NAME { > + name.as_ptr() > + } else { > + <#name as ::qemu_api::objects::ObjectImplUnsafe>::TYPE_INFO.name > + }, > + unmigratable: #migrateable_fish::UNMIGRATABLE, > + early_setup: #migrateable_fish::EARLY_SETUP, > + version_id: #migrateable_fish::VERSION_ID, > + minimum_version_id: #migrateable_fish::MINIMUM_VERSION_ID, > + priority: #migrateable_fish::PRIORITY, > + pre_load: Some(#pre_load), > + post_load: Some(#post_load), > + pre_save: Some(#pre_save), > + post_save: Some(#post_save), > + needed: Some(#needed), > + dev_unplug_pending: Some(#dev_unplug_pending), > + fields: ::core::ptr::null(), > + subsections: ::core::ptr::null(), > + }; > + > + #[no_mangle] Not needed (other occurrences below). > + pub unsafe extern "C" fn #pre_load(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::pre_load(instance.as_mut()) > + } > + } > + #[no_mangle] > + pub unsafe extern "C" fn #post_load(opaque: *mut ::core::ffi::c_void, version_id: core::ffi::c_int) -> ::core::ffi::c_int { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::post_load(instance.as_mut(), version_id) Again, introducing the Migrateable code and all these thunks is premature; but in any case, this can only return 0 or -1 so make Migrateable::post_load() return Result<(), ()>. > + } > + } > + #[no_mangle] > + pub unsafe extern "C" fn #pre_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::pre_save(instance.as_mut()) > + } > + } Likewise. > + #[no_mangle] > + pub unsafe extern "C" fn #post_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::post_save(instance.as_mut()) > + } > + } Likewise. > + #[no_mangle] > + pub unsafe extern "C" fn #needed(opaque: *mut ::core::ffi::c_void) -> bool { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::needed(instance.as_mut()) > + } > + } > + #[no_mangle] > + pub unsafe extern "C" fn #dev_unplug_pending(opaque: *mut ::core::ffi::c_void) -> bool { > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > + unsafe { > + ::qemu_api::objects::Migrateable::dev_unplug_pending(instance.as_mut()) > + } > + } > + }; > + > + let expanded = quote! { > + #vmstate_description > + }; > + (vmstate_description_ident, expanded) > +} > diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs > index 59aba592d9ae4c5a4cdfdc6f9b9b08363b8a57e5..7753a853fae72fc87e6dc642cf076c6d0c736345 100644 > --- a/rust/qemu-api-macros/src/lib.rs > +++ b/rust/qemu-api-macros/src/lib.rs > @@ -2,42 +2,21 @@ > // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > // SPDX-License-Identifier: GPL-2.0-or-later > > +#![allow(dead_code)] Why? > use proc_macro::TokenStream; > -use quote::{format_ident, quote}; > -use syn::{parse_macro_input, DeriveInput}; > + > +mod device; > +mod object; > +mod symbols; > +mod utilities; > > #[proc_macro_derive(Object)] > pub fn derive_object(input: TokenStream) -> TokenStream { > - let input = parse_macro_input!(input as DeriveInput); > - > - let name = input.ident; > - let module_static = format_ident!("__{}_LOAD_MODULE", name); > - > - let expanded = quote! { > - #[allow(non_upper_case_globals)] > - #[used] > - #[cfg_attr(target_os = "linux", link_section = ".ctors")] > - #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] > - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] > - pub static #module_static: extern "C" fn() = { > - extern "C" fn __register() { > - unsafe { > - ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO); > - } > - } > - > - extern "C" fn __load() { > - unsafe { > - ::qemu_api::bindings::register_module_init( > - Some(__register), > - ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM > - ); > - } > - } > - > - __load > - }; > - }; > + object::derive_object(input) Moving code to a separate file should be a separate patch from modifying the expansion of the macro. > diff --git a/rust/qemu-api-macros/src/symbols.rs b/rust/qemu-api-macros/src/symbols.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..f73768d228ed2b4d478c18336db56cb11e70f012 > --- /dev/null > +++ b/rust/qemu-api-macros/src/symbols.rs > @@ -0,0 +1,55 @@ > +// Copyright 2024, Linaro Limited > +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +use core::fmt; > +use syn::{Ident, Path}; > + > +#[derive(Copy, Clone, Debug)] > +pub struct Symbol(&'static str); > + > +pub const DEVICE: Symbol = Symbol("device"); > +pub const NAME: Symbol = Symbol("name"); > +pub const CATEGORY: Symbol = Symbol("category"); > +pub const CLASS_NAME: Symbol = Symbol("class_name"); > +pub const CLASS_NAME_OVERRIDE: Symbol = Symbol("class_name_override"); > +pub const QDEV_PROP: Symbol = Symbol("qdev_prop"); > +pub const MIGRATEABLE: Symbol = Symbol("migrateable"); > +pub const PROPERTIES: Symbol = Symbol("properties"); > +pub const PROPERTY: Symbol = Symbol("property"); Declare these in device.rs as needed, not here. This avoids "use symbols::*". It also allows making them not "pub", so that dead ones are detected by the compiler (e.g. MIGRATEABLE, PROPERTIES). > +pub fn assert_is_repr_c_struct(input: &DeriveInput, derive_macro: &'static str) { Nice but a bit overengineered. Unless you think/know that you'll have a use for Repr elsewhere, try sharing code with Junjie's macro https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/. Paolo
Thank you for the review comments Paolo. I will address any bits I did wrong and not much the rest, it's obvious you have a disagreement over how things are done and that's fine. This series does not attempt to solve everything at once and arguing again and again over "this Trait should have been OtherTrait and this thing should have been thing!()" is not productive. Your review style of relentless disagreement after disagreement is tiresome and impossible to deal with; it's like a denial of service for other human beings. I suggest you take a step back and take a deep breath before reviewing Rust patches again. I assure you I will make sure to address all your comments either in code, TODO comments, or patch messages. In the meantime, take it easy. On Sun, Oct 27, 2024 at 10:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > Hello, > > here is my second attempt to review this, this time placing the remarks > as close as possible to the code that is affected. However, the meat is > the same as in my previous replies to the 03/11 thread. > > I hope this shows that I have practical concerns about the patch and > it's not just FUD that it's not acceptable. > > On 10/24/24 16:03, Manos Pitsidianakis wrote: > > Add a new derive procedural macro to declare device models. Add > > corresponding DeviceImpl trait after already existing ObjectImpl trait. > > At the same time, add instance_init, instance_post_init, > > instance_finalize methods to the ObjectImpl trait and call them from the > > ObjectImplUnsafe trait, which is generated by the procedural macro. > > > > This allows all the boilerplate device model registration to be handled > > by macros, and all pertinent details to be declared through proc macro > > attributes or trait associated constants and methods. > > > > The device class can now be generated automatically and the name can be > > optionally overridden: > > > > ------------------------ >8 ------------------------ > > #[repr(C)] > > #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > > #[device(class_name_override = PL011Class)] > > /// PL011 Device Model in QEMU > > pub struct PL011State { > > The first design issue is already visible here in this example. I could > place the same comment when the code appears in rust/hw/char/pl011, but > it's easier to do it here. > > You have two derive macros, Object and Device. Object is derived by all > objects (even if right now we have only devices), Device is derived by > devices only. > > The class name is a property of any object, not just devices. It should > not be part of the #[device()] attribute. #[derive(Device)] and > #[device()] instead should take care of properties and categories (and > possibly vmstate, but I'm not sure about that and there's already enough > to say about this patch). > > > You also have no documentation, which means that users will have no idea > of what are the other sub-attributes of #[device()], including the > difference between class_name and class_name_override, or how categories > are defined. > > Even if we don't have support for rustdoc yet in tree, we should have > all the needed documentation as soon as the API moves from "ad hoc usage > of C symbols" to idiomatic. > > > Object methods (instance_init, etc) methods are now trait methods: > > > > ------------------------ >8 ------------------------ > > /// Trait a type must implement to be registered with QEMU. > > pub trait ObjectImpl { > > type Class: ClassImpl; > > const TYPE_NAME: &'static CStr; > > const PARENT_TYPE_NAME: Option<&'static CStr>; > > const ABSTRACT: bool; > > > Class, TYPE_NAME, PARENT_TYPE_NAME, ABSTRACT should be defined via > #[object()]. > > But actually, there is already room for defining a separate trait: > > /// # Safety > /// > /// - the first field of the struct must be of `Object` type, > /// or derived from it > /// > /// - `TYPE` must match the type name used in the `TypeInfo` (no matter > /// if it is defined in C or Rust). > /// > /// - the struct must be `#[repr(C)]` > pub unsafe trait ObjectType { > type Class: ClassImpl; > const TYPE_NAME: &'static CStr; > } > > ... because you can implement it even for classes that are defined in C > code. Then #[derive(Object)] can find the TYPE_NAME directly from the > first field of the struct, i.e. > > parent_obj: SysBusDevice; > > becomes > > const PARENT_TYPE_NAME: Option<&'static CStr> = > Some(<SysBusDevice as TypeImpl>::TYPE_NAME); > > while #[object()] would be just > > #[object(class_type = PL011Class, type_name = "pl011")] > > Accessing the type of the first field is easy using the get_fields() > function that Junjie added at > https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/ > > This shows another reason why I prefer to get CI to work first. Having > to do simple, but still non-trivial work, often provides code that can > be reused in more complex setups. > > > unsafe fn instance_init(&mut self) {} > > fn instance_post_init(&mut self) {} > > fn instance_finalize(&mut self) {} > > } > > In the trait, having a default implementation that is empty works > (unlike for realize/reset, as we'll see later). So this is a bit > simpler. However, instance_finalize should have a non-empty default > implementation: > > std::ptr::drop_in_place(self); > > which should be okay for most devices. > > Alternatively, leave out instance_post_init() and instance_finalize() > until we need them, and put the drop_in_place() call directly in the > unsafe function that goes in the TypeInfo. > > > ------------------------ >8 ------------------------ > > > > Device methods (realize/reset etc) are now safe idiomatic trait methods: > > > > ------------------------ >8 ------------------------ > > /// Implementation methods for device types. > > pub trait DeviceImpl: ObjectImpl { > > fn realize(&mut self) {} > > fn reset(&mut self) {} > > } > > ------------------------ >8 ------------------------ > > This is an incorrect definition of the trait. The default definition of > device methods is not "empty", it's "just reuse the superclass > implementation". In particular, this means that PL011LuminaryState > right now cannot use #[derive(Device)]. > > > The derive Device macro is responsible for creating all the extern "C" FFI > > functions that QEMU needs to call these methods. > > This is unnecessary. It is perfectly possible to write the extern "C" > functions (class_init, realize, reset) just once as either type-generic > functions, or functions in a trait. More on this later. > > > diff --git a/rust/qemu-api/src/objects.rs b/rust/qemu-api/src/objects.rs > > new file mode 100644 > > index 0000000000000000000000000000000000000000..5c6762023ed6914f9c6b7dd16a5e07f778c2d4fa > > --- /dev/null > > +++ b/rust/qemu-api/src/objects.rs > > @@ -0,0 +1,90 @@ > > +// Copyright 2024, Linaro Limited > > +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > +// SPDX-License-Identifier: GPL-2.0-or-later > > + > > +//! Implementation traits for QEMU objects, devices. > > + > > +use ::core::ffi::{c_int, c_void, CStr}; > > + > > +use crate::bindings::{DeviceState, Error, MigrationPriority, Object, ObjectClass, TypeInfo}; > > + > > +/// Trait a type must implement to be registered with QEMU. > > +pub trait ObjectImpl { > > + type Class: ClassImpl; > > + const TYPE_NAME: &'static CStr; > > + const PARENT_TYPE_NAME: Option<&'static CStr>; > > + const ABSTRACT: bool; > > These consts should entirely be derived from the #[object()] attribute. > You can facilitate the split by having two traits, one for things > derived from the attribute (the above four), and one for the vtable. > > > + unsafe fn instance_init(&mut self) {} > > + fn instance_post_init(&mut self) {} > > + fn instance_finalize(&mut self) {} > > +} > > See above remark on the default implementation of instance_finalize. > > > +/// The `extern`/`unsafe` analogue of [`ObjectImpl`]; it is used internally by `#[derive(Object)]` > > +/// and should not be implemented manually. > > +pub unsafe trait ObjectImplUnsafe { > > + const TYPE_INFO: TypeInfo; > > + > > + const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; > > + const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; > > + const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)>; > > +} > > + > > This trait is not needed at all, because it really has juts one > implementation. The fact that there is just one implementation is > hidden by the fact that you are generating the code instead of relying > on the type system. > > All you need is a single function, which will be called via the > module_init mechanism: > > fn rust_type_register<T: ObjectImpl>() { > let TypeInfo ti = ...; > unsafe { type_register(&ti); } > } > > > > > +/// Methods for QOM class types. > > +pub trait ClassImpl { > > + type Object: ObjectImpl; > > + > > + unsafe fn class_init(&mut self, _data: *mut core::ffi::c_void) {} > > + unsafe fn class_base_init(&mut self, _data: *mut core::ffi::c_void) {} > > +} > > + > > This trait (or more precisely class_init and class_base_init) is not > needed. class_base_init is only needed in very special cases, we can > just decide they won't be available in Rust for now and possible for ever. > > As to class_init device XYZ would only need a non-empty class_init > method if we added support for the _data argument. But then we would > need a way to provide the type of _data, and to cast _data to the > appropriate type; we would also need a way to provide a mapping from > multiple data objects to multiple type names, which is hard to do > because right now each Rust struct has a single type name associated. > > So, let's just keep only the auto-generated class_init for simplicity. > If we can just decide that, if device XYZ has superclass FooDevice, it > implements FooDeviceImpl and class_init is provided by the FooDevice > bindings. > > I can't really say if the "type Object" part is needed. I couldn't > offhand find anything that uses it, but I may have missed it. If so, it > can be in ClassImplUnsafe. > > > +/// The `extern`/`unsafe` analogue of [`ClassImpl`]; it is used internally by `#[derive(Object)]` > > +/// and should not be implemented manually. > > +pub unsafe trait ClassImplUnsafe { > > + const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)>; > > + const CLASS_BASE_INIT: Option< > > + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), > > + >; > > +} > > Again, no need to have CLASS_BASE_INIT here. > > > +/// Implementation methods for device types. > > +pub trait DeviceImpl: ObjectImpl { > > + fn realize(&mut self) {} > > + fn reset(&mut self) {} > > +} > > These unfortunately cannot be functions. Doing so forces the class_init > method to assign both dc->reset and dc->realize for _all_ classes, > whereas for example PL011LuminaryClass would not override either. > > Therefore, the definition must be > > pub trait DeviceImpl: ObjectImpl { > const REALIZE: Option<fn realize(&mut self)> = None; > const RESET: Option<fn realize(&mut self)> = None; > } > > Yes, it's uglier, but we cannot escape the fact that we're implementing > something that Rust doesn't have natively (inheritance). :( So we > cannot use language features meant for a completely different kind of > polymorphism. > > > +/// The `extern`/`unsafe` analogue of [`DeviceImpl`]; it is used internally by `#[derive(Device)]` > > +/// and should not be implemented manually. > > +pub unsafe trait DeviceImplUnsafe { > > + const REALIZE: Option<unsafe extern "C" fn(dev: *mut DeviceState, _errp: *mut *mut Error)>; > > + const RESET: Option<unsafe extern "C" fn(dev: *mut DeviceState)>; > > +} > > This trait is also unnecessary, because all that you need is a single > function: > > fn rust_device_class_init<T: DeviceImpl>( > klass: *mut ObjectClass, _data: *mut c_void) > > defined outside the procedural macro. #[derive(Device)] can define > ClassImplUnsafe to point CLASS_INIT to rust_device_class_init. > > (Later, rust_device_class_init() can be moved into a trait so that it's > possible to define other classes of devices, for example PCI devices. > Note that such an extension would be much easier, than if it was > _required_ to touch the procedural macro). > > > > > +/// Constant metadata and implementation methods for types with device migration state. > > +pub trait Migrateable: DeviceImplUnsafe { > > + const NAME: Option<&'static CStr> = None; > > + const UNMIGRATABLE: bool = true; > > + const EARLY_SETUP: bool = false; > > + const VERSION_ID: c_int = 1; > > + const MINIMUM_VERSION_ID: c_int = 1; > > + const PRIORITY: MigrationPriority = MigrationPriority::MIG_PRI_DEFAULT; > > + > > + unsafe fn pre_load(&mut self) -> c_int { > > + 0 > > + } > > + unsafe fn post_load(&mut self, _version_id: c_int) -> c_int { > > + 0 > > + } > > + unsafe fn pre_save(&mut self) -> c_int { > > + 0 > > + } > > + unsafe fn post_save(&mut self) -> c_int { > > + 0 > > + } > > + unsafe fn needed(&mut self) -> bool { > > + false > > + } > > + unsafe fn dev_unplug_pending(&mut self) -> bool { > > + false > > + } > > +} > > Premature. No need to add this trait until you add support for migration. > > > diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs > > deleted file mode 100644 > > index df54edbd4e27e7d2aafc243355d1826d52497c21..0000000000000000000000000000000000000000 > > --- a/rust/qemu-api/src/tests.rs > > +++ /dev/null > > @@ -1,49 +0,0 @@ > > Nope. Fix the test, don't remove it. > > > > -#[derive(Debug, qemu_api_macros::Object)] > > +#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] > > +#[device(class_name_override = PL011Class)] > > /// PL011 Device Model in QEMU > > pub struct PL011State { > > pub parent_obj: SysBusDevice, > > @@ -51,6 +52,7 @@ pub struct PL011State { > > pub read_count: usize, > > pub read_trigger: usize, > > #[doc(alias = "chr")] > > + #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] > > (See earlier comments on accepting only a LitStr and deriving qdev_prop > from the type). > > > +impl DeviceImpl for PL011State { > > + fn realize(&mut self) { > > + ... > > + } > > + > > + fn reset(&mut self) { > > + ... > > + } > > This extractions of code into DeviceImpl is good. However, as I said > above, I'm not sure about the trait itself. I'll remark later when I > encounter the definition. > > > +impl qemu_api::objects::Migrateable for PL011State {} > > Premature. > > Before moving on to the procedural macro code, my proposal to split the > patches is: > > 1) introduce the trait ObjectType, define it for Object, DeviceState and > SysBusDevice. > > 2) introduce the traits ObjectImpl, DeviceImpl and ClassImplUnsafe. > Define the first two for PL011State. > > 3) add to common code the wrappers that call into DeviceImpl, removing > them from PL011State > > 4) introduce the functions rust_type_register and rust_device_class_init > that use the traits. > > 5) remove most arguments of device_class_init!(), using the > infrastructure introduced in the previous two steps > > 6) split ObjectImpl into a part that will be covered by #[object()], > let's call it ObjectInfo > > 7) add implementation of #[object()], replace PL011State's > implementation of ObjectInfo with #[object()] > > 8) split DeviceImpl into a part that will be covered by #[device()] > (properties and categories), let's call it DeviceInfo > > 9) add #[derive(Device) and implementation of #[device()], replace > PL011State's implementation of DeviceInfo with #[device()] > > Where 1-5 should be submitted as a separate series, one that does not > touch procedural macros *at all* and just generalizes the pl011 code > that defines QOM types. > > > Anyhow, I'll continue reviewing the procedural macro code. > > > +#[derive(Debug, Default)] > > +struct DeriveContainer { > > + category: Option<syn::Path>, > > + class_name: Option<syn::Ident>, > > + class_name_override: Option<syn::Ident>, > > +} > > Rename to DeviceAttribute. > > > +impl Parse for DeriveContainer { > > + fn parse(input: ParseStream) -> Result<Self> { > > syn::Result represents a parse error, not an error in the allowed syntax > of the attribute. Below, you're using panic! and unwrap(), but probably > instead of syn::Result we need to have something like > > pub enum Error { > CompileError(syn::Span, String), > ParseError(syn::Error) > } > > which extends the CompileError enum of > https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/ > and is amenable to use with "?". In particular, note the idiom used by > the root derive_offsets() functions: > > let input = parse_macro_input!(input as DeriveInput); > let expanded = derive_offsets_or_error(input). > unwrap_or_else(Into::into); > > TokenStream::from(expanded) > > which works via an "impl From<CompileError> for proc_macro2::TokenStream". > > I believe that most of the benefit of this series (basically, all except > the #[property] attribute) can be obtained without the procedural macro. > Therefore, once we do add the procedural macro, we should not have it > panic on errors. > > > + let _: syn::Token![#] = input.parse()?; > > + let bracketed; > > + _ = syn::bracketed!(bracketed in input); > > + assert_eq!(DEVICE, bracketed.parse::<syn::Ident>()?); > > + let mut retval = Self { > > + category: None, > > + class_name: None, > > + class_name_override: None, > > + }; > > + let content; > > + _ = syn::parenthesized!(content in bracketed); > > + while !content.is_empty() { > > + let value: syn::Ident = content.parse()?; > > + if value == CLASS_NAME { > > + let _: syn::Token![=] = content.parse()?; > > + if retval.class_name.is_some() { > > + panic!("{} can only be used at most once", CLASS_NAME); > > + } > > No panic!, instead we need to return a compile_error!() TokenStream, or > as above a Result<> that can be converted to compile_error!() up in the > chain. > > > + retval.class_name = Some(content.parse()?); > > Please make this function a separate trait in utilities: > > trait AttributeParsing { > const NAME: Symbol; > fn set(&mut self, key: &syn::Ident, input: &ParseStream) -> Result<()>; > fn parse(input: ParseStream) -> Result<Self> { ... } > } > > Then the "if" can move to the struct-specific implementation of > AttributeParsing::set, while the rest can move to the default > implementation of AttributeParsing::parse. > > #[property()] and #[device()] (and also the proposed #[object()]) can > then share the implementation of AttributeParsing::parse. > > > + } else if value == CLASS_NAME_OVERRIDE { > > + let _: syn::Token![=] = content.parse()?; > > + if retval.class_name_override.is_some() { > > + panic!("{} can only be used at most once", CLASS_NAME_OVERRIDE); > > + }> + retval.class_name_override = Some(content.parse()?); > > + } else if value == CATEGORY { > > + let _: syn::Token![=] = content.parse()?; > > + if retval.category.is_some() { > > + panic!("{} can only be used at most once", CATEGORY); > > + } > > + let lit: syn::LitStr = content.parse()?; > > + let path: syn::Path = lit.parse()?; > > Do I understand that this would be > > category = "foo::bar::Baz" > > ? If so, why the extra quotes? There can actually be more than one > category, so at least add a TODO here. > > > +#[derive(Debug)] > > +struct QdevProperty { > > + name: Option<syn::LitCStr>, > > Just LitStr. Convert it to CString in the macro. You can reuse the > c_str!() macro that I'm adding in the series to fix CI and support old > rustc, i.e. quote! { ::qemu_api::c_str!(#name) } or something like that. > > > + qdev_prop: Option<syn::Path>, > > +} > > + > > +impl Parse for QdevProperty { > > + fn parse(input: ParseStream) -> Result<Self> { > > + let _: syn::Token![#] = input.parse()?; > > + let bracketed; > > + _ = syn::bracketed!(bracketed in input); > > + assert_eq!(PROPERTY, bracketed.parse::<syn::Ident>()?); > > + let mut retval = Self { > > + name: None, > > + qdev_prop: None, > > + }; > > + let content; > > + _ = syn::parenthesized!(content in bracketed); > > + while !content.is_empty() { > > + let value: syn::Ident = content.parse()?; > > + if value == NAME { > > + let _: syn::Token![=] = content.parse()?; > > + if retval.name.is_some() { > > + panic!("{} can only be used at most once", NAME); > > + } > > + retval.name = Some(content.parse()?); > > + } else if value == QDEV_PROP { > > + let _: syn::Token![=] = content.parse()?; > > + if retval.qdev_prop.is_some() { > > + panic!("{} can only be used at most once", QDEV_PROP); > > + } > > + retval.qdev_prop = Some(content.parse()?); > > + } else { > > + panic!("unrecognized token `{}`", value); > > + } > > + > > + if !content.is_empty() { > > + let _: syn::Token![,] = content.parse()?; > > + } > > + } > > + Ok(retval) > > See above with respect to the duplicated code with #[device()]. > > > + let derive_container: DeriveContainer = input > > + .attrs > > + .iter() > > + .find(|a| a.path() == DEVICE) > > + .map(|a| syn::parse(a.to_token_stream().into()).expect("could not parse device attr")) > > + .unwrap_or_default(); > > + let (qdev_properties_static, qdev_properties_expanded) = make_qdev_properties(&input); > > Please put functions before their callers. > > > + let realize_fn = format_ident!("__{}_realize_generated", name); > > + let reset_fn = format_ident!("__{}_reset_generated", name); > > + > > + let expanded = quote! { > > + unsafe impl ::qemu_api::objects::DeviceImplUnsafe for #name { > > + const REALIZE: ::core::option::Option< > > + unsafe extern "C" fn( > > + dev: *mut ::qemu_api::bindings::DeviceState, > > + errp: *mut *mut ::qemu_api::bindings::Error, > > + ), > > + > = Some(#realize_fn); > > + const RESET: ::core::option::Option< > > + unsafe extern "C" fn(dev: *mut ::qemu_api::bindings::DeviceState), > > + > = Some(#reset_fn); > > + } > > + > > + #[no_mangle] > > Not needed. > > > + pub unsafe extern "C" fn #realize_fn( > > + dev: *mut ::qemu_api::bindings::DeviceState, > > + errp: *mut *mut ::qemu_api::bindings::Error, > > + ) { > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > + unsafe { > > + ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); > > + } > > + } > > + > > + #[no_mangle] > > Not needed. > > > + pub unsafe extern "C" fn #reset_fn( > > + dev: *mut ::qemu_api::bindings::DeviceState, > > + ) { > > + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); > > + unsafe { > > + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); > > + } > > + } > > All this code depends on nothing but #name. This is not the C > preprocessor; the way to do it in Rust is monomorphization as described > above. > > > +fn gen_device_class( > > + derive_container: DeriveContainer, > > + qdev_properties_static: syn::Ident, > > + name: &syn::Ident, > > +) -> proc_macro2::TokenStream { > > + let (class_name, class_def) = match ( > > + derive_container.class_name_override, > > + derive_container.class_name, > > + ) { > > + (Some(class_name), _) => { > > + let class_expanded = quote! { > > + #[repr(C)] > > + pub struct #class_name { > > + _inner: [u8; 0], > > + } > > + }; > > + (class_name, class_expanded) > > + } > > + (None, Some(class_name)) => (class_name, quote! {}), > > + (None, None) => { > > + let class_name = format_ident!("{}Class", name); > > + let class_expanded = quote! { > > + #[repr(C)] > > + pub struct #class_name { > > + _inner: [u8; 0], > > + } > > This should have a DeviceClass member, it should not be a dummy 0-byte type. > > Also, this should be generated by #[derive(Object)]. > > > + }; > > + (class_name, class_expanded) > > + } > > + }; > > + let class_init_fn = format_ident!("__{}_class_init_generated", class_name); > > + let class_base_init_fn = format_ident!("__{}_class_base_init_generated", class_name); > > + > > + let (vmsd, vmsd_impl) = { > > + let (i, vmsd) = make_vmstate(name); > > + (quote! { &#i }, vmsd) > > + }; > > + let category = if let Some(category) = derive_container.category { > > + quote! { > > + const BITS_PER_LONG: u32 = ::core::ffi::c_ulong::BITS; > > + let _: ::qemu_api::bindings::DeviceCategory = #category; > > + let nr: ::core::ffi::c_ulong = #category as _; > > + let mask = 1 << (nr as u32 % BITS_PER_LONG); > > + let p = ::core::ptr::addr_of_mut!(dc.as_mut().categories).offset((nr as u32 / BITS_PER_LONG) as isize); > > + let p: *mut ::core::ffi::c_ulong = p.cast(); > > + let categories = p.read_unaligned(); > > + p.write_unaligned(categories | mask); > > What's wrong with > > const BITS_PER_ELEMENT: u32 = > ::core::mem::sizeof(dc.categories) / > dc.categories.len() * 8; > > dc.categories[((nr as u32) / BITS_PER_ELEMENT) as usize] > |= 1 << ((nr as u32) % BITS_PER_ELEMENT); > > ? > > > + #[no_mangle] > > + pub unsafe extern "C" fn #class_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { > > + { > > + { > > + let mut dc = > > + ::core::ptr::NonNull::new(klass.cast::<::qemu_api::bindings::DeviceClass>()).unwrap(); > > And then "let mut dc = dc.as_mut()". Just write the conversion once. > > > + unsafe { > > + dc.as_mut().realize = > > + <#name as ::qemu_api::objects::DeviceImplUnsafe>::REALIZE; > > + ::qemu_api::bindings::device_class_set_legacy_reset( > > + dc.as_mut(), > > + <#name as ::qemu_api::objects::DeviceImplUnsafe>::RESET > > + ); > > As written elsewhere, these should be conditional. > > > + dc.as_mut().vmsd = #vmsd; > > + #props > > + #category > > + }> + } > > All this code should be outside the macro, and should use trait consts > instead of quoting. > > > + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); > > + unsafe { > > + ::qemu_api::objects::ClassImpl::class_init(klass.as_mut(), data); > > + } > > + } > > + } > > + #[no_mangle] > > + pub unsafe extern "C" fn #class_base_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { > > + { > > + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); > > + unsafe { > > + ::qemu_api::objects::ClassImpl::class_base_init(klass.as_mut(), data); > > + } > > + } > > + } > > + > > + #vmsd_impl > > + } > > +} > > + > > +fn make_vmstate(name: &syn::Ident) -> (syn::Ident, proc_macro2::TokenStream) { > > Not needed. Just let the user provide a VMStateDescription in > DeviceImpl. I'm not sure if it's possible to make it a const; if not, > it can be a function returning a &'static VMStateDescription. > > > + let vmstate_description_ident = format_ident!("__VMSTATE_{}", name); > > + > > + let pre_load = format_ident!("__{}_pre_load_generated", name); > > + let post_load = format_ident!("__{}_post_load_generated", name); > > + let pre_save = format_ident!("__{}_pre_save_generated", name); > > + let post_save = format_ident!("__{}_post_save_generated", name); > > + let needed = format_ident!("__{}_needed_generated", name); > > + let dev_unplug_pending = format_ident!("__{}_dev_unplug_pending_generated", name); > > + > > + let migrateable_fish = quote! {<#name as ::qemu_api::objects::Migrateable>}; > > + let vmstate_description = quote! { > > + #[used] > > Attribute not needed. > > > + #[allow(non_upper_case_globals)] > > + pub static #vmstate_description_ident: ::qemu_api::bindings::VMStateDescription = ::qemu_api::bindings::VMStateDescription { > > + name: if let Some(name) = #migrateable_fish::NAME { > > + name.as_ptr() > > + } else { > > + <#name as ::qemu_api::objects::ObjectImplUnsafe>::TYPE_INFO.name > > + }, > > + unmigratable: #migrateable_fish::UNMIGRATABLE, > > + early_setup: #migrateable_fish::EARLY_SETUP, > > + version_id: #migrateable_fish::VERSION_ID, > > + minimum_version_id: #migrateable_fish::MINIMUM_VERSION_ID, > > + priority: #migrateable_fish::PRIORITY, > > + pre_load: Some(#pre_load), > > + post_load: Some(#post_load), > > + pre_save: Some(#pre_save), > > + post_save: Some(#post_save), > > + needed: Some(#needed), > > + dev_unplug_pending: Some(#dev_unplug_pending), > > + fields: ::core::ptr::null(), > > + subsections: ::core::ptr::null(), > > + }; > > + > > + #[no_mangle] > > Not needed (other occurrences below). > > > + pub unsafe extern "C" fn #pre_load(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::pre_load(instance.as_mut()) > > + } > > + } > > + #[no_mangle] > > + pub unsafe extern "C" fn #post_load(opaque: *mut ::core::ffi::c_void, version_id: core::ffi::c_int) -> ::core::ffi::c_int { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::post_load(instance.as_mut(), version_id) > > Again, introducing the Migrateable code and all these thunks is > premature; but in any case, this can only return 0 or -1 so make > Migrateable::post_load() return Result<(), ()>. > > > + } > > + } > > + #[no_mangle] > > + pub unsafe extern "C" fn #pre_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::pre_save(instance.as_mut()) > > + } > > + } > > Likewise. > > > + #[no_mangle] > > + pub unsafe extern "C" fn #post_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::post_save(instance.as_mut()) > > + } > > + } > > Likewise. > > > + #[no_mangle] > > + pub unsafe extern "C" fn #needed(opaque: *mut ::core::ffi::c_void) -> bool { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::needed(instance.as_mut()) > > + } > > + } > > + #[no_mangle] > > + pub unsafe extern "C" fn #dev_unplug_pending(opaque: *mut ::core::ffi::c_void) -> bool { > > + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); > > + unsafe { > > + ::qemu_api::objects::Migrateable::dev_unplug_pending(instance.as_mut()) > > + } > > + } > > + }; > > + > > + let expanded = quote! { > > + #vmstate_description > > + }; > > + (vmstate_description_ident, expanded) > > +} > > diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs > > index 59aba592d9ae4c5a4cdfdc6f9b9b08363b8a57e5..7753a853fae72fc87e6dc642cf076c6d0c736345 100644 > > --- a/rust/qemu-api-macros/src/lib.rs > > +++ b/rust/qemu-api-macros/src/lib.rs > > @@ -2,42 +2,21 @@ > > // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > // SPDX-License-Identifier: GPL-2.0-or-later > > > > +#![allow(dead_code)] > > Why? > > > use proc_macro::TokenStream; > > -use quote::{format_ident, quote}; > > -use syn::{parse_macro_input, DeriveInput}; > > + > > +mod device; > > +mod object; > > +mod symbols; > > +mod utilities; > > > > #[proc_macro_derive(Object)] > > pub fn derive_object(input: TokenStream) -> TokenStream { > > - let input = parse_macro_input!(input as DeriveInput); > > - > > - let name = input.ident; > > - let module_static = format_ident!("__{}_LOAD_MODULE", name); > > - > > - let expanded = quote! { > > - #[allow(non_upper_case_globals)] > > - #[used] > > - #[cfg_attr(target_os = "linux", link_section = ".ctors")] > > - #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] > > - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] > > - pub static #module_static: extern "C" fn() = { > > - extern "C" fn __register() { > > - unsafe { > > - ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO); > > - } > > - } > > - > > - extern "C" fn __load() { > > - unsafe { > > - ::qemu_api::bindings::register_module_init( > > - Some(__register), > > - ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM > > - ); > > - } > > - } > > - > > - __load > > - }; > > - }; > > + object::derive_object(input) > > Moving code to a separate file should be a separate patch from modifying > the expansion of the macro. > > > diff --git a/rust/qemu-api-macros/src/symbols.rs b/rust/qemu-api-macros/src/symbols.rs > > new file mode 100644 > > index 0000000000000000000000000000000000000000..f73768d228ed2b4d478c18336db56cb11e70f012 > > --- /dev/null > > +++ b/rust/qemu-api-macros/src/symbols.rs > > @@ -0,0 +1,55 @@ > > +// Copyright 2024, Linaro Limited > > +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > +// SPDX-License-Identifier: GPL-2.0-or-later > > + > > +use core::fmt; > > +use syn::{Ident, Path}; > > + > > +#[derive(Copy, Clone, Debug)] > > +pub struct Symbol(&'static str); > > + > > +pub const DEVICE: Symbol = Symbol("device"); > > +pub const NAME: Symbol = Symbol("name"); > > +pub const CATEGORY: Symbol = Symbol("category"); > > +pub const CLASS_NAME: Symbol = Symbol("class_name"); > > +pub const CLASS_NAME_OVERRIDE: Symbol = Symbol("class_name_override"); > > +pub const QDEV_PROP: Symbol = Symbol("qdev_prop"); > > +pub const MIGRATEABLE: Symbol = Symbol("migrateable"); > > +pub const PROPERTIES: Symbol = Symbol("properties"); > > +pub const PROPERTY: Symbol = Symbol("property"); > > Declare these in device.rs as needed, not here. This avoids "use > symbols::*". It also allows making them not "pub", so that dead ones > are detected by the compiler (e.g. MIGRATEABLE, PROPERTIES). > > > +pub fn assert_is_repr_c_struct(input: &DeriveInput, derive_macro: &'static str) { > > Nice but a bit overengineered. Unless you think/know that you'll have a > use for Repr elsewhere, try sharing code with Junjie's macro > https://lore.kernel.org/qemu-devel/20241025160209.194307-16-pbonzini@redhat.com/. > > Paolo >
Manos, I understand your desire to make fast progress, at the same time I can't stress enough the importance of building on a shared design. Each architectural decision we make now - whether about trait hierarchies, macro designs, or API patterns - will significantly impact future maintenance and development work. The suggestions about how to split the implementation into smaller patches aim to facilitate smaller and easier discussions about specific aspects, and they are only examples and in no way the only possible solution. However, with respect to design choices I think I pointed out major issues that can't be treated simply as wanting to "do everything at once". Dismissing design discussions as mere disagreements about style, or responding confrontationally that you will address "bits that are wrong and not much else" won't help us move forward. Code reviews, especially for core APIs, necessarily include discussion of design choices and language usage. This isn't the first time our technical discussions have become tense, and we're talking past each other at this point. My suggestion is that you try to explain your point of view to a third person, that can help you understand the patch review process and what is requested of both code contributors and reviewers. Thanks, Paolo Il dom 27 ott 2024, 23:40 Manos Pitsidianakis < manos.pitsidianakis@linaro.org> ha scritto: > Thank you for the review comments Paolo. I will address any bits I did > wrong and not much the rest, it's obvious you have a disagreement over > how things are done and that's fine. This series does not attempt to > solve everything at once and arguing again and again over "this Trait > should have been OtherTrait and this thing should have been thing!()" > is not productive. Your review style of relentless disagreement after > disagreement is tiresome and impossible to deal with; it's like a > denial of service for other human beings. I suggest you take a step > back and take a deep breath before reviewing Rust patches again. I > assure you I will make sure to address all your comments either in > code, TODO comments, or patch messages. > > In the meantime, take it easy. > >
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index c7193b41beec0b177dbc75ac0e43fcfea4c82bfb..c469877b1ca70dd1a02e3a2449c65ad3e57c93ae 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -9,7 +9,7 @@ use qemu_api::{ bindings::{self, *}, - definitions::ObjectImpl, + objects::*, }; use crate::{ @@ -26,7 +26,8 @@ pub const PL011_FIFO_DEPTH: usize = 16_usize; #[repr(C)] -#[derive(Debug, qemu_api_macros::Object)] +#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] +#[device(class_name_override = PL011Class)] /// PL011 Device Model in QEMU pub struct PL011State { pub parent_obj: SysBusDevice, @@ -51,6 +52,7 @@ pub struct PL011State { pub read_count: usize, pub read_trigger: usize, #[doc(alias = "chr")] + #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] pub char_backend: CharBackend, /// QEMU interrupts /// @@ -68,38 +70,17 @@ pub struct PL011State { #[doc(alias = "clk")] pub clock: NonNull<Clock>, #[doc(alias = "migrate_clk")] + #[property(name = c"migrate-clk", qdev_prop = qdev_prop_bool)] pub migrate_clock: bool, } impl ObjectImpl for PL011State { type Class = PL011Class; - const TYPE_INFO: qemu_api::bindings::TypeInfo = qemu_api::type_info! { Self }; + const TYPE_NAME: &'static CStr = crate::TYPE_PL011; const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE); const ABSTRACT: bool = false; - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_init); - const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None; - const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)> = None; -} -#[repr(C)] -pub struct PL011Class { - _inner: [u8; 0], -} - -impl qemu_api::definitions::Class for PL011Class { - const CLASS_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void), - > = Some(crate::device_class::pl011_class_init); - const CLASS_BASE_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void), - > = None; -} - -#[used] -pub static CLK_NAME: &CStr = c"clk"; - -impl PL011State { /// Initializes a pre-allocated, unitialized instance of `PL011State`. /// /// # Safety @@ -108,7 +89,7 @@ impl PL011State { /// `PL011State` type. It must not be called more than once on the same /// location/instance. All its fields are expected to hold unitialized /// values with the sole exception of `parent_obj`. - pub unsafe fn init(&mut self) { + unsafe fn instance_init(&mut self) { let dev = addr_of_mut!(*self).cast::<DeviceState>(); // SAFETY: // @@ -120,7 +101,7 @@ pub unsafe fn init(&mut self) { addr_of_mut!(*self).cast::<Object>(), &PL011_OPS, addr_of_mut!(*self).cast::<c_void>(), - Self::TYPE_INFO.name, + Self::TYPE_NAME.as_ptr(), 0x1000, ); let sbd = addr_of_mut!(*self).cast::<SysBusDevice>(); @@ -147,7 +128,49 @@ pub unsafe fn init(&mut self) { .unwrap(); } } +} +impl DeviceImpl for PL011State { + fn realize(&mut self) { + // SAFETY: self.char_backend has the correct size and alignment for a + // CharBackend object, and its callbacks are of the correct types. + unsafe { + qemu_chr_fe_set_handlers( + addr_of_mut!(self.char_backend), + Some(pl011_can_receive), + Some(pl011_receive), + Some(pl011_event), + None, + addr_of_mut!(*self).cast::<c_void>(), + core::ptr::null_mut(), + true, + ); + } + } + + fn reset(&mut self) { + self.line_control.reset(); + self.receive_status_error_clear.reset(); + self.dmacr = 0; + self.int_enabled = 0; + self.int_level = 0; + self.ilpr = 0; + self.ibrd = 0; + self.fbrd = 0; + self.read_trigger = 1; + self.ifl = 0x12; + self.control.reset(); + self.flags = 0.into(); + self.reset_fifo(); + } +} + +impl qemu_api::objects::Migrateable for PL011State {} + +#[used] +pub static CLK_NAME: &CStr = c"clk"; + +impl PL011State { pub fn read( &mut self, offset: hwaddr, @@ -394,39 +417,6 @@ fn set_read_trigger(&mut self) { self.read_trigger = 1; } - pub fn realize(&mut self) { - // SAFETY: self.char_backend has the correct size and alignment for a - // CharBackend object, and its callbacks are of the correct types. - unsafe { - qemu_chr_fe_set_handlers( - addr_of_mut!(self.char_backend), - Some(pl011_can_receive), - Some(pl011_receive), - Some(pl011_event), - None, - addr_of_mut!(*self).cast::<c_void>(), - core::ptr::null_mut(), - true, - ); - } - } - - pub fn reset(&mut self) { - self.line_control.reset(); - self.receive_status_error_clear.reset(); - self.dmacr = 0; - self.int_enabled = 0; - self.int_level = 0; - self.ilpr = 0; - self.ibrd = 0; - self.fbrd = 0; - self.read_trigger = 1; - self.ifl = 0x12; - self.control.reset(); - self.flags = 0.into(); - self.reset_fifo(); - } - pub fn reset_fifo(&mut self) { self.read_count = 0; self.read_pos = 0; @@ -583,17 +573,3 @@ pub fn update(&self) { dev } } - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -#[no_mangle] -pub unsafe extern "C" fn pl011_init(obj: *mut Object) { - unsafe { - debug_assert!(!obj.is_null()); - let mut state = NonNull::new_unchecked(obj.cast::<PL011State>()); - state.as_mut().init(); - } -} diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs deleted file mode 100644 index b7ab31af02d7bb50ae94be0b153baafc7ccfa375..0000000000000000000000000000000000000000 --- a/rust/hw/char/pl011/src/device_class.rs +++ /dev/null @@ -1,70 +0,0 @@ -// Copyright 2024, Linaro Limited -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> -// SPDX-License-Identifier: GPL-2.0-or-later - -use core::ptr::NonNull; - -use qemu_api::{bindings::*, definitions::ObjectImpl}; - -use crate::device::PL011State; - -#[used] -pub static VMSTATE_PL011: VMStateDescription = VMStateDescription { - name: PL011State::TYPE_INFO.name, - unmigratable: true, - ..unsafe { ::core::mem::MaybeUninit::<VMStateDescription>::zeroed().assume_init() } -}; - -qemu_api::declare_properties! { - PL011_PROPERTIES, - qemu_api::define_property!( - c"chardev", - PL011State, - char_backend, - unsafe { &qdev_prop_chr }, - CharBackend - ), - qemu_api::define_property!( - c"migrate-clk", - PL011State, - migrate_clock, - unsafe { &qdev_prop_bool }, - bool - ), -} - -qemu_api::device_class_init! { - pl011_class_init, - props => PL011_PROPERTIES, - realize_fn => Some(pl011_realize), - legacy_reset_fn => Some(pl011_reset), - vmsd => VMSTATE_PL011, -} - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -#[no_mangle] -pub unsafe extern "C" fn pl011_realize(dev: *mut DeviceState, _errp: *mut *mut Error) { - unsafe { - assert!(!dev.is_null()); - let mut state = NonNull::new_unchecked(dev.cast::<PL011State>()); - state.as_mut().realize(); - } -} - -/// # Safety -/// -/// We expect the FFI user of this function to pass a valid pointer, that has -/// the same size as [`PL011State`]. We also expect the device is -/// readable/writeable from one thread at any time. -#[no_mangle] -pub unsafe extern "C" fn pl011_reset(dev: *mut DeviceState) { - unsafe { - assert!(!dev.is_null()); - let mut state = NonNull::new_unchecked(dev.cast::<PL011State>()); - state.as_mut().reset(); - } -} diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs index 2939ee50c99ceaacf6ec68127272d58814e33679..f4d9cce4b01f605cfcbec7ea87c8b2009d77ee52 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -42,7 +42,6 @@ extern crate qemu_api; pub mod device; -pub mod device_class; pub mod memory_ops; pub const TYPE_PL011: &::core::ffi::CStr = c"pl011"; diff --git a/rust/qemu-api-macros/src/device.rs b/rust/qemu-api-macros/src/device.rs new file mode 100644 index 0000000000000000000000000000000000000000..3b965576a065601cd5c97d5ab6a2501f96d16a61 --- /dev/null +++ b/rust/qemu-api-macros/src/device.rs @@ -0,0 +1,433 @@ +// Copyright 2024, Linaro Limited +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> +// SPDX-License-Identifier: GPL-2.0-or-later + +use proc_macro::TokenStream; +use quote::{format_ident, quote, ToTokens}; +use syn::{ + parse::{Parse, ParseStream}, + Result, +}; +use syn::{parse_macro_input, DeriveInput}; + +use crate::{symbols::*, utilities::*}; + +#[derive(Debug, Default)] +struct DeriveContainer { + category: Option<syn::Path>, + class_name: Option<syn::Ident>, + class_name_override: Option<syn::Ident>, +} + +impl Parse for DeriveContainer { + fn parse(input: ParseStream) -> Result<Self> { + let _: syn::Token![#] = input.parse()?; + let bracketed; + _ = syn::bracketed!(bracketed in input); + assert_eq!(DEVICE, bracketed.parse::<syn::Ident>()?); + let mut retval = Self { + category: None, + class_name: None, + class_name_override: None, + }; + let content; + _ = syn::parenthesized!(content in bracketed); + while !content.is_empty() { + let value: syn::Ident = content.parse()?; + if value == CLASS_NAME { + let _: syn::Token![=] = content.parse()?; + if retval.class_name.is_some() { + panic!("{} can only be used at most once", CLASS_NAME); + } + retval.class_name = Some(content.parse()?); + } else if value == CLASS_NAME_OVERRIDE { + let _: syn::Token![=] = content.parse()?; + if retval.class_name_override.is_some() { + panic!("{} can only be used at most once", CLASS_NAME_OVERRIDE); + } + retval.class_name_override = Some(content.parse()?); + } else if value == CATEGORY { + let _: syn::Token![=] = content.parse()?; + if retval.category.is_some() { + panic!("{} can only be used at most once", CATEGORY); + } + let lit: syn::LitStr = content.parse()?; + let path: syn::Path = lit.parse()?; + retval.category = Some(path); + } else { + panic!("unrecognized token `{}`", value); + } + + if !content.is_empty() { + let _: syn::Token![,] = content.parse()?; + } + } + if retval.class_name.is_some() && retval.class_name_override.is_some() { + panic!( + "Cannot define `{}` and `{}` at the same time", + CLASS_NAME, CLASS_NAME_OVERRIDE + ); + } + Ok(retval) + } +} + +#[derive(Debug)] +struct QdevProperty { + name: Option<syn::LitCStr>, + qdev_prop: Option<syn::Path>, +} + +impl Parse for QdevProperty { + fn parse(input: ParseStream) -> Result<Self> { + let _: syn::Token![#] = input.parse()?; + let bracketed; + _ = syn::bracketed!(bracketed in input); + assert_eq!(PROPERTY, bracketed.parse::<syn::Ident>()?); + let mut retval = Self { + name: None, + qdev_prop: None, + }; + let content; + _ = syn::parenthesized!(content in bracketed); + while !content.is_empty() { + let value: syn::Ident = content.parse()?; + if value == NAME { + let _: syn::Token![=] = content.parse()?; + if retval.name.is_some() { + panic!("{} can only be used at most once", NAME); + } + retval.name = Some(content.parse()?); + } else if value == QDEV_PROP { + let _: syn::Token![=] = content.parse()?; + if retval.qdev_prop.is_some() { + panic!("{} can only be used at most once", QDEV_PROP); + } + retval.qdev_prop = Some(content.parse()?); + } else { + panic!("unrecognized token `{}`", value); + } + + if !content.is_empty() { + let _: syn::Token![,] = content.parse()?; + } + } + Ok(retval) + } +} + +pub fn derive_device(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + + assert_is_repr_c_struct(&input, "Device"); + + let derive_container: DeriveContainer = input + .attrs + .iter() + .find(|a| a.path() == DEVICE) + .map(|a| syn::parse(a.to_token_stream().into()).expect("could not parse device attr")) + .unwrap_or_default(); + let (qdev_properties_static, qdev_properties_expanded) = make_qdev_properties(&input); + let class_expanded = gen_device_class(derive_container, qdev_properties_static, &input.ident); + let name = input.ident; + + let realize_fn = format_ident!("__{}_realize_generated", name); + let reset_fn = format_ident!("__{}_reset_generated", name); + + let expanded = quote! { + unsafe impl ::qemu_api::objects::DeviceImplUnsafe for #name { + const REALIZE: ::core::option::Option< + unsafe extern "C" fn( + dev: *mut ::qemu_api::bindings::DeviceState, + errp: *mut *mut ::qemu_api::bindings::Error, + ), + > = Some(#realize_fn); + const RESET: ::core::option::Option< + unsafe extern "C" fn(dev: *mut ::qemu_api::bindings::DeviceState), + > = Some(#reset_fn); + } + + #[no_mangle] + pub unsafe extern "C" fn #realize_fn( + dev: *mut ::qemu_api::bindings::DeviceState, + errp: *mut *mut ::qemu_api::bindings::Error, + ) { + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); + unsafe { + ::qemu_api::objects::DeviceImpl::realize(instance.as_mut()); + } + } + + #[no_mangle] + pub unsafe extern "C" fn #reset_fn( + dev: *mut ::qemu_api::bindings::DeviceState, + ) { + let mut instance = NonNull::new(dev.cast::<#name>()).expect(concat!("Expected dev to be a non-null pointer of type ", stringify!(#name))); + unsafe { + ::qemu_api::objects::DeviceImpl::reset(instance.as_mut()); + } + } + + #qdev_properties_expanded + #class_expanded + }; + + TokenStream::from(expanded) +} + +fn make_qdev_properties(input: &DeriveInput) -> (syn::Ident, proc_macro2::TokenStream) { + let name = &input.ident; + + let qdev_properties: Vec<(syn::Field, QdevProperty)> = match &input.data { + syn::Data::Struct(syn::DataStruct { + fields: syn::Fields::Named(fields), + .. + }) => fields + .named + .iter() + .map(|f| { + f.attrs + .iter() + .filter(|a| a.path() == PROPERTY) + .map(|a| (f.clone(), a.clone())) + }) + .flatten() + .map(|(f, a)| { + ( + f.clone(), + syn::parse(a.to_token_stream().into()).expect("could not parse property attr"), + ) + }) + .collect::<Vec<(syn::Field, QdevProperty)>>(), + _other => unreachable!(), + }; + + let mut properties_expanded = quote! { + unsafe { ::core::mem::MaybeUninit::<::qemu_api::bindings::Property>::zeroed().assume_init() } + }; + let prop_len = qdev_properties.len() + 1; + for (field, prop) in qdev_properties { + 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 prop = quote! { + ::qemu_api::bindings::Property { + name: ::core::ffi::CStr::as_ptr(#prop_name), + info: unsafe { &#qdev_prop }, + offset: ::core::mem::offset_of!(#name, #field_name) as _, + bitnr: 0, + bitmask: 0, + set_default: false, + defval: ::qemu_api::bindings::Property__bindgen_ty_1 { i: 0 }, + arrayoffset: 0, + arrayinfo: ::core::ptr::null(), + arrayfieldsize: 0, + link_type: ::core::ptr::null(), + } + }; + properties_expanded = quote! { + #prop, + #properties_expanded + }; + } + let properties_ident = format_ident!("__{}_QDEV_PROPERTIES", name); + let expanded = quote! { + #[no_mangle] + pub static mut #properties_ident: [::qemu_api::bindings::Property; #prop_len] = [#properties_expanded]; + }; + (properties_ident, expanded) +} + +fn gen_device_class( + derive_container: DeriveContainer, + qdev_properties_static: syn::Ident, + name: &syn::Ident, +) -> proc_macro2::TokenStream { + let (class_name, class_def) = match ( + derive_container.class_name_override, + derive_container.class_name, + ) { + (Some(class_name), _) => { + let class_expanded = quote! { + #[repr(C)] + pub struct #class_name { + _inner: [u8; 0], + } + }; + (class_name, class_expanded) + } + (None, Some(class_name)) => (class_name, quote! {}), + (None, None) => { + let class_name = format_ident!("{}Class", name); + let class_expanded = quote! { + #[repr(C)] + pub struct #class_name { + _inner: [u8; 0], + } + }; + (class_name, class_expanded) + } + }; + let class_init_fn = format_ident!("__{}_class_init_generated", class_name); + let class_base_init_fn = format_ident!("__{}_class_base_init_generated", class_name); + + let (vmsd, vmsd_impl) = { + let (i, vmsd) = make_vmstate(name); + (quote! { &#i }, vmsd) + }; + let category = if let Some(category) = derive_container.category { + quote! { + const BITS_PER_LONG: u32 = ::core::ffi::c_ulong::BITS; + let _: ::qemu_api::bindings::DeviceCategory = #category; + let nr: ::core::ffi::c_ulong = #category as _; + let mask = 1 << (nr as u32 % BITS_PER_LONG); + let p = ::core::ptr::addr_of_mut!(dc.as_mut().categories).offset((nr as u32 / BITS_PER_LONG) as isize); + let p: *mut ::core::ffi::c_ulong = p.cast(); + let categories = p.read_unaligned(); + p.write_unaligned(categories | mask); + } + } else { + quote! {} + }; + let props = quote! { + ::qemu_api::bindings::device_class_set_props(dc.as_mut(), #qdev_properties_static.as_mut_ptr()); + }; + + quote! { + #class_def + + impl ::qemu_api::objects::ClassImpl for #class_name { + type Object = #name; + } + + unsafe impl ::qemu_api::objects::ClassImplUnsafe for #class_name { + const CLASS_INIT: Option< + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void), + > = Some(#class_init_fn); + const CLASS_BASE_INIT: Option< + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void), + > = Some(#class_base_init_fn); + } + + #[no_mangle] + pub unsafe extern "C" fn #class_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { + { + { + let mut dc = + ::core::ptr::NonNull::new(klass.cast::<::qemu_api::bindings::DeviceClass>()).unwrap(); + unsafe { + dc.as_mut().realize = + <#name as ::qemu_api::objects::DeviceImplUnsafe>::REALIZE; + ::qemu_api::bindings::device_class_set_legacy_reset( + dc.as_mut(), + <#name as ::qemu_api::objects::DeviceImplUnsafe>::RESET + ); + dc.as_mut().vmsd = #vmsd; + #props + #category + } + } + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); + unsafe { + ::qemu_api::objects::ClassImpl::class_init(klass.as_mut(), data); + } + } + } + #[no_mangle] + pub unsafe extern "C" fn #class_base_init_fn(klass: *mut ObjectClass, data: *mut core::ffi::c_void) { + { + let mut klass = NonNull::new(klass.cast::<#class_name>()).expect(concat!("Expected klass to be a non-null pointer of type ", stringify!(#class_name))); + unsafe { + ::qemu_api::objects::ClassImpl::class_base_init(klass.as_mut(), data); + } + } + } + + #vmsd_impl + } +} + +fn make_vmstate(name: &syn::Ident) -> (syn::Ident, proc_macro2::TokenStream) { + let vmstate_description_ident = format_ident!("__VMSTATE_{}", name); + + let pre_load = format_ident!("__{}_pre_load_generated", name); + let post_load = format_ident!("__{}_post_load_generated", name); + let pre_save = format_ident!("__{}_pre_save_generated", name); + let post_save = format_ident!("__{}_post_save_generated", name); + let needed = format_ident!("__{}_needed_generated", name); + let dev_unplug_pending = format_ident!("__{}_dev_unplug_pending_generated", name); + + let migrateable_fish = quote! {<#name as ::qemu_api::objects::Migrateable>}; + let vmstate_description = quote! { + #[used] + #[allow(non_upper_case_globals)] + pub static #vmstate_description_ident: ::qemu_api::bindings::VMStateDescription = ::qemu_api::bindings::VMStateDescription { + name: if let Some(name) = #migrateable_fish::NAME { + name.as_ptr() + } else { + <#name as ::qemu_api::objects::ObjectImplUnsafe>::TYPE_INFO.name + }, + unmigratable: #migrateable_fish::UNMIGRATABLE, + early_setup: #migrateable_fish::EARLY_SETUP, + version_id: #migrateable_fish::VERSION_ID, + minimum_version_id: #migrateable_fish::MINIMUM_VERSION_ID, + priority: #migrateable_fish::PRIORITY, + pre_load: Some(#pre_load), + post_load: Some(#post_load), + pre_save: Some(#pre_save), + post_save: Some(#post_save), + needed: Some(#needed), + dev_unplug_pending: Some(#dev_unplug_pending), + fields: ::core::ptr::null(), + subsections: ::core::ptr::null(), + }; + + #[no_mangle] + pub unsafe extern "C" fn #pre_load(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::pre_load(instance.as_mut()) + } + } + #[no_mangle] + pub unsafe extern "C" fn #post_load(opaque: *mut ::core::ffi::c_void, version_id: core::ffi::c_int) -> ::core::ffi::c_int { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::post_load(instance.as_mut(), version_id) + } + } + #[no_mangle] + pub unsafe extern "C" fn #pre_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::pre_save(instance.as_mut()) + } + } + #[no_mangle] + pub unsafe extern "C" fn #post_save(opaque: *mut ::core::ffi::c_void) -> ::core::ffi::c_int { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::post_save(instance.as_mut()) + } + } + #[no_mangle] + pub unsafe extern "C" fn #needed(opaque: *mut ::core::ffi::c_void) -> bool { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::needed(instance.as_mut()) + } + } + #[no_mangle] + pub unsafe extern "C" fn #dev_unplug_pending(opaque: *mut ::core::ffi::c_void) -> bool { + let mut instance = NonNull::new(opaque.cast::<#name>()).expect(concat!("Expected opaque to be a non-null pointer of type ", stringify!(#name), "::Object")); + unsafe { + ::qemu_api::objects::Migrateable::dev_unplug_pending(instance.as_mut()) + } + } + }; + + let expanded = quote! { + #vmstate_description + }; + (vmstate_description_ident, expanded) +} diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs index 59aba592d9ae4c5a4cdfdc6f9b9b08363b8a57e5..7753a853fae72fc87e6dc642cf076c6d0c736345 100644 --- a/rust/qemu-api-macros/src/lib.rs +++ b/rust/qemu-api-macros/src/lib.rs @@ -2,42 +2,21 @@ // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> // SPDX-License-Identifier: GPL-2.0-or-later +#![allow(dead_code)] + use proc_macro::TokenStream; -use quote::{format_ident, quote}; -use syn::{parse_macro_input, DeriveInput}; + +mod device; +mod object; +mod symbols; +mod utilities; #[proc_macro_derive(Object)] pub fn derive_object(input: TokenStream) -> TokenStream { - let input = parse_macro_input!(input as DeriveInput); - - let name = input.ident; - let module_static = format_ident!("__{}_LOAD_MODULE", name); - - let expanded = quote! { - #[allow(non_upper_case_globals)] - #[used] - #[cfg_attr(target_os = "linux", link_section = ".ctors")] - #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] - pub static #module_static: extern "C" fn() = { - extern "C" fn __register() { - unsafe { - ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::definitions::ObjectImpl>::TYPE_INFO); - } - } - - extern "C" fn __load() { - unsafe { - ::qemu_api::bindings::register_module_init( - Some(__register), - ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM - ); - } - } - - __load - }; - }; + object::derive_object(input) +} - TokenStream::from(expanded) +#[proc_macro_derive(Device, attributes(device, property))] +pub fn derive_device(input: TokenStream) -> TokenStream { + device::derive_device(input) } diff --git a/rust/qemu-api-macros/src/object.rs b/rust/qemu-api-macros/src/object.rs new file mode 100644 index 0000000000000000000000000000000000000000..f808069aea42de752dea7524fef64467427f105c --- /dev/null +++ b/rust/qemu-api-macros/src/object.rs @@ -0,0 +1,107 @@ +// Copyright 2024, Linaro Limited +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> +// SPDX-License-Identifier: GPL-2.0-or-later + +use proc_macro::TokenStream; +use quote::{format_ident, quote}; +use syn::{parse_macro_input, DeriveInput}; + +use crate::utilities::*; + +pub fn derive_object(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + + assert_is_repr_c_struct(&input, "Object"); + + let name = input.ident; + let module_static = format_ident!("__{}_LOAD_MODULE", name); + + let ctors = quote! { + #[allow(non_upper_case_globals)] + #[used] + #[cfg_attr(target_os = "linux", link_section = ".ctors")] + #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] + #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] + pub static #module_static: extern "C" fn() = { + extern "C" fn __register() { + unsafe { + ::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::objects::ObjectImplUnsafe>::TYPE_INFO); + } + } + + extern "C" fn __load() { + unsafe { + ::qemu_api::bindings::register_module_init( + Some(__register), + ::qemu_api::bindings::module_init_type::MODULE_INIT_QOM + ); + } + } + + __load + }; + }; + + let instance_init = format_ident!("__{}_instance_init_generated", name); + let instance_post_init = format_ident!("__{}_instance_post_init_generated", name); + let instance_finalize = format_ident!("__{}_instance_finalize_generated", name); + + let obj_impl_unsafe = quote! { + unsafe impl ::qemu_api::objects::ObjectImplUnsafe for #name { + const TYPE_INFO: ::qemu_api::bindings::TypeInfo = + ::qemu_api::bindings::TypeInfo { + name: <Self as ::qemu_api::objects::ObjectImpl>::TYPE_NAME.as_ptr(), + parent: if let Some(pname) = <Self as ::qemu_api::objects::ObjectImpl>::PARENT_TYPE_NAME { + pname.as_ptr() + } else { + ::core::ptr::null() + }, + instance_size: ::core::mem::size_of::<Self>() as ::qemu_api::bindings::size_t, + instance_align: ::core::mem::align_of::<Self>() as ::qemu_api::bindings::size_t, + instance_init: <Self as ::qemu_api::objects::ObjectImplUnsafe>::INSTANCE_INIT, + instance_post_init: <Self as ::qemu_api::objects::ObjectImplUnsafe>::INSTANCE_POST_INIT, + instance_finalize: <Self as ::qemu_api::objects::ObjectImplUnsafe>::INSTANCE_FINALIZE, + abstract_: <Self as ::qemu_api::objects::ObjectImpl>::ABSTRACT, + class_size: ::core::mem::size_of::<<Self as ::qemu_api::objects::ObjectImpl>::Class>() as ::qemu_api::bindings::size_t, + class_init: <<Self as ::qemu_api::objects::ObjectImpl>::Class as ::qemu_api::objects::ClassImplUnsafe>::CLASS_INIT, + class_base_init: <<Self as ::qemu_api::objects::ObjectImpl>::Class as ::qemu_api::objects::ClassImplUnsafe>::CLASS_BASE_INIT, + class_data: ::core::ptr::null_mut(), + interfaces: ::core::ptr::null_mut(), + }; + const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut ::qemu_api::bindings::Object)> = Some(#instance_init); + const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut ::qemu_api::bindings::Object)> = Some(#instance_post_init); + const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut ::qemu_api::bindings::Object)> = Some(#instance_finalize); + } + + #[no_mangle] + pub unsafe extern "C" fn #instance_init(obj: *mut ::qemu_api::bindings::Object) { + let mut instance = NonNull::new(obj.cast::<#name>()).expect(concat!("Expected obj to be a non-null pointer of type ", stringify!(#name))); + unsafe { + ::qemu_api::objects::ObjectImpl::instance_init(instance.as_mut()); + } + } + + #[no_mangle] + pub unsafe extern "C" fn #instance_post_init(obj: *mut ::qemu_api::bindings::Object) { + let mut instance = NonNull::new(obj.cast::<#name>()).expect(concat!("Expected obj to be a non-null pointer of type ", stringify!(#name))); + unsafe { + ::qemu_api::objects::ObjectImpl::instance_post_init(instance.as_mut()); + } + } + + #[no_mangle] + pub unsafe extern "C" fn #instance_finalize(obj: *mut ::qemu_api::bindings::Object) { + let mut instance = NonNull::new(obj.cast::<#name>()).expect(concat!("Expected obj to be a non-null pointer of type ", stringify!(#name))); + unsafe { + ::qemu_api::objects::ObjectImpl::instance_finalize(instance.as_mut()); + } + } + }; + + let expanded = quote! { + #obj_impl_unsafe + + #ctors + }; + TokenStream::from(expanded) +} diff --git a/rust/qemu-api-macros/src/symbols.rs b/rust/qemu-api-macros/src/symbols.rs new file mode 100644 index 0000000000000000000000000000000000000000..f73768d228ed2b4d478c18336db56cb11e70f012 --- /dev/null +++ b/rust/qemu-api-macros/src/symbols.rs @@ -0,0 +1,55 @@ +// Copyright 2024, Linaro Limited +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> +// SPDX-License-Identifier: GPL-2.0-or-later + +use core::fmt; +use syn::{Ident, Path}; + +#[derive(Copy, Clone, Debug)] +pub struct Symbol(&'static str); + +pub const DEVICE: Symbol = Symbol("device"); +pub const NAME: Symbol = Symbol("name"); +pub const CATEGORY: Symbol = Symbol("category"); +pub const CLASS_NAME: Symbol = Symbol("class_name"); +pub const CLASS_NAME_OVERRIDE: Symbol = Symbol("class_name_override"); +pub const QDEV_PROP: Symbol = Symbol("qdev_prop"); +pub const MIGRATEABLE: Symbol = Symbol("migrateable"); +pub const PROPERTIES: Symbol = Symbol("properties"); +pub const PROPERTY: Symbol = Symbol("property"); + +impl PartialEq<Symbol> for Ident { + fn eq(&self, word: &Symbol) -> bool { + self == word.0 + } +} + +impl<'a> PartialEq<Symbol> for &'a Ident { + fn eq(&self, word: &Symbol) -> bool { + *self == word.0 + } +} + +impl PartialEq<Symbol> for Path { + fn eq(&self, word: &Symbol) -> bool { + self.is_ident(word.0) + } +} + +impl<'a> PartialEq<Symbol> for &'a Path { + fn eq(&self, word: &Symbol) -> bool { + self.is_ident(word.0) + } +} + +impl PartialEq<Ident> for Symbol { + fn eq(&self, ident: &Ident) -> bool { + ident == self.0 + } +} + +impl fmt::Display for Symbol { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + self.0.fmt(fmt) + } +} diff --git a/rust/qemu-api-macros/src/utilities.rs b/rust/qemu-api-macros/src/utilities.rs new file mode 100644 index 0000000000000000000000000000000000000000..bd8776539aa0bb3bcaa023bd88d962efe1431746 --- /dev/null +++ b/rust/qemu-api-macros/src/utilities.rs @@ -0,0 +1,152 @@ +// Copyright 2024, Linaro Limited +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> +// SPDX-License-Identifier: GPL-2.0-or-later + +use syn::{parenthesized, token, Data, DeriveInput, LitInt}; + +#[derive(Default)] +pub enum Abi { + #[default] + Rust, + C, + Transparent, + Other(String), +} + +#[derive(Default)] +pub struct Repr { + pub abi: Abi, + /// whether the attribute was declared in the definition. + pub present: bool, + pub align: Option<usize>, + pub packed: Option<usize>, +} + +impl core::fmt::Display for Repr { + fn fmt(&self, fmt: &mut core::fmt::Formatter) -> core::fmt::Result { + write!(fmt, "repr(")?; + match &self.abi { + Abi::C => write!(fmt, "C")?, + Abi::Rust => write!(fmt, "Rust")?, + Abi::Transparent => write!(fmt, "transparent")?, + Abi::Other(s) => write!(fmt, "{}", s)?, + } + if self.align.is_some() || self.packed.is_some() { + write!(fmt, ", ")?; + if let Some(v) = self.align { + write!(fmt, "align({})", v)?; + if self.packed.is_some() { + write!(fmt, ", ")?; + } + } + match self.packed { + Some(1) => write!(fmt, "packed")?, + Some(n) => write!(fmt, "packed({})", n)?, + None => {} + } + } + write!(fmt, ")") + } +} + +impl Repr { + pub fn detect_repr(attrs: &[syn::Attribute]) -> Self { + let mut repr = Self::default(); + + // We don't validate the repr attribute; if it's invalid rustc will complain + // anyway. + for attr in attrs { + if attr.path().is_ident("repr") { + repr.present = true; + if let Err(err) = attr.parse_nested_meta(|meta| { + // #[repr(C)] + if meta.path.is_ident("C") { + repr.abi = Abi::C; + return Ok(()); + } + + // #[repr(Rust)] + if meta.path.is_ident("Rust") { + repr.abi = Abi::Rust; + return Ok(()); + } + + // #[repr(transparent)] + if meta.path.is_ident("transparent") { + repr.abi = Abi::Transparent; + return Ok(()); + } + + // #[repr(align(N))] + if meta.path.is_ident("align") { + let content; + parenthesized!(content in meta.input); + let lit: LitInt = content.parse()?; + let n: usize = lit.base10_parse()?; + repr.align = Some(n); + return Ok(()); + } + + // #[repr(packed)] or #[repr(packed(N))], omitted N means 1 + if meta.path.is_ident("packed") { + repr.packed = if meta.input.peek(token::Paren) { + let content; + parenthesized!(content in meta.input); + let lit: LitInt = content.parse()?; + let n: usize = lit.base10_parse()?; + Some(n) + } else { + Some(1) + }; + return Ok(()); + } + + if let Some(i) = meta.path.get_ident() { + repr.abi = Abi::Other(i.to_string()); + } + + Err(meta.error("unrecognized repr")) + }) { + println!("Error while processing Object Derive macro: {}", err); + } + } + } + repr + } +} + +pub fn assert_is_repr_c_struct(input: &DeriveInput, derive_macro: &'static str) { + if !matches!(input.data, Data::Struct(_)) { + panic!( + "`{}` derive macro can only be used with structs, and `{}` is {}", + derive_macro, + input.ident, + match input.data { + Data::Struct(_) => unreachable!(), + Data::Enum(_) => "enum", + Data::Union(_) => "union", + } + ); + } + match Repr::detect_repr(&input.attrs) { + Repr { abi: Abi::C, .. } => { /* all good */ } + Repr { + abi: Abi::Transparent, + .. + } => { + // If the data layout is `transparent`, then its representation + // depends on the ABI of the wrapped type. We cannot + // detect it here. + } + other => { + panic!( + "`{}` derive macro can only be used with repr(C) structs, and `{}` {} \ + {}\nHint: Annotate the struct with `#[repr(C)]`.", + derive_macro, + input.ident, + if other.present { "is" } else { "defaults to" }, + other, + ); + } + } +} diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index c72d34b607df1da90185046f4d9c26b3cb6c6523..0bd70b59afcc005251135802897954789b068e6e 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -3,8 +3,7 @@ _qemu_api_rs = static_library( structured_sources( [ 'src/lib.rs', - 'src/definitions.rs', - 'src/device_class.rs', + 'src/objects.rs', ], {'.' : bindings_rs}, ), diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs deleted file mode 100644 index 60bd3f8aaa65ae131a9c4628a96ac52f590d7324..0000000000000000000000000000000000000000 --- a/rust/qemu-api/src/definitions.rs +++ /dev/null @@ -1,97 +0,0 @@ -// Copyright 2024, Linaro Limited -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> -// SPDX-License-Identifier: GPL-2.0-or-later - -//! Definitions required by QEMU when registering a device. - -use ::core::ffi::{c_void, CStr}; - -use crate::bindings::{Object, ObjectClass, TypeInfo}; - -/// Trait a type must implement to be registered with QEMU. -pub trait ObjectImpl { - type Class; - const TYPE_INFO: TypeInfo; - const TYPE_NAME: &'static CStr; - const PARENT_TYPE_NAME: Option<&'static CStr>; - const ABSTRACT: bool; - const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; - const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; - const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)>; -} - -pub trait Class { - const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)>; - const CLASS_BASE_INIT: Option< - unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), - >; -} - -#[macro_export] -macro_rules! module_init { - ($func:expr, $type:expr) => { - #[used] - #[cfg_attr(target_os = "linux", link_section = ".ctors")] - #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] - pub static LOAD_MODULE: extern "C" fn() = { - extern "C" fn __load() { - unsafe { - $crate::bindings::register_module_init(Some($func), $type); - } - } - - __load - }; - }; - (qom: $func:ident => $body:block) => { - // NOTE: To have custom identifiers for the ctor func we need to either supply - // them directly as a macro argument or create them with a proc macro. - #[used] - #[cfg_attr(target_os = "linux", link_section = ".ctors")] - #[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")] - #[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")] - pub static LOAD_MODULE: extern "C" fn() = { - extern "C" fn __load() { - #[no_mangle] - unsafe extern "C" fn $func() { - $body - } - - unsafe { - $crate::bindings::register_module_init( - Some($func), - $crate::bindings::module_init_type::MODULE_INIT_QOM, - ); - } - } - - __load - }; - }; -} - -#[macro_export] -macro_rules! type_info { - ($t:ty) => { - $crate::bindings::TypeInfo { - name: <$t as $crate::definitions::ObjectImpl>::TYPE_NAME.as_ptr(), - parent: if let Some(pname) = <$t as $crate::definitions::ObjectImpl>::PARENT_TYPE_NAME { - pname.as_ptr() - } else { - ::core::ptr::null_mut() - }, - instance_size: ::core::mem::size_of::<$t>() as $crate::bindings::size_t, - instance_align: ::core::mem::align_of::<$t>() as $crate::bindings::size_t, - instance_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_INIT, - instance_post_init: <$t as $crate::definitions::ObjectImpl>::INSTANCE_POST_INIT, - instance_finalize: <$t as $crate::definitions::ObjectImpl>::INSTANCE_FINALIZE, - abstract_: <$t as $crate::definitions::ObjectImpl>::ABSTRACT, - class_size: ::core::mem::size_of::<<$t as $crate::definitions::ObjectImpl>::Class>() as $crate::bindings::size_t, - class_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_INIT, - class_base_init: <<$t as $crate::definitions::ObjectImpl>::Class as $crate::definitions::Class>::CLASS_BASE_INIT, - class_data: ::core::ptr::null_mut(), - interfaces: ::core::ptr::null_mut(), - }; - } -} diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs deleted file mode 100644 index 1ea95beb78dbf8637d9af1edb668d51411a9ac33..0000000000000000000000000000000000000000 --- a/rust/qemu-api/src/device_class.rs +++ /dev/null @@ -1,128 +0,0 @@ -// Copyright 2024, Linaro Limited -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> -// SPDX-License-Identifier: GPL-2.0-or-later - -use std::sync::OnceLock; - -use crate::bindings::Property; - -#[macro_export] -macro_rules! device_class_init { - ($func:ident, props => $props:ident, realize_fn => $realize_fn:expr, legacy_reset_fn => $legacy_reset_fn:expr, vmsd => $vmsd:ident$(,)*) => { - #[no_mangle] - pub unsafe extern "C" fn $func( - klass: *mut $crate::bindings::ObjectClass, - _: *mut ::core::ffi::c_void, - ) { - let mut dc = - ::core::ptr::NonNull::new(klass.cast::<$crate::bindings::DeviceClass>()).unwrap(); - dc.as_mut().realize = $realize_fn; - dc.as_mut().vmsd = &$vmsd; - $crate::bindings::device_class_set_legacy_reset(dc.as_mut(), $legacy_reset_fn); - $crate::bindings::device_class_set_props(dc.as_mut(), $props.as_mut_ptr()); - } - }; -} - -#[macro_export] -macro_rules! define_property { - ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr, default = $defval:expr$(,)*) => { - $crate::bindings::Property { - name: { - #[used] - static _TEMP: &::core::ffi::CStr = $name; - _TEMP.as_ptr() - }, - info: $prop, - offset: ::core::mem::offset_of!($state, $field) - .try_into() - .expect("Could not fit offset value to type"), - bitnr: 0, - bitmask: 0, - set_default: true, - defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval.into() }, - arrayoffset: 0, - arrayinfo: ::core::ptr::null(), - arrayfieldsize: 0, - link_type: ::core::ptr::null(), - } - }; - ($name:expr, $state:ty, $field:expr, $prop:expr, $type:expr$(,)*) => { - $crate::bindings::Property { - name: { - #[used] - static _TEMP: &::core::ffi::CStr = $name; - _TEMP.as_ptr() - }, - info: $prop, - offset: ::core::mem::offset_of!($state, $field) - .try_into() - .expect("Could not fit offset value to type"), - bitnr: 0, - bitmask: 0, - set_default: false, - defval: $crate::bindings::Property__bindgen_ty_1 { i: 0 }, - arrayoffset: 0, - arrayinfo: ::core::ptr::null(), - arrayfieldsize: 0, - link_type: ::core::ptr::null(), - } - }; -} - -#[repr(C)] -pub struct Properties<const N: usize>(pub OnceLock<[Property; N]>, pub fn() -> [Property; N]); - -impl<const N: usize> Properties<N> { - pub fn as_mut_ptr(&mut self) -> *mut Property { - _ = self.0.get_or_init(self.1); - self.0.get_mut().unwrap().as_mut_ptr() - } -} - -#[macro_export] -macro_rules! declare_properties { - ($ident:ident, $($prop:expr),*$(,)*) => { - - const fn _calc_prop_len() -> usize { - let mut len = 1; - $({ - _ = stringify!($prop); - len += 1; - })* - len - } - const PROP_LEN: usize = _calc_prop_len(); - - fn _make_properties() -> [$crate::bindings::Property; PROP_LEN] { - [ - $($prop),*, - unsafe { ::core::mem::MaybeUninit::<$crate::bindings::Property>::zeroed().assume_init() }, - ] - } - - #[no_mangle] - pub static mut $ident: $crate::device_class::Properties<PROP_LEN> = $crate::device_class::Properties(::std::sync::OnceLock::new(), _make_properties); - }; -} - -#[macro_export] -macro_rules! vm_state_description { - ($(#[$outer:meta])* - $name:ident, - $(name: $vname:expr,)* - $(unmigratable: $um_val:expr,)* - ) => { - #[used] - $(#[$outer])* - pub static $name: $crate::bindings::VMStateDescription = $crate::bindings::VMStateDescription { - $(name: { - #[used] - static VMSTATE_NAME: &::core::ffi::CStr = $vname; - $vname.as_ptr() - },)* - unmigratable: true, - ..unsafe { ::core::mem::MaybeUninit::<$crate::bindings::VMStateDescription>::zeroed().assume_init() } - }; - } -} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index e72fb4b4bb13b0982f828b6ec1cfe848c3e6bdf0..b94adc15288cdc62de7679988f549ebd80f895d7 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -27,11 +27,7 @@ unsafe impl Sync for bindings::Property {} unsafe impl Sync for bindings::TypeInfo {} unsafe impl Sync for bindings::VMStateDescription {} -pub mod definitions; -pub mod device_class; - -#[cfg(test)] -mod tests; +pub mod objects; use std::alloc::{GlobalAlloc, Layout}; diff --git a/rust/qemu-api/src/objects.rs b/rust/qemu-api/src/objects.rs new file mode 100644 index 0000000000000000000000000000000000000000..5c6762023ed6914f9c6b7dd16a5e07f778c2d4fa --- /dev/null +++ b/rust/qemu-api/src/objects.rs @@ -0,0 +1,90 @@ +// Copyright 2024, Linaro Limited +// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> +// SPDX-License-Identifier: GPL-2.0-or-later + +//! Implementation traits for QEMU objects, devices. + +use ::core::ffi::{c_int, c_void, CStr}; + +use crate::bindings::{DeviceState, Error, MigrationPriority, Object, ObjectClass, TypeInfo}; + +/// Trait a type must implement to be registered with QEMU. +pub trait ObjectImpl { + type Class: ClassImpl; + const TYPE_NAME: &'static CStr; + const PARENT_TYPE_NAME: Option<&'static CStr>; + const ABSTRACT: bool; + + unsafe fn instance_init(&mut self) {} + fn instance_post_init(&mut self) {} + fn instance_finalize(&mut self) {} +} + +/// The `extern`/`unsafe` analogue of [`ObjectImpl`]; it is used internally by `#[derive(Object)]` +/// and should not be implemented manually. +pub unsafe trait ObjectImplUnsafe { + const TYPE_INFO: TypeInfo; + + const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; + const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)>; + const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)>; +} + +/// Methods for QOM class types. +pub trait ClassImpl { + type Object: ObjectImpl; + + unsafe fn class_init(&mut self, _data: *mut core::ffi::c_void) {} + unsafe fn class_base_init(&mut self, _data: *mut core::ffi::c_void) {} +} + +/// The `extern`/`unsafe` analogue of [`ClassImpl`]; it is used internally by `#[derive(Object)]` +/// and should not be implemented manually. +pub unsafe trait ClassImplUnsafe { + const CLASS_INIT: Option<unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void)>; + const CLASS_BASE_INIT: Option< + unsafe extern "C" fn(klass: *mut ObjectClass, data: *mut c_void), + >; +} + +/// Implementation methods for device types. +pub trait DeviceImpl: ObjectImpl { + fn realize(&mut self) {} + fn reset(&mut self) {} +} + +/// The `extern`/`unsafe` analogue of [`DeviceImpl`]; it is used internally by `#[derive(Device)]` +/// and should not be implemented manually. +pub unsafe trait DeviceImplUnsafe { + const REALIZE: Option<unsafe extern "C" fn(dev: *mut DeviceState, _errp: *mut *mut Error)>; + const RESET: Option<unsafe extern "C" fn(dev: *mut DeviceState)>; +} + +/// Constant metadata and implementation methods for types with device migration state. +pub trait Migrateable: DeviceImplUnsafe { + const NAME: Option<&'static CStr> = None; + const UNMIGRATABLE: bool = true; + const EARLY_SETUP: bool = false; + const VERSION_ID: c_int = 1; + const MINIMUM_VERSION_ID: c_int = 1; + const PRIORITY: MigrationPriority = MigrationPriority::MIG_PRI_DEFAULT; + + unsafe fn pre_load(&mut self) -> c_int { + 0 + } + unsafe fn post_load(&mut self, _version_id: c_int) -> c_int { + 0 + } + unsafe fn pre_save(&mut self) -> c_int { + 0 + } + unsafe fn post_save(&mut self) -> c_int { + 0 + } + unsafe fn needed(&mut self) -> bool { + false + } + unsafe fn dev_unplug_pending(&mut self) -> bool { + false + } +} diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs deleted file mode 100644 index df54edbd4e27e7d2aafc243355d1826d52497c21..0000000000000000000000000000000000000000 --- a/rust/qemu-api/src/tests.rs +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2024, Linaro Limited -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org> -// SPDX-License-Identifier: GPL-2.0-or-later - -use crate::{ - bindings::*, declare_properties, define_property, device_class_init, vm_state_description, -}; - -#[test] -fn test_device_decl_macros() { - // Test that macros can compile. - vm_state_description! { - VMSTATE, - name: c"name", - unmigratable: true, - } - - #[repr(C)] - pub struct DummyState { - pub char_backend: CharBackend, - pub migrate_clock: bool, - } - - declare_properties! { - DUMMY_PROPERTIES, - define_property!( - c"chardev", - DummyState, - char_backend, - unsafe { &qdev_prop_chr }, - CharBackend - ), - define_property!( - c"migrate-clk", - DummyState, - migrate_clock, - unsafe { &qdev_prop_bool }, - bool - ), - } - - device_class_init! { - dummy_class_init, - props => DUMMY_PROPERTIES, - realize_fn => None, - reset_fn => None, - vmsd => VMSTATE, - } -} diff --git a/subprojects/packagefiles/syn-2-rs/meson.build b/subprojects/packagefiles/syn-2-rs/meson.build index a53335f3092e06723039513a1bf5a0d35b4afcd7..9f56ce1c24d0ff86e9b0146b0f82c37ac868fab7 100644 --- a/subprojects/packagefiles/syn-2-rs/meson.build +++ b/subprojects/packagefiles/syn-2-rs/meson.build @@ -24,6 +24,7 @@ _syn_rs = static_library( '--cfg', 'feature="printing"', '--cfg', 'feature="clone-impls"', '--cfg', 'feature="proc-macro"', + '--cfg', 'feature="extra-traits"', ], dependencies: [ quote_dep,
Add a new derive procedural macro to declare device models. Add corresponding DeviceImpl trait after already existing ObjectImpl trait. At the same time, add instance_init, instance_post_init, instance_finalize methods to the ObjectImpl trait and call them from the ObjectImplUnsafe trait, which is generated by the procedural macro. This allows all the boilerplate device model registration to be handled by macros, and all pertinent details to be declared through proc macro attributes or trait associated constants and methods. The device class can now be generated automatically and the name can be optionally overridden: ------------------------ >8 ------------------------ #[repr(C)] #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::Device)] #[device(class_name_override = PL011Class)] /// PL011 Device Model in QEMU pub struct PL011State { ------------------------ >8 ------------------------ Properties are now marked as field attributes: ------------------------ >8 ------------------------ #[property(name = c"chardev", qdev_prop = qdev_prop_chr)] pub char_backend: CharBackend, ------------------------ >8 ------------------------ Object methods (instance_init, etc) methods are now trait methods: ------------------------ >8 ------------------------ /// Trait a type must implement to be registered with QEMU. pub trait ObjectImpl { type Class: ClassImpl; const TYPE_NAME: &'static CStr; const PARENT_TYPE_NAME: Option<&'static CStr>; const ABSTRACT: bool; unsafe fn instance_init(&mut self) {} fn instance_post_init(&mut self) {} fn instance_finalize(&mut self) {} } ------------------------ >8 ------------------------ Device methods (realize/reset etc) are now safe idiomatic trait methods: ------------------------ >8 ------------------------ /// Implementation methods for device types. pub trait DeviceImpl: ObjectImpl { fn realize(&mut self) {} fn reset(&mut self) {} } ------------------------ >8 ------------------------ The derive Device macro is responsible for creating all the extern "C" FFI functions that QEMU needs to call these methods. Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- rust/hw/char/pl011/src/device.rs | 124 +++----- rust/hw/char/pl011/src/device_class.rs | 70 ----- rust/hw/char/pl011/src/lib.rs | 1 - rust/qemu-api-macros/src/device.rs | 433 ++++++++++++++++++++++++++ rust/qemu-api-macros/src/lib.rs | 45 +-- rust/qemu-api-macros/src/object.rs | 107 +++++++ rust/qemu-api-macros/src/symbols.rs | 55 ++++ rust/qemu-api-macros/src/utilities.rs | 152 +++++++++ rust/qemu-api/meson.build | 3 +- rust/qemu-api/src/definitions.rs | 97 ------ rust/qemu-api/src/device_class.rs | 128 -------- rust/qemu-api/src/lib.rs | 6 +- rust/qemu-api/src/objects.rs | 90 ++++++ rust/qemu-api/src/tests.rs | 49 --- subprojects/packagefiles/syn-2-rs/meson.build | 1 + 15 files changed, 902 insertions(+), 459 deletions(-)