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-pbonz...@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.pitsidiana...@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-pbonz...@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.pitsidiana...@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.pitsidiana...@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-pbonz...@redhat.com/.
Paolo
