Re: [PATCH v8 6/8] rust: add crate to expose bindings and interfaces

[Junjie Mao <junjie.mao@intel.com>]

On 8/26/2024 2:12 PM, Manos Pitsidianakis wrote:
> On Mon, 26 Aug 2024 08:03, Junjie Mao <junjie.mao@intel.com> wrote:
>> Hi Manos,
>>
>> On 8/23/2024 4:11 PM, Manos Pitsidianakis wrote:
>>> Add rust/qemu-api, which exposes rust-bindgen generated FFI bindings and
>>> provides some declaration macros for symbols visible to the rest of
>>> QEMU.
>>>
>>> Co-authored-by: Junjie Mao <junjie.mao@intel.com>
>>> Co-authored-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Junjie Mao <junjie.mao@intel.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> ---
>>>   MAINTAINERS                       |   6 ++
>>>   rust/meson.build                  |   1 +
>>>   rust/qemu-api/.gitignore          |   2 +
>>>   rust/qemu-api/Cargo.lock          |   7 +++
>>>   rust/qemu-api/Cargo.toml          |  26 ++++++++
>>>   rust/qemu-api/README.md           |  17 +++++
>>>   rust/qemu-api/build.rs            |  14 +++++
>>>   rust/qemu-api/meson.build         |  20 ++++++
>>>   rust/qemu-api/rustfmt.toml        |   1 +
>>>   rust/qemu-api/src/definitions.rs  | 109 ++++++++++++++++++++++++++++++++
>>>   rust/qemu-api/src/device_class.rs | 128 ++++++++++++++++++++++++++++++++++++++
>>>   rust/qemu-api/src/lib.rs          | 102 ++++++++++++++++++++++++++++++
>>>   rust/qemu-api/src/tests.rs        |  49 +++++++++++++++
>>>   rust/rustfmt.toml                 |   7 +++
>>>   14 files changed, 489 insertions(+)
>>>
[snip]
>>> diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
>>> new file mode 100644
>>> index 0000000000..4abd0253bd
>>> --- /dev/null
>>> +++ b/rust/qemu-api/src/definitions.rs
>>> @@ -0,0 +1,109 @@
>>> +// 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.
>>> +
>>> +/// Trait a type must implement to be registered with QEMU.
>>> +pub trait ObjectImpl {
>>> +    type Class;
>>> +    const TYPE_INFO: crate::bindings::TypeInfo;
>>> +    const TYPE_NAME: &'static ::core::ffi::CStr;
>>> +    const PARENT_TYPE_NAME: Option<&'static ::core::ffi::CStr>;
>>> +    const INSTANCE_INIT: ::core::option::Option<
>>> +        unsafe extern "C" fn(obj: *mut crate::bindings::Object),
>>> +    >;
>>> +    const INSTANCE_POST_INIT: ::core::option::Option<
>>> +        unsafe extern "C" fn(obj: *mut crate::bindings::Object),
>>> +    >;
>>> +    const INSTANCE_FINALIZE: ::core::option::Option<
>>> +        unsafe extern "C" fn(obj: *mut crate::bindings::Object),
>>> +    >;
>>> +    const ABSTRACT: bool;
>>> +}
>>> +
>>> +pub trait Class {
>>> +    const CLASS_INIT: ::core::option::Option<
>>> +        unsafe extern "C" fn(
>>> +            klass: *mut crate::bindings::ObjectClass,
>>> +            data: *mut core::ffi::c_void,
>>> +        ),
>>> +    >;
>>> +    const CLASS_BASE_INIT: ::core::option::Option<
>>> +        unsafe extern "C" fn(
>>> +            klass: *mut crate::bindings::ObjectClass,
>>> +            data: *mut core::ffi::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() = {
>>> +            assert!($type < 
>>> $crate::bindings::module_init_type_MODULE_INIT_MAX);
>>> +
>>> +            extern "C" fn __load() {
>>> +                unsafe {
>>> +                    $crate::bindings::register_module_init(Some($func), $type);
>>> +                }
>>> +            }
>>> +
>>> +            __load
>>> +        };
>>> +    };
>>> +    (qom: $func:ident => $body:block) => {
>>
>> This arm looks duplicating what #[derive(Object)] is for, while both have 
>> their strengths and limitations: module_init!() provides more flexibility on 
>> the registration function body, and #[derive(Object)] is much more convenient 
>> to use.
>>
>> Complex registration functions are not rare, and thus the Rust APIs should 
>> ideally have both strengths: clean type declaration in most cases, and full 
>> flexibility when needed. In the current codebase, we have ~1080 uses of 
>> type_init(), with 750 of them having a registration function as simple as a 
>> single call to type_register_static() (disclaimer: those numbers are collected 
>> via brute-force searches and may not be accurate). More complex cases include:
>>
>> 1. Registering multiple types (e.g., multiple models of same device) that 
>> share the same data structure, e.g., hw/misc/aspeed_xdma.c and 
>> hw/xtensa/xtfpga.c. There are ~200 uses of this kind in the codebase.
>>
>> 2. Use domain-specific registration function, e.g., ui/egl-headless.c, 
>> audio/ossaudio.c and hw/virtio/virtio-net-pci.c.
>>
>> 3. Other device-specific operations, e.g., hw/net/spapr_llan.c.
>>
> 
> This is why I left the decl macro, I was prototyping this series with a second 
> Rust device (that is not included in the patches) and I needed more logic in the 
> module init.
> 
>> My rough idea is to define a proc macro around an impl block to collect 
>> constants (type names, parent names, etc.) as tokens and callbacks (class 
>> init, instance init, etc.) as functions, from which we generate TypeInfo and 
>> (optionally) type registration code. As an example:
> 
> Do you think we should not use a trait to define type info at all by the way?
> 

I'm not sure, to be honest. Traits are a great way to specify a series of 
functions and mostly fit our needs here. I'm still thinking about how a 
trait-based approach works for multi-model devices where multiple TypeInfo can 
be defined for one structure. A naive way is to define one struct for each 
TypeInfo, which should work but does not look perfect to me.

>>
>>   pub struct PL011State {
>>     ...
>>   }
>>
>>   #[qemu_type(name = "pl011", parent = TYPE_SYS_BUS_DEVICE, (abstract)*)]
>>   impl PL011State {
>>     #[class_init]
>>     pub fn class_init(klass: *mut ObjectClass, data: *mut core::ffi::c_void) {
>>       ...
>>     }
>>
>>     #[instance_init]
>>     pub fn init(obj: *mut Object) { ... }
>>
>>     ...
>>   }
>>
>> The proc macro then generates a TypeInfo instance named TYPE_INFO_pl011, with 
>> optional callbacks being None when not given. A registration function will 
>> also be generated unless qemu_type! has a no_register token.
> 
> Maybe this too can be a trait method people can override with a blank 
> implementation to avoid registration...
> 

Agree.

>> Devices can still use module_init! to define their own registration function.
>>
>> The class_init callback is specified together with instance_init because it is 
>> common for multi-model devices to provide a different class_init even they 
>> share the same class structure. Refer to hw/misc/aspeed_xdma.c for an example.
> 
> Thanks I will take a look. QEMU Classes are a bit complex indeed.
> 
>>
>> What do you think? It is still preliminary and the example can have 
>> grammatical issues, but I can try drafting if you think that is a good direction.
> 
> In my plan I wanted to eventually have all these callbacks available to Rust 
> code via trait methods which only take rust references instead of pointers. Then 
> the proc macros would generate extern "C" wrappers for each of them, make a 
> typeinfo declaration, set everything up. I like your approach too. Should we 
> wait until we have an actual device that requires redesigning this? We're free 
> to change things anyway.
> 

Your idea looks promising, too, esp. the "take rust references" part. It may be 
difficult to figure out if a callback should be None since the trait will always 
define a default, empty implementation, but the performance overhead of calling 
empty constructors / destructors should be negligible.

I agree that we'd better try a variety of device types to better understand 
different use cases before we conclude on the API design. I'll also try 
prototyping some device for a deeper understanding of the current APIs.

---
Best Regards
Junjie Mao

>>
>> ---
>> Best Regards
>> Junjie Mao
>>
>>> +        // 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>(),
>>> +            instance_align: ::core::mem::align_of::<$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>(),
>>> +            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(),
>>> +        };
>>> +    }
>>> +}