qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [RFC PATCH v4 3/7] rust: add crate to expose bindings and interfaces
Date: Mon, 8 Jul 2024 17:40:49 +0200
User-agent: Mozilla Thunderbird 
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,
+    }
+}
reply via email to
[Prev in Thread] Current Thread [Next in Thread]