Re: [RFC PATCH v4 3/7] rust: add crate to expose bindings and interfaces

[Paolo Bonzini <pbonzini@redhat.com>]

On 7/4/24 14:15, 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.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   MAINTAINERS                       |   7 ++
>   rust/.cargo/config.toml           |   2 +
>   rust/qemu-api/.gitignore          |   2 +
>   rust/qemu-api/Cargo.lock          |   7 ++
>   rust/qemu-api/Cargo.toml          |  59 ++++++++++++++
>   rust/qemu-api/README.md           |  17 ++++
>   rust/qemu-api/build.rs            |  48 +++++++++++
>   rust/qemu-api/deny.toml           |  57 +++++++++++++
>   rust/qemu-api/meson.build         |   0
>   rust/qemu-api/rustfmt.toml        |   1 +
>   rust/qemu-api/src/bindings.rs     |   8 ++
>   rust/qemu-api/src/definitions.rs  | 112 +++++++++++++++++++++++++
>   rust/qemu-api/src/device_class.rs | 131 ++++++++++++++++++++++++++++++

I'd rather put the various definitions in directories that roughly 
correspond to the C files, so rust/qemu-api/src/{util,qom,hw/core/}.
> +correctness = { level = "deny", priority = -1 }
> +suspicious = { level = "deny", priority = -1 }
> +complexity = { level = "deny", priority = -1 }
> +perf = { level = "deny", priority = -1 }
> +cargo = { level = "deny", priority = -1 }
> +nursery = { level = "deny", priority = -1 }
> +style = { level = "deny", priority = -1 }

Groups are problematic because they can (and will) cause breakage when 
new failures are added.  I think we should have a non-fatal job to test 
with groups, but by default we can add a large number of lints and 
"allow" the groups.

> +cast_ptr_alignment = "allow"

Why?

> +missing_safety_doc = "allow"

Please add it to individual files at the top.

> diff --git a/rust/qemu-api/README.md b/rust/qemu-api/README.md
> new file mode 100644
> index 0000000000..f16a1a929d
> --- /dev/null
> +++ b/rust/qemu-api/README.md
> @@ -0,0 +1,17 @@
> +# QEMU bindings and API wrappers
> +
> +This library exports helper Rust types, Rust macros and C FFI bindings for internal QEMU APIs.
> +
> +The C bindings can be generated with `bindgen`, using this build target:
> +
> +```console
> +$ ninja bindings-aarch64-softmmu.rs
> +```
> +
> +## Generate Rust documentation
> +
> +To generate docs for this crate, including private items:
> +
> +```sh
> +cargo doc --no-deps --document-private-items
> +```
> diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
> new file mode 100644
> index 0000000000..13164f8371
> --- /dev/null
> +++ b/rust/qemu-api/build.rs
> @@ -0,0 +1,48 @@
> +// Copyright 2024 Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +// SPDX-License-Identifier: GPL-2.0 OR GPL-3.0-or-later
> +
> +use std::{env, path::Path};
> +
> +fn main() {
> +    println!("cargo:rerun-if-env-changed=MESON_BUILD_ROOT");
> +    println!("cargo:rerun-if-changed=src/bindings.rs.inc");
> +
> +    let out_dir = env::var_os("OUT_DIR").unwrap();
> +
> +    if let Some(build_dir) = std::env::var_os("MESON_BUILD_ROOT") {
> +        let mut build_dir = Path::new(&build_dir).to_path_buf();
> +        let mut out_dir = Path::new(&out_dir).to_path_buf();
> +        assert!(
> +            build_dir.exists(),
> +            "MESON_BUILD_ROOT value does not exist on filesystem: {}",
> +            build_dir.display()
> +        );
> +        assert!(
> +            build_dir.is_dir(),
> +            "MESON_BUILD_ROOT value is not actually a directory: {}",
> +            build_dir.display()
> +        );
> +        // TODO: add logic for other guest target architectures.
> +        build_dir.push("bindings-aarch64-softmmu.rs");
> +        let bindings_rs = build_dir;
> +        assert!(
> +            bindings_rs.exists(),
> +            "MESON_BUILD_ROOT/bindings-aarch64-softmmu.rs does not exist on filesystem: {}",
> +            bindings_rs.display()
> +        );
> +        assert!(
> +            bindings_rs.is_file(),
> +            "MESON_BUILD_ROOT/bindings-aarch64-softmmu.rs is not a file: {}",
> +            bindings_rs.display()
> +        );
> +        out_dir.push("bindings.rs");
> +        std::fs::copy(bindings_rs, out_dir).unwrap();
> +        println!("cargo:rustc-cfg=MESON_BINDINGS_RS");
> +    } else if !Path::new("src/bindings.rs.inc").exists() {
> +        panic!(
> +            "No generated C bindings found! Either build them manually with bindgen or with meson \
> +             (`ninja bindings-aarch64-softmmu.rs`) and copy them to src/bindings.rs.inc, or build \
> +             through meson."
> +        );
> +    }
> +}
> diff --git a/rust/qemu-api/deny.toml b/rust/qemu-api/deny.toml
> new file mode 100644
> index 0000000000..3992380509
> --- /dev/null
> +++ b/rust/qemu-api/deny.toml
> @@ -0,0 +1,57 @@
> +# cargo-deny configuration file
> +
> +[graph]
> +targets = [
> +    "aarch64-unknown-linux-gnu",
> +    "x86_64-unknown-linux-gnu",
> +    "x86_64-apple-darwin",
> +    "aarch64-apple-darwin",
> +    "x86_64-pc-windows-gnu",
> +    "aarch64-pc-windows-gnullvm",

Do we actually support LLVM Windows targets?

> +unsafe impl Sync for TypeInfo {}
> +unsafe impl Sync for VMStateDescription {}
> +
> +#[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")]

I'd rather use the ctor crate.  Also, this should be src/util/module.rs 
so that it's easy to find the matching C source.

> +        pub static LOAD_MODULE: extern "C" fn() = {
> +            assert!($type < $crate::bindings::module_init_type_MODULE_INIT_MAX);
> +
> +            extern "C" fn __load() {
> +                // ::std::panic::set_hook(::std::boxed::Box::new(|_| {}));
> +
> +                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() {
> +                // ::std::panic::set_hook(::std::boxed::Box::new(|_| {}));
> +                #[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 {
> +    ($(#[$outer:meta])*
> +     $name:ident: $t:ty,
> +     $(name: $tname:expr,)*
> +     $(parent: $pname:expr,)*
> +     $(instance_init: $ii_fn:expr,)*
> +     $(instance_post_init: $ipi_fn:expr,)*
> +     $(instance_finalize: $if_fn:expr,)*
> +     $(abstract_: $a_val:expr,)*
> +     $(class_init: $ci_fn:expr,)*
> +     $(class_base_init: $cbi_fn:expr,)*
> +    ) => {
> +        #[used]
> +        $(#[$outer])*
> +        pub static $name: $crate::bindings::TypeInfo = $crate::bindings::TypeInfo {
> +                $(name: {
> +                #[used]
> +                static TYPE_NAME: &::core::ffi::CStr = $tname;
> +                $tname.as_ptr()
> +            },)*

Please use the cstr crate.  Even better, add a trait

pub unsafe trait ObjectType: Sized {
     const TYPE_NAME: &'static CStr;
}

and then just <$t as ObjectType>::TYPE_NAME

Likewise, the parent could be mandatory like

+     $name:ident: $t:ty($p:ty)

and use <$p as ObjectType>::TYPE_NAME.

> +            ..unsafe { MaybeUninit::<$crate::bindings::TypeInfo>::zeroed().assume_init() }

I suggest something like the Zeroed trait I had in my experiment:

==========
#![allow(clippy::undocumented_unsafe_blocks)]

use std::mem::MaybeUninit;

/// Trait providing an easy way to obtain an all-zero
/// value for a struct
///
/// # Safety
///
/// Only add this to a type if `MaybeUninit::zeroed().assume_init()`
/// is valid for that type.

pub unsafe trait Zeroed: Sized {
     fn zeroed() -> Self {
         // SAFETY: If this weren't safe, just do not add the
         // trait to a type.
         unsafe { MaybeUninit::zeroed().assume_init() }
     }
}

// Put here all the impls that you need for the bindgen-provided types.
unsafe impl Zeroed for crate::bindings::TypeInfo {}
=========

So that you can simply write Zeroed::zeroed(); there are other 
occurrences of this for Property, VMStateDescription, MemoryRegionOps.

> +use std::sync::OnceLock;

OnceLock is not needed, initialization is single-threaded and executed 
once only.  You can just make declare_properties return a fn() and

$crate::bindings::device_class_set_props(dc.as_mut(), $props());

> +            dc.as_mut().realize = $realize_fn;
> +            dc.as_mut().reset = $reset_fn;

Nice.  Might also add the same trick as type_info!, for example 
$(realize: $realize_fn:expr,)*, to remove the "Some()".


> +#[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()
> +            },

Also please use cstr!() here.

> +            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(),
> +        }
> +    };

I think you can just use "..Zeroed::zeroed()" instead of listing all the 
fields one by one.

> +#[repr(C)]
> +pub struct Properties<const N: usize>(pub OnceLock<[Property; N]>, pub fn() -> [Property; N]);
> +
> +impl<const N: usize> Properties<N> {
> +    pub unsafe fn as_mut_ptr(&mut self) -> *mut Property {
> +        _ = self.0.get_or_init(self.1);
> +        self.0.get_mut().unwrap().as_mut_ptr()
> +    }
> + > +        #[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
> new file mode 100644
> index 0000000000..74825c84e7
> --- /dev/null
> +++ b/rust/qemu-api/src/lib.rs
> @@ -0,0 +1,29 @@
> +// Copyright 2024 Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +// SPDX-License-Identifier: GPL-2.0 OR GPL-3.0-or-later
> +
> +#![doc = include_str!("../README.md")]
> +
> +// FIXME: remove improper_ctypes
> +#[allow(
> +    improper_ctypes_definitions,
> +    improper_ctypes,
> +    non_camel_case_types,
> +    non_snake_case,
> +    non_upper_case_globals
> +)]
> +#[allow(
> +    clippy::missing_const_for_fn,
> +    clippy::useless_transmute,

This one definitely shouldn't be there.

Paolo

> +    clippy::too_many_arguments,
> +    clippy::approx_constant,
> +    clippy::use_self,
> +    clippy::cast_lossless,
> +)]
> +#[rustfmt::skip]
> +pub mod bindings;
> +
> +pub mod definitions;
> +pub mod device_class;
> +
> +#[cfg(test)]
> +mod tests;
> diff --git a/rust/qemu-api/src/tests.rs b/rust/qemu-api/src/tests.rs
> new file mode 100644
> index 0000000000..88c26308ee
> --- /dev/null
> +++ b/rust/qemu-api/src/tests.rs
> @@ -0,0 +1,48 @@
> +// Copyright 2024 Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +// SPDX-License-Identifier: GPL-2.0 OR GPL-3.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,
> +    }
> +}