* [PATCH 01/12] rust: cell: add wrapper for FFI types
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-02-27 14:22 ` [PATCH 02/12] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
` (10 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Inspired by the same-named type in Linux. This type provides the compiler
with a correct view of what goes on with FFI types. In addition, it
separates the glue code from the bindgen-generated code, allowing
traits such as Send, Sync or Zeroable to be specified independently
for C and Rust structs.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/devel/rust.rst | 34 +++++--
rust/qemu-api/src/cell.rs | 197 ++++++++++++++++++++++++++++++++++++--
2 files changed, 216 insertions(+), 15 deletions(-)
diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 5d8aa3a45bc..784c3e40bdc 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -296,15 +296,33 @@ of ``&mut self``; access to internal fields must use *interior mutability*
to go from a shared reference to a ``&mut``.
Whenever C code provides you with an opaque ``void *``, avoid converting it
-to a Rust mutable reference, and use a shared reference instead. Rust code
-will then have to use QEMU's ``BqlRefCell`` and ``BqlCell`` type, which
-enforce that locking rules for the "Big QEMU Lock" are respected. These cell
-types are also known to the ``vmstate`` crate, which is able to "look inside"
-them when building an in-memory representation of a ``struct``'s layout.
-Note that the same is not true of a ``RefCell`` or ``Mutex``.
+to a Rust mutable reference, and use a shared reference instead. The
+``qemu_api::cell`` module provides wrappers that can be used to tell the
+Rust compiler about interior mutability, and optionally to enforce locking
+rules for the "Big QEMU Lock". In the future, similar cell types might
+also be provided for ``AioContext``-based locking as well.
-In the future, similar cell types might also be provided for ``AioContext``-based
-locking as well.
+In particular, device code will usually rely on the ``BqlRefCell`` and
+``BqlCell`` type to ensure that data is accessed correctly under the
+"Big QEMU Lock". These cell types are also known to the ``vmstate``
+crate, which is able to "look inside" them when building an in-memory
+representation of a ``struct``'s layout. Note that the same is not true
+of a ``RefCell`` or ``Mutex``.
+
+Bindings code instead will usually use the ``Opaque`` type, which hides
+the contents of the underlying struct and can be easily converted to
+a raw pointer, for use in calls to C functions. It can be used for
+example as follows::
+
+ #[repr(transparent)]
+ #[derive(Debug)]
+ pub struct Object(Opaque<bindings::Object>);
+
+The bindings will then manually check for the big QEMU lock with
+assertions, which allows the wrapper to be declared thread-safe::
+
+ unsafe impl Send for Object {}
+ unsafe impl Sync for Object {}
Writing bindings to C code
''''''''''''''''''''''''''
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index eae4e2ce786..7c00109d0e9 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -27,7 +27,7 @@
// IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.
-//! BQL-protected mutable containers.
+//! QEMU-specific mutable containers
//!
//! Rust memory safety is based on this rule: Given an object `T`, it is only
//! possible to have one of the following:
@@ -43,8 +43,10 @@
//! usually have their pointer shared with the "outside world very early in
//! their lifetime", for example when they create their
//! [`MemoryRegion`s](crate::bindings::MemoryRegion). Therefore, individual
-//! parts of a device must be made mutable in a controlled manner through the
-//! use of cell types.
+//! parts of a device must be made mutable in a controlled manner; this module
+//! provides the tools to do so.
+//!
+//! ## Cell types
//!
//! [`BqlCell<T>`] and [`BqlRefCell<T>`] allow doing this via the Big QEMU Lock.
//! While they are essentially the same single-threaded primitives that are
@@ -71,7 +73,7 @@
//! QEMU device implementations is usually incorrect and can lead to
//! thread-safety issues.
//!
-//! ## `BqlCell<T>`
+//! ### `BqlCell<T>`
//!
//! [`BqlCell<T>`] implements interior mutability by moving values in and out of
//! the cell. That is, an `&mut T` to the inner value can never be obtained as
@@ -91,7 +93,7 @@
//! - [`set`](BqlCell::set): this method replaces the interior value,
//! dropping the replaced value.
//!
-//! ## `BqlRefCell<T>`
+//! ### `BqlRefCell<T>`
//!
//! [`BqlRefCell<T>`] uses Rust's lifetimes to implement "dynamic borrowing", a
//! process whereby one can claim temporary, exclusive, mutable access to the
@@ -111,13 +113,82 @@
//! Multiple immutable borrows are allowed via [`borrow`](BqlRefCell::borrow),
//! or a single mutable borrow via [`borrow_mut`](BqlRefCell::borrow_mut). The
//! thread will panic if these rules are violated or if the BQL is not held.
+//!
+//! ## Opaque wrappers
+//!
+//! The cell types from the previous section are useful at the boundaries
+//! of code that requires interior mutability. When writing glue code that
+//! interacts directly with C structs, however, it is useful to operate
+//! at a lower level.
+//!
+//! C functions often violate Rust's fundamental assumptions about memory
+//! safety by modifying memory even if it is shared. Furthermore, C structs
+//! often start their life uninitialized and may be populated lazily.
+//!
+//! For this reason, this module provides the [`Opaque<T>`] type to opt out
+//! of Rust's usual guarantees about the wrapped type. Access to the wrapped
+//! value is always through raw pointers, obtained via methods like
+//! [`as_mut_ptr()`](Opaque::as_mut_ptr) and [`as_ptr()`](Opaque::as_ptr). These
+//! pointers can then be passed to C functions or dereferenced; both actions
+//! require `unsafe` blocks, making it clear where safety guarantees must be
+//! manually verified. For example
+//!
+//! ```ignore
+//! unsafe {
+//! let state = Opaque::<MyStruct>::uninit();
+//! qemu_struct_init(state.as_mut_ptr());
+//! }
+//! ```
+//!
+//! [`Opaque<T>`] will usually be wrapped one level further, so that
+//! bridge methods can be added to the wrapper:
+//!
+//! ```ignore
+//! pub struct MyStruct(Opaque<bindings::MyStruct>);
+//!
+//! impl MyStruct {
+//! fn new() -> Pin<Box<MyStruct>> {
+//! let result = Box::pin(unsafe { Opaque::uninit() });
+//! unsafe { qemu_struct_init(result.as_mut_ptr()) };
+//! result
+//! }
+//! }
+//! ```
+//!
+//! This pattern of wrapping bindgen-generated types in [`Opaque<T>`] provides
+//! several advantages:
+//!
+//! * The choice of traits to be implemented is not limited by the
+//! bindgen-generated code. For example, [`Drop`] can be added without
+//! disabling [`Copy`] on the underlying bindgen type
+//!
+//! * [`Send`] and [`Sync`] implementations can be controlled by the wrapper
+//! type rather than being automatically derived from the C struct's layout
+//!
+//! * Methods can be implemented in a separate crate from the bindgen-generated
+//! bindings
+//!
+//! * [`Debug`](std::fmt::Debug) and [`Display`](std::fmt::Display)
+//! implementations can be customized to be more readable than the raw C
+//! struct representation
+//!
+//! The [`Opaque<T>`] type does not include BQL validation; it is possible to
+//! assert in the code that the right lock is taken, to use it together
+//! with a custom lock guard type, or to let C code take the lock, as
+//! appropriate. It is also possible to use it with non-thread-safe
+//! types, since by default (unlike [`BqlCell`] and [`BqlRefCell`]
+//! it is neither `Sync` nor `Send`.
+//!
+//! While [`Opaque<T>`] is necessary for C interop, it should be used sparingly
+//! and only at FFI boundaries. For QEMU-specific types that need interior
+//! mutability, prefer [`BqlCell`] or [`BqlRefCell`].
use std::{
cell::{Cell, UnsafeCell},
cmp::Ordering,
fmt,
- marker::PhantomData,
- mem,
+ marker::{PhantomData, PhantomPinned},
+ mem::{self, MaybeUninit},
ops::{Deref, DerefMut},
ptr::NonNull,
};
@@ -840,3 +911,115 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
(**self).fmt(f)
}
}
+
+/// Stores an opaque value that is shared with C code.
+///
+/// Often, C structs can changed when calling a C function even if they are
+/// behind a shared Rust reference, or they can be initialized lazily and have
+/// invalid bit patterns (e.g. `3` for a [`bool`]). This goes against Rust's
+/// strict aliasing rules, which normally prevent mutation through shared
+/// references.
+///
+/// Wrapping the struct with `Opaque<T>` ensures that the Rust compiler does not
+/// assume the usual constraints that Rust structs require, and allows using
+/// shared references on the Rust side.
+///
+/// `Opaque<T>` is `#[repr(transparent)]`, so that it matches the memory layout
+/// of `T`.
+#[repr(transparent)]
+pub struct Opaque<T> {
+ value: UnsafeCell<MaybeUninit<T>>,
+ // PhantomPinned also allows multiple references to the `Opaque<T>`, i.e.
+ // one `&mut Opaque<T>` can coexist with a `&mut T` or any number of `&T`;
+ // see https://docs.rs/pinned-aliasable/latest/pinned_aliasable/.
+ _pin: PhantomPinned,
+}
+
+impl<T> Opaque<T> {
+ /// Creates a new shared reference from a C pointer
+ ///
+ /// # Safety
+ ///
+ /// The pointer must be valid, though it need not point to a valid value.
+ pub unsafe fn from_raw<'a>(ptr: *mut T) -> &'a Self {
+ let ptr = NonNull::new(ptr).unwrap().cast::<Self>();
+ // SAFETY: Self is a transparent wrapper over T
+ unsafe { ptr.as_ref() }
+ }
+
+ /// Creates a new opaque object with uninitialized contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be initialized and pinned before
+ /// calling them.
+ #[allow(clippy::missing_const_for_fn)]
+ pub unsafe fn uninit() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::uninit()),
+ _pin: PhantomPinned,
+ }
+ }
+
+ /// Creates a new opaque object with zeroed contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be pinned (and possibly initialized)
+ /// before calling them.
+ #[allow(clippy::missing_const_for_fn)]
+ pub unsafe fn zeroed() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::zeroed()),
+ _pin: PhantomPinned,
+ }
+ }
+
+ /// Returns a raw mutable pointer to the opaque data.
+ pub const fn as_mut_ptr(&self) -> *mut T {
+ UnsafeCell::get(&self.value).cast()
+ }
+
+ /// Returns a raw pointer to the opaque data.
+ pub const fn as_ptr(&self) -> *const T {
+ self.as_mut_ptr() as *const _
+ }
+
+ /// Returns a raw pointer to the opaque data that can be passed to a
+ /// C function as `void *`.
+ pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
+ UnsafeCell::get(&self.value).cast()
+ }
+}
+
+impl<T> fmt::Debug for Opaque<T> {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ let mut name: String = "Opaque<".to_string();
+ name += std::any::type_name::<T>();
+ name += ">";
+ f.debug_tuple(&name).field(&self.as_ptr()).finish()
+ }
+}
+
+impl<T: Default> Opaque<T> {
+ /// Creates a new opaque object with default contents.
+ ///
+ /// # Safety
+ ///
+ /// Ultimately the pointer to the returned value will be dereferenced
+ /// in another `unsafe` block, for example when passing it to a C function,
+ /// but the functions containing the dereference are usually safe. The
+ /// value returned from `uninit()` must be pinned before calling them.
+ pub unsafe fn new() -> Self {
+ Self {
+ value: UnsafeCell::new(MaybeUninit::new(T::default())),
+ _pin: PhantomPinned,
+ }
+ }
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 02/12] rust: qemu_api_macros: add Wrapper derive macro
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
2025-02-27 14:22 ` [PATCH 01/12] rust: cell: add wrapper for FFI types Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-02-27 14:22 ` [PATCH 03/12] rust: vmstate: add std::pin::Pin as transparent wrapper Paolo Bonzini
` (9 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Add a derive macro that makes it easy to peel off all the layers of
specialness (UnsafeCell, MaybeUninit, etc.) and just get a pointer
to the wrapped type; and likewise add them back starting from a
*mut.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/devel/rust.rst | 8 +--
rust/qemu-api-macros/src/lib.rs | 86 ++++++++++++++++++++++++++++++++-
rust/qemu-api/meson.build | 7 +--
rust/qemu-api/src/cell.rs | 31 ++++++++++++
4 files changed, 123 insertions(+), 9 deletions(-)
diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 784c3e40bdc..64e04bc5c41 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -315,11 +315,13 @@ a raw pointer, for use in calls to C functions. It can be used for
example as follows::
#[repr(transparent)]
- #[derive(Debug)]
+ #[derive(Debug, qemu_api_macros::Wrapper)]
pub struct Object(Opaque<bindings::Object>);
-The bindings will then manually check for the big QEMU lock with
-assertions, which allows the wrapper to be declared thread-safe::
+where the special ``derive`` macro provides useful methods such as
+``from_raw``, ``as_ptr`` and ``as_mut_ptr``. The bindings will then
+manually check for the big QEMU lock with assertions, which allows
+the wrapper to be declared thread-safe::
unsafe impl Send for Object {}
unsafe impl Sync for Object {}
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 7ec218202f4..3df9c48035d 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -6,7 +6,7 @@
use quote::quote;
use syn::{
parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned, token::Comma, Data,
- DeriveInput, Field, Fields, Ident, Meta, Path, Token, Type, Variant, Visibility,
+ DeriveInput, Field, Fields, FieldsUnnamed, Ident, Meta, Path, Token, Type, Variant, Visibility,
};
mod utils;
@@ -33,6 +33,35 @@ fn get_fields<'a>(
}
}
+fn get_unnamed_field<'a>(input: &'a DeriveInput, msg: &str) -> Result<&'a Field, MacroError> {
+ if let Data::Struct(s) = &input.data {
+ let unnamed = match &s.fields {
+ Fields::Unnamed(FieldsUnnamed {
+ unnamed: ref fields,
+ ..
+ }) => fields,
+ _ => {
+ return Err(MacroError::Message(
+ format!("Tuple struct required for {}", msg),
+ s.fields.span(),
+ ))
+ }
+ };
+ if unnamed.len() != 1 {
+ return Err(MacroError::Message(
+ format!("A single field is required for {}", msg),
+ s.fields.span(),
+ ));
+ }
+ Ok(&unnamed[0])
+ } else {
+ Err(MacroError::Message(
+ format!("Struct required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
+
fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
let expected = parse_quote! { #[repr(C)] };
@@ -46,6 +75,19 @@ fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
}
}
+fn is_transparent_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
+ let expected = parse_quote! { #[repr(transparent)] };
+
+ if input.attrs.iter().any(|attr| attr == &expected) {
+ Ok(())
+ } else {
+ Err(MacroError::Message(
+ format!("#[repr(transparent)] required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
+
fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(Object)]")?;
@@ -72,6 +114,48 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
TokenStream::from(expanded)
}
+fn derive_opaque_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
+ is_transparent_repr(&input, "#[derive(Wrapper)]")?;
+
+ let name = &input.ident;
+ let field = &get_unnamed_field(&input, "#[derive(Wrapper)]")?;
+ let typ = &field.ty;
+
+ // TODO: how to add "::qemu_api"? For now, this is only used in the
+ // qemu_api crate so it's not a problem.
+ Ok(quote! {
+ unsafe impl crate::cell::Wrapper for #name {
+ type Wrapped = <#typ as crate::cell::Wrapper>::Wrapped;
+ }
+ impl #name {
+ pub unsafe fn from_raw<'a>(ptr: *mut <Self as crate::cell::Wrapper>::Wrapped) -> &'a Self {
+ let ptr = ::std::ptr::NonNull::new(ptr).unwrap().cast::<Self>();
+ unsafe { ptr.as_ref() }
+ }
+
+ pub const fn as_mut_ptr(&self) -> *mut <Self as crate::cell::Wrapper>::Wrapped {
+ self.0.as_mut_ptr()
+ }
+
+ pub const fn as_ptr(&self) -> *const <Self as crate::cell::Wrapper>::Wrapped {
+ self.0.as_ptr()
+ }
+
+ pub const fn as_void_ptr(&self) -> *mut ::core::ffi::c_void {
+ self.0.as_void_ptr()
+ }
+ }
+ })
+}
+
+#[proc_macro_derive(Wrapper)]
+pub fn derive_opaque(input: TokenStream) -> TokenStream {
+ let input = parse_macro_input!(input as DeriveInput);
+ let expanded = derive_opaque_or_error(input).unwrap_or_else(Into::into);
+
+ TokenStream::from(expanded)
+}
+
#[rustfmt::skip::macros(quote)]
fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(offsets)]")?;
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index bcf1cf780f3..6e52c4bad74 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -42,16 +42,13 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
- dependencies: libc_dep,
+ dependencies: [libc_dep, qemu_api_macros],
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
suite: ['unit', 'rust'])
-qemu_api = declare_dependency(
- link_with: _qemu_api_rs,
- dependencies: qemu_api_macros,
-)
+qemu_api = declare_dependency(link_with: _qemu_api_rs)
# Rust executables do not support objects, so add an intermediate step.
rust_qemu_api_objs = static_library(
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
index 7c00109d0e9..24efb3a2f2a 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -1023,3 +1023,34 @@ pub unsafe fn new() -> Self {
}
}
}
+
+/// Annotates [`Self`] as a transparent wrapper for another type.
+///
+/// Usually defined via the [`qemu_api_macros::Wrapper`] derive macro.
+///
+/// # Examples
+///
+/// ```
+/// # use std::mem::ManuallyDrop;
+/// # use qemu_api::cell::Wrapper;
+/// #[repr(transparent)]
+/// pub struct Example {
+/// inner: ManuallyDrop<String>,
+/// }
+///
+/// unsafe impl Wrapper for Example {
+/// type Wrapped = String;
+/// }
+/// ```
+///
+/// # Safety
+///
+/// `Self` must be a `#[repr(transparent)]` wrapper for the `Wrapped` type,
+/// whether directly or indirectly.
+pub unsafe trait Wrapper {
+ type Wrapped;
+}
+
+unsafe impl<T> Wrapper for Opaque<T> {
+ type Wrapped = T;
+}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 03/12] rust: vmstate: add std::pin::Pin as transparent wrapper
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
2025-02-27 14:22 ` [PATCH 01/12] rust: cell: add wrapper for FFI types Paolo Bonzini
2025-02-27 14:22 ` [PATCH 02/12] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 13:25 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
` (8 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/vmstate.rs | 1 +
1 file changed, 1 insertion(+)
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 24a4dc81e7f..1e7ba531e2a 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -330,6 +330,7 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
impl_vmstate_transparent!(std::cell::Cell<T> where T: VMState);
impl_vmstate_transparent!(std::cell::UnsafeCell<T> where T: VMState);
+impl_vmstate_transparent!(std::pin::Pin<T> where T: VMState);
impl_vmstate_transparent!(crate::cell::BqlCell<T> where T: VMState);
impl_vmstate_transparent!(crate::cell::BqlRefCell<T> where T: VMState);
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (2 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 03/12] rust: vmstate: add std::pin::Pin as transparent wrapper Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 13:48 ` Zhao Liu
2025-03-03 14:28 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 05/12] rust: irq: wrap IRQState with Opaque<> Paolo Bonzini
` (7 subsequent siblings)
11 siblings, 2 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Timers must be pinned in memory, because modify() stores a pointer to them
in the TimerList. To ensure this is the case, replace the separate new()
and init_full() with a single function that returns a pinned box. Because
the only way to obtain a Timer is through Timer::new_full(), modify()
knows that the timer it got is also pinned. In the future the pinning
requirement will be expressed through the pin_init crate instead.
Note that Timer is a bit different from other users of Opaque, in that
it is created in Rust code rather than C code. This is why it has to
use the unsafe Opaque::new() function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 7 -----
rust/hw/timer/hpet/src/hpet.rs | 23 ++++++++---------
rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++------------
3 files changed, 41 insertions(+), 36 deletions(-)
diff --git a/meson.build b/meson.build
index 0a2c61d2bfa..fc02e5fc763 100644
--- a/meson.build
+++ b/meson.build
@@ -4098,13 +4098,6 @@ if have_rust
foreach enum : c_bitfields
bindgen_args += ['--bitfield-enum', enum]
endforeach
- c_nocopy = [
- 'QEMUTimer',
- ]
- # Used to customize Drop trait
- foreach struct : c_nocopy
- bindgen_args += ['--no-copy', struct]
- endforeach
# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
#
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..ce4b289d0c8 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,6 +4,7 @@
use std::{
ffi::CStr,
+ pin::Pin,
ptr::{addr_of_mut, null_mut, NonNull},
slice::from_ref,
};
@@ -156,7 +157,7 @@ pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
index: usize,
- qemu_timer: Option<Box<Timer>>,
+ qemu_timer: Option<Pin<Box<Timer>>>,
/// timer block abstraction containing this timer
state: Option<NonNull<HPETState>>,
@@ -189,18 +190,14 @@ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
}
fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Box::new({
- let mut t = Timer::new();
- t.init_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- );
- t
- }));
+ self.qemu_timer = Some(Timer::new_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ &self.get_state().timers[self.index],
+ ));
}
fn get_state(&self) -> &HPETState {
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..a8b2ab058b6 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -2,40 +2,51 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::os::raw::{c_int, c_void};
+use std::{
+ os::raw::{c_int, c_void},
+ pin::Pin,
+};
use crate::{
bindings::{self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType},
callbacks::FnCall,
+ cell::Opaque,
};
-pub type Timer = bindings::QEMUTimer;
-pub type TimerListGroup = bindings::QEMUTimerListGroup;
+/// A safe wrapper around [`bindings::QEMUTimer`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Timer(Opaque<bindings::QEMUTimer>);
+
+unsafe impl Send for Timer {}
+unsafe impl Sync for Timer {}
+
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct TimerListGroup(Opaque<bindings::QEMUTimerListGroup>);
+
+unsafe impl Send for TimerListGroup {}
+unsafe impl Sync for TimerListGroup {}
impl Timer {
pub const MS: u32 = bindings::SCALE_MS;
pub const US: u32 = bindings::SCALE_US;
pub const NS: u32 = bindings::SCALE_NS;
- pub fn new() -> Self {
- Default::default()
- }
-
- const fn as_mut_ptr(&self) -> *mut Self {
- self as *const Timer as *mut _
- }
-
- pub fn init_full<'timer, 'opaque: 'timer, T, F>(
- &'timer mut self,
+ pub fn new_full<'opaque, T, F>(
timer_list_group: Option<&TimerListGroup>,
clk_type: ClockType,
scale: u32,
attributes: u32,
_cb: F,
opaque: &'opaque T,
- ) where
+ ) -> Pin<Box<Self>>
F: for<'a> FnCall<(&'a T,)>,
{
+ // SAFETY: returning a pinned box makes it certain that the object
+ // will not move when added to a TimerList with `modify()`.
+ let t = unsafe { Box::pin(Self(Opaque::new())) };
let _: () = F::ASSERT_IS_SOME;
/// timer expiration callback
@@ -51,7 +62,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
- self,
+ t.as_mut_ptr(),
if let Some(g) = timer_list_group {
g as *const TimerListGroup as *mut _
} else {
@@ -62,11 +73,14 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
attributes as c_int,
Some(timer_cb),
(opaque as *const T).cast::<c_void>() as *mut c_void,
- )
+ );
}
+ t
}
pub fn modify(&self, expire_time: u64) {
+ // SAFETY: the only way to obtain a Timer is via methods that return a
+ // Pin<Box<Self>>, therefore the timer is pinned
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
@@ -75,6 +89,7 @@ pub fn delete(&self) {
}
}
+// FIXME: use something like PinnedDrop from the pinned_init crate
impl Drop for Timer {
fn drop(&mut self) {
self.delete()
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-02-27 14:22 ` [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
@ 2025-03-03 13:48 ` Zhao Liu
2025-03-03 15:58 ` Paolo Bonzini
2025-03-03 14:28 ` Zhao Liu
1 sibling, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 13:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Thu, Feb 27, 2025 at 03:22:11PM +0100, Paolo Bonzini wrote:
> Date: Thu, 27 Feb 2025 15:22:11 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
> X-Mailer: git-send-email 2.48.1
>
> Timers must be pinned in memory, because modify() stores a pointer to them
> in the TimerList. To ensure this is the case, replace the separate new()
> and init_full() with a single function that returns a pinned box. Because
> the only way to obtain a Timer is through Timer::new_full(), modify()
> knows that the timer it got is also pinned. In the future the pinning
> requirement will be expressed through the pin_init crate instead.
>
> Note that Timer is a bit different from other users of Opaque, in that
> it is created in Rust code rather than C code. This is why it has to
> use the unsafe Opaque::new() function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> meson.build | 7 -----
> rust/hw/timer/hpet/src/hpet.rs | 23 ++++++++---------
> rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++------------
> 3 files changed, 41 insertions(+), 36 deletions(-)
Great! LGTM,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
(But pls wait, I have a question below...)
> @@ -156,7 +157,7 @@ pub struct HPETTimer {
> /// timer N index within the timer block (`HPETState`)
> #[doc(alias = "tn")]
> index: usize,
> - qemu_timer: Option<Box<Timer>>,
> + qemu_timer: Option<Pin<Box<Timer>>>,
I'm removing this Option<> wrapper in migration series. This is because
Option<> can't be treated as pointer as you mentioned in [*].
So for this reason, does this mean that VMStateField cannot accept
Option<>? I realize that all the current VMStateFlags don't seem
compatible with Option<> unless a new flag is introduced.
[*]: https://lore.kernel.org/qemu-devel/9a0389fa-765c-443b-ac2f-7c99ed862982@redhat.com/
Thanks,
Zhao
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-03 13:48 ` Zhao Liu
@ 2025-03-03 15:58 ` Paolo Bonzini
2025-03-04 9:13 ` Zhao Liu
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-03-03 15:58 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 3/3/25 14:48, Zhao Liu wrote:
>> @@ -156,7 +157,7 @@ pub struct HPETTimer {
>> /// timer N index within the timer block (`HPETState`)
>> #[doc(alias = "tn")]
>> index: usize,
>> - qemu_timer: Option<Box<Timer>>,
>> + qemu_timer: Option<Pin<Box<Timer>>>,
>
> I'm removing this Option<> wrapper in migration series. This is because
> Option<> can't be treated as pointer as you mentioned in [*].
>
> So for this reason, does this mean that VMStateField cannot accept
> Option<>? I realize that all the current VMStateFlags don't seem
> compatible with Option<> unless a new flag is introduced.
>
> [*]: https://lore.kernel.org/qemu-devel/9a0389fa-765c-443b-ac2f-7c99ed862982@redhat.com/
Ok, so let's get rid of the option. I didn't really like it anyway...
If the Timer is embedded in the HPETTimer, there needs to be some
"unsafe" in order to make sure that the pinning is observed, and also
because an uninitialized Timer is bad and can cause a NULL pointer
dereference in modify()... i.e. Timer shouldn't have implemented
Default!
However, the lifetime checks in init_full() are preserved, so overall
this is better---at least for now. Linux also had unsafe initialization
for quite some time, so I'm okay with it.
The replacements for this patch are below.
Paolo
From 2d74bdf176b2fbeb6205396d0021f68a9e72bde1 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 3 Mar 2025 16:27:08 +0100
Subject: [PATCH 1/2] rust: hpet: embed Timer without the Option and Box
indirection
This simplifies things for migration, since Option<Box<QEMUTimer>> does not
implement VMState.
This also shows a soundness issue because Timer::new() will leave a NULL
timer list pointer, which can then be dereferenced by Timer::modify(). It
will be fixed shortly.
Suggested-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..02c81ae048f 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -151,14 +151,14 @@ fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
/// HPET Timer Abstraction
#[repr(C)]
-#[derive(Debug, Default, qemu_api_macros::offsets)]
+#[derive(Debug, qemu_api_macros::offsets)]
pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
index: usize,
- qemu_timer: Option<Box<Timer>>,
+ qemu_timer: Timer,
/// timer block abstraction containing this timer
- state: Option<NonNull<HPETState>>,
+ state: NonNull<HPETState>,
// Memory-mapped, software visible timer registers
/// Timer N Configuration and Capability Register
@@ -181,32 +181,34 @@ pub struct HPETTimer {
}
impl HPETTimer {
- fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
- *self = HPETTimer::default();
- self.index = index;
- self.state = NonNull::new(state_ptr);
- self
- }
+ fn init(&mut self, index: usize, state: &HPETState) {
+ *self = HPETTimer {
+ index,
+ qemu_timer: Timer::new(),
+ state: NonNull::new(state as *const _ as *mut _).unwrap(),
+ config: 0,
+ cmp: 0,
+ fsb: 0,
+ cmp64: 0,
+ period: 0,
+ wrap_flag: 0,
+ last: 0,
+ };
- fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Box::new({
- let mut t = Timer::new();
- t.init_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- );
- t
- }));
+ self.qemu_timer.init_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ &state.timers[self.index],
+ )
}
fn get_state(&self) -> &HPETState {
// SAFETY:
// the pointer is convertible to a reference
- unsafe { self.state.unwrap().as_ref() }
+ unsafe { self.state.as_ref() }
}
fn is_int_active(&self) -> bool {
@@ -330,7 +332,7 @@ fn arm_timer(&mut self, tick: u64) {
}
self.last = ns;
- self.qemu_timer.as_ref().unwrap().modify(self.last);
+ self.qemu_timer.modify(self.last);
}
fn set_timer(&mut self) {
@@ -353,7 +355,7 @@ fn set_timer(&mut self) {
fn del_timer(&mut self) {
// Just remove the timer from the timer_list without destroying
// this timer instance.
- self.qemu_timer.as_ref().unwrap().delete();
+ self.qemu_timer.delete();
if self.is_int_active() {
// For level-triggered interrupt, this leaves interrupt status
@@ -581,13 +583,8 @@ fn handle_legacy_irq(&self, irq: u32, level: u32) {
}
fn init_timer(&self) {
- let raw_ptr: *mut HPETState = self as *const HPETState as *mut HPETState;
-
for (index, timer) in self.timers.iter().enumerate() {
- timer
- .borrow_mut()
- .init(index, raw_ptr)
- .init_timer_with_state();
+ timer.borrow_mut().init(index, self);
}
}
--
2.48.1
From 276020645786b6537c50bb37795f281b5d630f27 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 14 Feb 2025 12:06:13 +0100
Subject: [PATCH 2/2] rust: timer: wrap QEMUTimer with Opaque<> and express
pinning requirements
Timers must be pinned in memory, because modify() stores a pointer to them
in the TimerList. To express this requirement, change init_full() to take
a pinned reference. Because the only way to obtain a Timer is through
Timer::new(), which is unsafe, modify() can assume that the timer it got
was later initialized; and because the initialization takes a Pin<&mut
Timer> modify() can assume that the timer is pinned. In the future the
pinning requirement will be expressed through the pin_init crate instead.
Note that Timer is a bit different from other users of Opaque, in that
it is created in Rust code rather than C code. This is why it has to
use the unsafe constructors provided by Opaque; and in fact Timer::new()
is also unsafe, because it leaves it to the caller to invoke init_full()
before modify(). Without a call to init_full(), modify() will cause a
NULL pointer dereference.
An alternative could be to combine new() + init_full() by returning a
pinned box; however, using a reference makes it easier to express
the requirement that the opaque outlives the timer.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 7 -----
rust/hw/timer/hpet/src/hpet.rs | 10 ++++++--
rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++++++--------
3 files changed, 44 insertions(+), 20 deletions(-)
diff --git a/meson.build b/meson.build
index 9b1c0ba6346..4e8f0a6c00d 100644
--- a/meson.build
+++ b/meson.build
@@ -4098,13 +4098,6 @@ if have_rust
foreach enum : c_bitfields
bindgen_args += ['--bitfield-enum', enum]
endforeach
- c_nocopy = [
- 'QEMUTimer',
- ]
- # Used to customize Drop trait
- foreach struct : c_nocopy
- bindgen_args += ['--no-copy', struct]
- endforeach
# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
#
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 02c81ae048f..3d3d6ef8eec 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,6 +4,7 @@
use std::{
ffi::CStr,
+ pin::Pin,
ptr::{addr_of_mut, null_mut, NonNull},
slice::from_ref,
};
@@ -184,7 +185,9 @@ impl HPETTimer {
fn init(&mut self, index: usize, state: &HPETState) {
*self = HPETTimer {
index,
- qemu_timer: Timer::new(),
+ // SAFETY: the HPETTimer will only be used after the timer
+ // is initialized below.
+ qemu_timer: unsafe { Timer::new() },
state: NonNull::new(state as *const _ as *mut _).unwrap(),
config: 0,
cmp: 0,
@@ -195,7 +198,10 @@ fn init(&mut self, index: usize, state: &HPETState) {
last: 0,
};
- self.qemu_timer.init_full(
+ // SAFETY: HPETTimer is only used as part of HPETState, which is
+ // always pinned.
+ let qemu_timer = unsafe { Pin::new_unchecked(&mut self.qemu_timer) };
+ qemu_timer.init_full(
None,
CLOCK_VIRTUAL,
Timer::NS,
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..f0b04ef95d7 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -2,31 +2,51 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::os::raw::{c_int, c_void};
+use std::{
+ os::raw::{c_int, c_void},
+ pin::Pin,
+};
use crate::{
bindings::{self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType},
callbacks::FnCall,
+ cell::Opaque,
};
-pub type Timer = bindings::QEMUTimer;
-pub type TimerListGroup = bindings::QEMUTimerListGroup;
+/// A safe wrapper around [`bindings::QEMUTimer`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Timer(Opaque<bindings::QEMUTimer>);
+
+unsafe impl Send for Timer {}
+unsafe impl Sync for Timer {}
+
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct TimerListGroup(Opaque<bindings::QEMUTimerListGroup>);
+
+unsafe impl Send for TimerListGroup {}
+unsafe impl Sync for TimerListGroup {}
impl Timer {
pub const MS: u32 = bindings::SCALE_MS;
pub const US: u32 = bindings::SCALE_US;
pub const NS: u32 = bindings::SCALE_NS;
- pub fn new() -> Self {
- Default::default()
- }
-
- const fn as_mut_ptr(&self) -> *mut Self {
- self as *const Timer as *mut _
+ /// Create a `Timer` struct without initializing it.
+ ///
+ /// # Safety
+ ///
+ /// The timer must be initialized before it is armed with
+ /// [`modify`](Self::modify).
+ pub unsafe fn new() -> Self {
+ // SAFETY: requirements relayed to callers of Timer::new
+ Self(unsafe { Opaque::zeroed() })
}
+ /// Create a new timer with the given attributes.
pub fn init_full<'timer, 'opaque: 'timer, T, F>(
- &'timer mut self,
+ self: Pin<&'timer mut Self>,
timer_list_group: Option<&TimerListGroup>,
clk_type: ClockType,
scale: u32,
@@ -51,7 +71,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
- self,
+ self.as_mut_ptr(),
if let Some(g) = timer_list_group {
g as *const TimerListGroup as *mut _
} else {
@@ -67,14 +87,19 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
}
pub fn modify(&self, expire_time: u64) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
pub fn delete(&self) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_del(self.as_mut_ptr()) }
}
}
+// FIXME: use something like PinnedDrop from the pinned_init crate
impl Drop for Timer {
fn drop(&mut self) {
self.delete()
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-03 15:58 ` Paolo Bonzini
@ 2025-03-04 9:13 ` Zhao Liu
2025-03-06 10:45 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-03-04 9:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Mon, Mar 03, 2025 at 04:58:57PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 16:58:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
>
> On 3/3/25 14:48, Zhao Liu wrote:
> > > @@ -156,7 +157,7 @@ pub struct HPETTimer {
> > > /// timer N index within the timer block (`HPETState`)
> > > #[doc(alias = "tn")]
> > > index: usize,
> > > - qemu_timer: Option<Box<Timer>>,
> > > + qemu_timer: Option<Pin<Box<Timer>>>,
> >
> > I'm removing this Option<> wrapper in migration series. This is because
> > Option<> can't be treated as pointer as you mentioned in [*].
> >
> > So for this reason, does this mean that VMStateField cannot accept
> > Option<>? I realize that all the current VMStateFlags don't seem
> > compatible with Option<> unless a new flag is introduced.
> >
> > [*]: https://lore.kernel.org/qemu-devel/9a0389fa-765c-443b-ac2f-7c99ed862982@redhat.com/
>
> Ok, so let's get rid of the option. I didn't really like it anyway...
>
> If the Timer is embedded in the HPETTimer, there needs to be some
> "unsafe" in order to make sure that the pinning is observed, and also
> because an uninitialized Timer is bad and can cause a NULL pointer
> dereference in modify()... i.e. Timer shouldn't have implemented
> Default!
Yes! Good point.
> However, the lifetime checks in init_full() are preserved, so overall
> this is better---at least for now. Linux also had unsafe initialization
> for quite some time, so I'm okay with it.
The overall design is okay for me too.
> The replacements for this patch are below.
I have comments about current Opaque<> implemation below...
> From 2d74bdf176b2fbeb6205396d0021f68a9e72bde1 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Mon, 3 Mar 2025 16:27:08 +0100
> Subject: [PATCH 1/2] rust: hpet: embed Timer without the Option and Box
> indirection
>
> This simplifies things for migration, since Option<Box<QEMUTimer>> does not
> implement VMState.
>
> This also shows a soundness issue because Timer::new() will leave a NULL
> timer list pointer, which can then be dereferenced by Timer::modify(). It
> will be fixed shortly.
Good catch!
> Suggested-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 31 deletions(-)
Thanks. This cleanup is fine for me!
...
> From 276020645786b6537c50bb37795f281b5d630f27 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Fri, 14 Feb 2025 12:06:13 +0100
> Subject: [PATCH 2/2] rust: timer: wrap QEMUTimer with Opaque<> and express
> pinning requirements
>
> Timers must be pinned in memory, because modify() stores a pointer to them
> in the TimerList. To express this requirement, change init_full() to take
> a pinned reference. Because the only way to obtain a Timer is through
> Timer::new(), which is unsafe, modify() can assume that the timer it got
> was later initialized; and because the initialization takes a Pin<&mut
> Timer> modify() can assume that the timer is pinned. In the future the
> pinning requirement will be expressed through the pin_init crate instead.
>
> Note that Timer is a bit different from other users of Opaque, in that
> it is created in Rust code rather than C code. This is why it has to
> use the unsafe constructors provided by Opaque; and in fact Timer::new()
> is also unsafe, because it leaves it to the caller to invoke init_full()
> before modify(). Without a call to init_full(), modify() will cause a
> NULL pointer dereference.
>
> An alternative could be to combine new() + init_full() by returning a
> pinned box; however, using a reference makes it easier to express
> the requirement that the opaque outlives the timer.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> meson.build | 7 -----
> rust/hw/timer/hpet/src/hpet.rs | 10 ++++++--
> rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++++++--------
> 3 files changed, 44 insertions(+), 20 deletions(-)
...
> impl Timer {
> pub const MS: u32 = bindings::SCALE_MS;
> pub const US: u32 = bindings::SCALE_US;
> pub const NS: u32 = bindings::SCALE_NS;
> - pub fn new() -> Self {
> - Default::default()
> - }
> -
> - const fn as_mut_ptr(&self) -> *mut Self {
> - self as *const Timer as *mut _
> + /// Create a `Timer` struct without initializing it.
> + ///
> + /// # Safety
> + ///
> + /// The timer must be initialized before it is armed with
> + /// [`modify`](Self::modify).
> + pub unsafe fn new() -> Self {
> + // SAFETY: requirements relayed to callers of Timer::new
> + Self(unsafe { Opaque::zeroed() })
We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
marks the timer as initialized, which disables MaybeUninit's ability to check for
initialization. e.g.,
// No compiling error or runtime panic
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
let _t = unsafe { t.assume_init() };
Further more, I spent some time trying to figure out if MaybeUninit in
Opaque<> could help identify UB caused by uninitialized Timer, but I found
it doesn't work. :-(
There're 2 cases:
// No compiling error or runtime panic
let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
let _v = unsafe { v.get_mut().assume_init() };
// Runtime panic: Illegal instruction
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::uninit();
let _t = unsafe { t.assume_init() };
I understand that the outer UnsafeCell wrapper makes MaybeUninit's
checks not work.
But when I adjust MaybeUninit as the outer wrapper, the UB check can
work:
// Runtime panic: Illegal instruction
let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
let _v = unsafe { v.assume_init() };
And there's another example:
https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
Compared with linux's Opaque, it also puts MaybeUninit on the outermost
layer.
Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
mutability is expected... but this layout breaks MaybeUninit's functionality.
> + /// Create a new timer with the given attributes.
> pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> - &'timer mut self,
> + self: Pin<&'timer mut Self>,
> timer_list_group: Option<&TimerListGroup>,
> clk_type: ClockType,
> scale: u32,
> @@ -51,7 +71,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> // SAFETY: the opaque outlives the timer
> unsafe {
> timer_init_full(
> - self,
> + self.as_mut_ptr(),
> if let Some(g) = timer_list_group {
> g as *const TimerListGroup as *mut _
> } else {
> @@ -67,14 +87,19 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> }
> pub fn modify(&self, expire_time: u64) {
> + // SAFETY: the only way to obtain a Timer safely is via methods that
> + // take a Pin<&mut Self>, therefore the timer is pinned
The SAFETY should also be ensured by MaybeUninit, I think. But I haven't
verified if MaybeUninit<UnsafeCell<>> can work on FFI case...
> unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
> }
> pub fn delete(&self) {
> + // SAFETY: the only way to obtain a Timer safely is via methods that
> + // take a Pin<&mut Self>, therefore the timer is pinned
> unsafe { timer_del(self.as_mut_ptr()) }
> }
> }
> +// FIXME: use something like PinnedDrop from the pinned_init crate
> impl Drop for Timer {
> fn drop(&mut self) {
> self.delete()
> --
> 2.48.1
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-04 9:13 ` Zhao Liu
@ 2025-03-06 10:45 ` Paolo Bonzini
2025-03-06 11:35 ` Zhao Liu
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-03-06 10:45 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 3/4/25 10:13, Zhao Liu wrote:
>> - const fn as_mut_ptr(&self) -> *mut Self {
>> - self as *const Timer as *mut _
>> + /// Create a `Timer` struct without initializing it.
>> + ///
>> + /// # Safety
>> + ///
>> + /// The timer must be initialized before it is armed with
>> + /// [`modify`](Self::modify).
>> + pub unsafe fn new() -> Self {
>> + // SAFETY: requirements relayed to callers of Timer::new
>> + Self(unsafe { Opaque::zeroed() })
>
> We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
> marks the timer as initialized, which disables MaybeUninit's ability to check for
> initialization. e.g.,
While neither is good, a zeroed area of memory behaves better than an
uninitialized one... In particular, Drop calls timer_del() which works
fine with a zeroed QEMUTimer. With Opaque::uninit() you could have a
crash just with
drop(Timer::new());
> // No compiling error or runtime panic
> let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
> let _t = unsafe { t.assume_init() };
>
> Further more, I spent some time trying to figure out if MaybeUninit in
> Opaque<> could help identify UB caused by uninitialized Timer, but I found
> it doesn't work. :-(
>
> // No compiling error or runtime panic
> let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
> let _v = unsafe { v.get_mut().assume_init() };
>
> But when I adjust MaybeUninit as the outer wrapper, the UB check can
> work:
>
> // Runtime panic: Illegal instruction
> let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
> let _v = unsafe { v.assume_init() };
>
> Compared with linux's Opaque, it also puts MaybeUninit on the outermost
> layer.
Yes, I admit I just copied what Linux does. :)
> And there's another example:
>
> https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
>
> Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
> mutability is expected... but this layout breaks MaybeUninit's functionality.
Thanks for the example from the documentation! Indeed it should be
possible to do
/// Returns a raw mutable pointer to the opaque data.
pub const fn as_mut_ptr(&self) -> *mut T {
UnsafeCell::raw_get(self.value.as_ptr())
}
/// Returns a raw pointer to the opaque data that can be passed to a
/// C function as `void *`.
pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
self.as_mut_ptr().cast()
}
pub const fn raw_get(slot: *const Self) -> *mut T {
// SAFETY: even if uninitialized, slot points to a MaybeUninit
let slot = slot.cast::<MaybeUninit<UnsafeCell<T>>>;
UnsafeCell::raw_get(slot.as_ptr())
}
if Opaque<> uses a MaybeUninit<UnsafeCell<T>>. I'm a bit worried of
deviating from what Linux does though...
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-06 10:45 ` Paolo Bonzini
@ 2025-03-06 11:35 ` Zhao Liu
0 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-06 11:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> While neither is good, a zeroed area of memory behaves better than an
> uninitialized one... In particular, Drop calls timer_del() which works fine
> with a zeroed QEMUTimer. With Opaque::uninit() you could have a crash just
> with
>
> drop(Timer::new());
Good point.
> > // No compiling error or runtime panic
> > let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
> > let _t = unsafe { t.assume_init() };
> >
> > Further more, I spent some time trying to figure out if MaybeUninit in
> > Opaque<> could help identify UB caused by uninitialized Timer, but I found
> > it doesn't work. :-(
> >
> > // No compiling error or runtime panic
> > let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
> > let _v = unsafe { v.get_mut().assume_init() };
> >
> > But when I adjust MaybeUninit as the outer wrapper, the UB check can
> > work:
> >
> > // Runtime panic: Illegal instruction
> > let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
> > let _v = unsafe { v.assume_init() };
> >
> > Compared with linux's Opaque, it also puts MaybeUninit on the outermost
> > layer.
>
> Yes, I admit I just copied what Linux does. :)
Thanks for pointing this! I realized I referred the old code, since this
commit, linux puts the UnsafeCell to the outer layer [2]
[2]: https://github.com/torvalds/linux/commit/35cad617df2eeef8440a38e82bb2d81ae32ca50d
It seems that, at least from the Linux view, here the role of MaybeUninit
(as the cases I tested) is not a main concern, and Rust convention is
superior...
> > And there's another example:
> >
> > https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
> >
> > Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
> > mutability is expected... but this layout breaks MaybeUninit's functionality.
>
> Thanks for the example from the documentation! Indeed it should be possible
> to do
>
> /// Returns a raw mutable pointer to the opaque data.
> pub const fn as_mut_ptr(&self) -> *mut T {
> UnsafeCell::raw_get(self.value.as_ptr())
> }
>
> /// Returns a raw pointer to the opaque data that can be passed to a
> /// C function as `void *`.
> pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
> self.as_mut_ptr().cast()
> }
>
> pub const fn raw_get(slot: *const Self) -> *mut T {
> // SAFETY: even if uninitialized, slot points to a MaybeUninit
> let slot = slot.cast::<MaybeUninit<UnsafeCell<T>>>;
> UnsafeCell::raw_get(slot.as_ptr())
> }
>
> if Opaque<> uses a MaybeUninit<UnsafeCell<T>>. I'm a bit worried of
> deviating from what Linux does though...
Thank you, this convertion to UnsafeCell<MaybeUninit<T>> in Linux
history convinces me... I also agree that we should follow it for now :-).
Regards,
Zhao
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-02-27 14:22 ` [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
2025-03-03 13:48 ` Zhao Liu
@ 2025-03-03 14:28 ` Zhao Liu
2025-03-03 14:51 ` Paolo Bonzini
1 sibling, 1 reply; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 14:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> - pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> - &'timer mut self,
> + pub fn new_full<'opaque, T, F>(
> timer_list_group: Option<&TimerListGroup>,
> clk_type: ClockType,
> scale: u32,
> attributes: u32,
> _cb: F,
> opaque: &'opaque T,
> - ) where
> + ) -> Pin<Box<Self>>
> F: for<'a> FnCall<(&'a T,)>,
> {
Ah, the lifetime here isn't effectively bound... However, I also
referred to your latest code [1] :), and it seems that this issue
has already been fixed. (Nit: The code still has a complaint from
`cargo fmt`)
[1]: https://gitlab.com/bonzini/qemu/-/commit/ccb9f6dc738f503a696d8d50f1b5e4576ee80bc6
Regards,
Zhao
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-03 14:28 ` Zhao Liu
@ 2025-03-03 14:51 ` Paolo Bonzini
2025-03-03 16:15 ` Zhao Liu
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-03-03 14:51 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 3/3/25 15:28, Zhao Liu wrote:
>> - pub fn init_full<'timer, 'opaque: 'timer, T, F>(
>> - &'timer mut self,
>> + pub fn new_full<'opaque, T, F>(
>> timer_list_group: Option<&TimerListGroup>,
>> clk_type: ClockType,
>> scale: u32,
>> attributes: u32,
>> _cb: F,
>> opaque: &'opaque T,
>> - ) where
>> + ) -> Pin<Box<Self>>
>> F: for<'a> FnCall<(&'a T,)>,
>> {
>
> Ah, the lifetime here isn't effectively bound... However, I also
> referred to your latest code [1] :), and it seems that this issue
> has already been fixed. (Nit: The code still has a complaint from
> `cargo fmt`)
I am not sure if the change I have in that commit actually does
anything, unfortunately... :( which is why I wanted to use init_full
instead of new_full.
It's easiest to marked new_full() unsafe for now.
Paolo
> [1]: https://gitlab.com/bonzini/qemu/-/commit/ccb9f6dc738f503a696d8d50f1b5e4576ee80bc6
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements
2025-03-03 14:51 ` Paolo Bonzini
@ 2025-03-03 16:15 ` Zhao Liu
0 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 16:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Mon, Mar 03, 2025 at 03:51:25PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 15:51:25 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
>
> On 3/3/25 15:28, Zhao Liu wrote:
> > > - pub fn init_full<'timer, 'opaque: 'timer, T, F>(
> > > - &'timer mut self,
> > > + pub fn new_full<'opaque, T, F>(
> > > timer_list_group: Option<&TimerListGroup>,
> > > clk_type: ClockType,
> > > scale: u32,
> > > attributes: u32,
> > > _cb: F,
> > > opaque: &'opaque T,
> > > - ) where
> > > + ) -> Pin<Box<Self>>
> > > F: for<'a> FnCall<(&'a T,)>,
> > > {
> >
> > Ah, the lifetime here isn't effectively bound... However, I also
> > referred to your latest code [1] :), and it seems that this issue
> > has already been fixed. (Nit: The code still has a complaint from
> > `cargo fmt`)
>
> I am not sure if the change I have in that commit actually does anything,
> unfortunately... :( which is why I wanted to use init_full instead of
> new_full.
EMM, I tried with the below case...
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index a440c9f4cb98..c167d69eef4c 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -190,14 +190,17 @@ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
}
fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Timer::new_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- ));
+ {
+ let tmp = &self.get_state().timers[self.index];
+ self.qemu_timer = Some(Timer::new_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ tmp,
+ ));
+ }
}
fn get_state(&self) -> &HPETState {
---
It can compile and seems lifetime doesn't work... I think if we want the
lifetime check, it would be necessary to store &opaque in Rust's Timer
wrapper and specify a lifetime for Timer.
Maybe we need something like (similar to MemoryRegionOps):
pub struct Timer<'timer, T> {
Opaque<bindings::QEMUTimer>,
PhantomData<&'timer T>,
}
> It's easiest to marked new_full() unsafe for now.
Yes, I agree.
Thanks,
Zhao
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 05/12] rust: irq: wrap IRQState with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (3 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and express pinning requirements Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 15:07 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 06/12] rust: qom: wrap Object " Paolo Bonzini
` (6 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/irq.rs | 15 ++++++++++-----
rust/qemu-api/src/sysbus.rs | 1 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 34c19263c23..1222d4fde30 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -8,10 +8,16 @@
use crate::{
bindings::{self, qemu_set_irq},
+ cell::Opaque,
prelude::*,
qom::ObjectClass,
};
+/// An opaque wrapper around [`bindings::IRQState`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct IRQState(Opaque<bindings::IRQState>);
+
/// Interrupt sources are used by devices to pass changes to a value (typically
/// a boolean). The interrupt sink is usually an interrupt controller or
/// GPIO controller.
@@ -21,8 +27,7 @@
/// method sends a `true` value to the sink. If the guest has to see a
/// different polarity, that change is performed by the board between the
/// device and the interrupt controller.
-pub type IRQState = bindings::IRQState;
-
+///
/// Interrupts are implemented as a pointer to the interrupt "sink", which has
/// type [`IRQState`]. A device exposes its source as a QOM link property using
/// a function such as [`SysBusDeviceMethods::init_irq`], and
@@ -40,7 +45,7 @@ pub struct InterruptSource<T = bool>
where
c_int: From<T>,
{
- cell: BqlCell<*mut IRQState>,
+ cell: BqlCell<*mut bindings::IRQState>,
_marker: PhantomData<T>,
}
@@ -79,11 +84,11 @@ pub fn set(&self, level: T) {
}
}
- pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState {
+ pub(crate) const fn as_ptr(&self) -> *mut *mut bindings::IRQState {
self.cell.as_ptr()
}
- pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState {
+ pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut bindings::IRQState {
assert!(!slice.is_empty());
slice[0].as_ptr()
}
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 04821a2b9b3..48803a655f9 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -79,6 +79,7 @@ fn mmio_map(&self, id: u32, addr: u64) {
fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) {
assert!(bql_locked());
let id: i32 = id.try_into().unwrap();
+ let irq: &IRQState = irq;
unsafe {
bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr());
}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 05/12] rust: irq: wrap IRQState with Opaque<>
2025-02-27 14:22 ` [PATCH 05/12] rust: irq: wrap IRQState with Opaque<> Paolo Bonzini
@ 2025-03-03 15:07 ` Zhao Liu
0 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 15:07 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Thu, Feb 27, 2025 at 03:22:12PM +0100, Paolo Bonzini wrote:
> Date: Thu, 27 Feb 2025 15:22:12 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 05/12] rust: irq: wrap IRQState with Opaque<>
> X-Mailer: git-send-email 2.48.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/irq.rs | 15 ++++++++++-----
> rust/qemu-api/src/sysbus.rs | 1 +
> 2 files changed, 11 insertions(+), 5 deletions(-)
...
> +///
> /// Interrupts are implemented as a pointer to the interrupt "sink", which has
> /// type [`IRQState`]. A device exposes its source as a QOM link property using
> /// a function such as [`SysBusDeviceMethods::init_irq`], and
> @@ -40,7 +45,7 @@ pub struct InterruptSource<T = bool>
> where
> c_int: From<T>,
> {
> - cell: BqlCell<*mut IRQState>,
> + cell: BqlCell<*mut bindings::IRQState>,
> _marker: PhantomData<T>,
> }
Thanks! Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 06/12] rust: qom: wrap Object with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (4 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 05/12] rust: irq: wrap IRQState with Opaque<> Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-02-27 14:22 ` [PATCH 07/12] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
` (5 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/memory.rs | 2 +-
rust/qemu-api/src/qdev.rs | 6 +++---
rust/qemu-api/src/qom.rs | 35 ++++++++++++++++++++++-------------
4 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index d2868639ff6..be6dd68c09c 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -46,9 +46,6 @@ unsafe impl Sync for MemoryRegion {}
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
-unsafe impl Send for Object {}
-unsafe impl Sync for Object {}
-
unsafe impl Send for SysBusDevice {}
unsafe impl Sync for SysBusDevice {}
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 682951ab44e..713c494ca2e 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -157,7 +157,7 @@ unsafe fn do_init_io(
let cstr = CString::new(name).unwrap();
memory_region_init_io(
slot,
- owner.cast::<Object>(),
+ owner.cast::<bindings::Object>(),
ops,
owner.cast::<c_void>(),
cstr.as_ptr(),
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index c136457090c..1a4d1f38762 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -52,7 +52,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_enter_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
@@ -65,7 +65,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_hold_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
@@ -78,7 +78,7 @@ pub trait ResettablePhasesImpl {
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_resettable_exit_fn<T: ResettablePhasesImpl>(
- obj: *mut Object,
+ obj: *mut bindings::Object,
typ: ResetType,
) {
let state = NonNull::new(obj).unwrap().cast::<T>();
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 5488643a2fd..2defbd23516 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -101,16 +101,24 @@
ptr::NonNull,
};
-pub use bindings::{Object, ObjectClass};
+pub use bindings::ObjectClass;
use crate::{
bindings::{
self, object_class_dynamic_cast, object_dynamic_cast, object_get_class,
object_get_typename, object_new, object_ref, object_unref, TypeInfo,
},
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
};
+/// A safe wrapper around [`bindings::Object`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Object(Opaque<bindings::Object>);
+
+unsafe impl Send for Object {}
+unsafe impl Sync for Object {}
+
/// Marker trait: `Self` can be statically upcasted to `P` (i.e. `P` is a direct
/// or indirect parent of `Self`).
///
@@ -199,7 +207,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
}
}
-unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut bindings::Object) {
let mut state = NonNull::new(obj).unwrap().cast::<T>();
// SAFETY: obj is an instance of T, since rust_instance_init<T>
// is called from QOM core as the instance_init function
@@ -209,7 +217,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
}
}
-unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut bindings::Object) {
let state = NonNull::new(obj).unwrap().cast::<T>();
// SAFETY: obj is an instance of T, since rust_instance_post_init<T>
// is called from QOM core as the instance_post_init function
@@ -230,7 +238,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
<T as ObjectImpl>::CLASS_INIT(unsafe { klass.as_mut() })
}
-unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
+unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut bindings::Object) {
// SAFETY: obj is an instance of T, since drop_object<T> is called
// from the QOM core function object_deinit() as the instance_finalize
// function for class T. Note that while object_deinit() will drop the
@@ -280,14 +288,14 @@ pub unsafe trait ObjectType: Sized {
/// Return the receiver as an Object. This is always safe, even
/// if this type represents an interface.
fn as_object(&self) -> &Object {
- unsafe { &*self.as_object_ptr() }
+ unsafe { &*self.as_ptr().cast() }
}
/// Return the receiver as a const raw pointer to Object.
/// This is preferrable to `as_object_mut_ptr()` if a C
/// function only needs a `const Object *`.
- fn as_object_ptr(&self) -> *const Object {
- self.as_ptr().cast()
+ fn as_object_ptr(&self) -> *const bindings::Object {
+ self.as_object().as_ptr()
}
/// Return the receiver as a mutable raw pointer to Object.
@@ -297,8 +305,8 @@ fn as_object_ptr(&self) -> *const Object {
/// This cast is always safe, but because the result is mutable
/// and the incoming reference is not, this should only be used
/// for calls to C functions, and only if needed.
- unsafe fn as_object_mut_ptr(&self) -> *mut Object {
- self.as_object_ptr() as *mut _
+ unsafe fn as_object_mut_ptr(&self) -> *mut bindings::Object {
+ self.as_object().as_mut_ptr()
}
}
@@ -621,7 +629,7 @@ pub trait ObjectImpl: ObjectType + IsA<Object> {
/// We expect the FFI user of this function to pass a valid pointer that
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
-unsafe extern "C" fn rust_unparent_fn<T: ObjectImpl>(dev: *mut Object) {
+unsafe extern "C" fn rust_unparent_fn<T: ObjectImpl>(dev: *mut bindings::Object) {
let state = NonNull::new(dev).unwrap().cast::<T>();
T::UNPARENT.unwrap()(unsafe { state.as_ref() });
}
@@ -796,8 +804,9 @@ fn new() -> Owned<Self> {
// SAFETY: the object created by object_new is allocated on
// the heap and has a reference count of 1
unsafe {
- let obj = &*object_new(Self::TYPE_NAME.as_ptr());
- Owned::from_raw(obj.unsafe_cast::<Self>())
+ let raw_obj = object_new(Self::TYPE_NAME.as_ptr());
+ let obj = Object::from_raw(raw_obj).unsafe_cast::<Self>();
+ Owned::from_raw(obj)
}
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 07/12] rust: qdev: wrap Clock and DeviceState with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (5 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 06/12] rust: qom: wrap Object " Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-02-27 14:22 ` [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice Paolo Bonzini
` (4 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 6 ----
rust/qemu-api/src/qdev.rs | 68 ++++++++++++++++++++++++-----------
rust/qemu-api/src/vmstate.rs | 2 +-
3 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index be6dd68c09c..6e70a75a0e6 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -34,12 +34,6 @@ unsafe impl Sync for CharBackend {}
unsafe impl Send for Chardev {}
unsafe impl Sync for Chardev {}
-unsafe impl Send for Clock {}
-unsafe impl Sync for Clock {}
-
-unsafe impl Send for DeviceState {}
-unsafe impl Sync for DeviceState {}
-
unsafe impl Send for MemoryRegion {}
unsafe impl Sync for MemoryRegion {}
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 1a4d1f38762..1c4a67b5728 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -10,12 +10,12 @@
ptr::NonNull,
};
-pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property, ResetType};
+pub use bindings::{ClockEvent, DeviceClass, Property, ResetType};
use crate::{
bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error, ResettableClass},
callbacks::FnCall,
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
chardev::Chardev,
irq::InterruptSource,
prelude::*,
@@ -23,6 +23,22 @@
vmstate::VMStateDescription,
};
+/// A safe wrapper around [`bindings::Clock`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Clock(Opaque<bindings::Clock>);
+
+unsafe impl Send for Clock {}
+unsafe impl Sync for Clock {}
+
+/// A safe wrapper around [`bindings::DeviceState`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct DeviceState(Opaque<bindings::DeviceState>);
+
+unsafe impl Send for DeviceState {}
+unsafe impl Sync for DeviceState {}
+
/// Trait providing the contents of the `ResettablePhases` struct,
/// which is part of the QOM `Resettable` interface.
pub trait ResettablePhasesImpl {
@@ -117,7 +133,10 @@ fn vmsd() -> Option<&'static VMStateDescription> {
/// We expect the FFI user of this function to pass a valid pointer that
/// can be downcasted to type `T`. We also expect the device is
/// readable/writeable from one thread at any time.
-unsafe extern "C" fn rust_realize_fn<T: DeviceImpl>(dev: *mut DeviceState, _errp: *mut *mut Error) {
+unsafe extern "C" fn rust_realize_fn<T: DeviceImpl>(
+ dev: *mut bindings::DeviceState,
+ _errp: *mut *mut Error,
+) {
let state = NonNull::new(dev).unwrap().cast::<T>();
T::REALIZE.unwrap()(unsafe { state.as_ref() });
}
@@ -251,7 +270,7 @@ fn init_clock_in<F: for<'a> FnCall<(&'a Self::Target, ClockEvent)>>(
events: ClockEvent,
) -> Owned<Clock> {
fn do_init_clock_in(
- dev: *mut DeviceState,
+ dev: &DeviceState,
name: &str,
cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)>,
events: ClockEvent,
@@ -265,14 +284,15 @@ fn do_init_clock_in(
unsafe {
let cstr = CString::new(name).unwrap();
let clk = bindings::qdev_init_clock_in(
- dev,
+ dev.as_mut_ptr(),
cstr.as_ptr(),
cb,
- dev.cast::<c_void>(),
+ dev.as_void_ptr(),
events.0,
);
- Owned::from(&*clk)
+ let clk: &Clock = Clock::from_raw(clk);
+ Owned::from(clk)
}
}
@@ -289,7 +309,7 @@ fn do_init_clock_in(
None
};
- do_init_clock_in(self.as_mut_ptr(), name, cb, events)
+ do_init_clock_in(self.upcast(), name, cb, events)
}
/// Add an output clock named `name`.
@@ -304,9 +324,10 @@ fn do_init_clock_in(
fn init_clock_out(&self, name: &str) -> Owned<Clock> {
unsafe {
let cstr = CString::new(name).unwrap();
- let clk = bindings::qdev_init_clock_out(self.as_mut_ptr(), cstr.as_ptr());
+ let clk = bindings::qdev_init_clock_out(self.upcast().as_mut_ptr(), cstr.as_ptr());
- Owned::from(&*clk)
+ let clk: &Clock = Clock::from_raw(clk);
+ Owned::from(clk)
}
}
@@ -314,7 +335,11 @@ fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
assert!(bql_locked());
let c_propname = CString::new(propname).unwrap();
unsafe {
- bindings::qdev_prop_set_chr(self.as_mut_ptr(), c_propname.as_ptr(), chr.as_mut_ptr());
+ bindings::qdev_prop_set_chr(
+ self.upcast().as_mut_ptr(),
+ c_propname.as_ptr(),
+ chr.as_mut_ptr(),
+ );
}
}
@@ -323,8 +348,17 @@ fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(
num_lines: u32,
_cb: F,
) {
- let _: () = F::ASSERT_IS_SOME;
+ fn do_init_gpio_in(
+ dev: &DeviceState,
+ num_lines: u32,
+ gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int),
+ ) {
+ unsafe {
+ qdev_init_gpio_in(dev.as_mut_ptr(), Some(gpio_in_cb), num_lines as c_int);
+ }
+ }
+ let _: () = F::ASSERT_IS_SOME;
unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
opaque: *mut c_void,
line: c_int,
@@ -337,19 +371,13 @@ fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(
let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
rust_irq_handler::<Self::Target, F>;
- unsafe {
- qdev_init_gpio_in(
- self.as_mut_ptr::<DeviceState>(),
- Some(gpio_in_cb),
- num_lines as c_int,
- );
- }
+ do_init_gpio_in(self.upcast(), num_lines, gpio_in_cb);
}
fn init_gpio_out(&self, pins: &[InterruptSource]) {
unsafe {
qdev_init_gpio_out(
- self.as_mut_ptr::<DeviceState>(),
+ self.upcast().as_mut_ptr(),
InterruptSource::slice_as_ptr(pins),
pins.len() as c_int,
);
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 1e7ba531e2a..f0510ae769d 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -470,7 +470,7 @@ macro_rules! vmstate_clock {
$crate::assert_field_type!(
$struct_name,
$field_name,
- $crate::qom::Owned<$crate::bindings::Clock>
+ $crate::qom::Owned<$crate::qdev::Clock>
);
$crate::offset_of!($struct_name, $field_name)
},
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (6 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 07/12] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 15:09 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 09/12] rust: sysbus: wrap SysBusDevice with Opaque<> Paolo Bonzini
` (3 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Fields of SysBusDevice must only be accessed with the BQL taken. Add
a wrapper that verifies that.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 4 +---
rust/qemu-api/src/sysbus.rs | 12 ++++++++++++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index ce4b289d0c8..a440c9f4cb9 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -724,8 +724,6 @@ fn realize(&self) {
}
fn reset_hold(&self, _type: ResetType) {
- let sbd = self.upcast::<SysBusDevice>();
-
for timer in self.timers.iter().take(self.num_timers.get()) {
timer.borrow_mut().reset();
}
@@ -738,7 +736,7 @@ fn reset_hold(&self, _type: ResetType) {
HPETFwConfig::update_hpet_cfg(
self.hpet_id.get(),
self.capability.get() as u32,
- sbd.mmio[0].addr,
+ self.mmio_addr(0).unwrap(),
);
// to document that the RTC lowers its output on reset as well
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 48803a655f9..0790576d446 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -64,6 +64,18 @@ fn init_irq(&self, irq: &InterruptSource) {
}
}
+ // TODO: do we want a type like GuestAddress here?
+ fn mmio_addr(&self, id: u32) -> Option<u64> {
+ assert!(bql_locked());
+ let sbd = self.upcast();
+ let id: usize = id.try_into().unwrap();
+ if sbd.mmio[id].memory.is_null() {
+ None
+ } else {
+ Some(sbd.mmio[id].addr)
+ }
+ }
+
// TODO: do we want a type like GuestAddress here?
fn mmio_map(&self, id: u32, addr: u64) {
assert!(bql_locked());
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice
2025-02-27 14:22 ` [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice Paolo Bonzini
@ 2025-03-03 15:09 ` Zhao Liu
0 siblings, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 15:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Thu, Feb 27, 2025 at 03:22:15PM +0100, Paolo Bonzini wrote:
> Date: Thu, 27 Feb 2025 15:22:15 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice
> X-Mailer: git-send-email 2.48.1
>
> Fields of SysBusDevice must only be accessed with the BQL taken. Add
> a wrapper that verifies that.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/timer/hpet/src/hpet.rs | 4 +---
> rust/qemu-api/src/sysbus.rs | 12 ++++++++++++
> 2 files changed, 13 insertions(+), 3 deletions(-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 09/12] rust: sysbus: wrap SysBusDevice with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (7 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 08/12] rust: hpet: do not access fields of SysBusDevice Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 15:19 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 10/12] rust: memory: wrap MemoryRegion " Paolo Bonzini
` (2 subsequent siblings)
11 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/sysbus.rs | 29 +++++++++++++++++++++--------
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 6e70a75a0e6..b791ca6d87f 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -40,9 +40,6 @@ unsafe impl Sync for MemoryRegion {}
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
-unsafe impl Send for SysBusDevice {}
-unsafe impl Sync for SysBusDevice {}
-
// SAFETY: this is a pure data struct
unsafe impl Send for CoalescedMemoryRange {}
unsafe impl Sync for CoalescedMemoryRange {}
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 0790576d446..e92502a8fe6 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -6,11 +6,11 @@
use std::{ffi::CStr, ptr::addr_of_mut};
-pub use bindings::{SysBusDevice, SysBusDeviceClass};
+pub use bindings::SysBusDeviceClass;
use crate::{
bindings,
- cell::bql_locked,
+ cell::{bql_locked, Opaque},
irq::{IRQState, InterruptSource},
memory::MemoryRegion,
prelude::*,
@@ -18,6 +18,14 @@
qom::Owned,
};
+/// A safe wrapper around [`bindings::SysBusDevice`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct SysBusDevice(Opaque<bindings::SysBusDevice>);
+
+unsafe impl Send for SysBusDevice {}
+unsafe impl Sync for SysBusDevice {}
+
unsafe impl ObjectType for SysBusDevice {
type Class = SysBusDeviceClass;
const TYPE_NAME: &'static CStr =
@@ -49,7 +57,7 @@ pub trait SysBusDeviceMethods: ObjectDeref
fn init_mmio(&self, iomem: &MemoryRegion) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_mmio(self.as_mut_ptr(), iomem.as_mut_ptr());
+ bindings::sysbus_init_mmio(self.upcast().as_mut_ptr(), iomem.as_mut_ptr());
}
}
@@ -60,14 +68,16 @@ fn init_mmio(&self, iomem: &MemoryRegion) {
fn init_irq(&self, irq: &InterruptSource) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr());
+ bindings::sysbus_init_irq(self.upcast().as_mut_ptr(), irq.as_ptr());
}
}
// TODO: do we want a type like GuestAddress here?
fn mmio_addr(&self, id: u32) -> Option<u64> {
assert!(bql_locked());
- let sbd = self.upcast();
+ // SAFETY: the BQL ensures that no one else writes to sbd.mmio[], and
+ // the SysBusDevice must be initialized to get an IsA<SysBusDevice>.
+ let sbd = unsafe { *self.upcast().as_ptr() };
let id: usize = id.try_into().unwrap();
if sbd.mmio[id].memory.is_null() {
None
@@ -81,7 +91,7 @@ fn mmio_map(&self, id: u32, addr: u64) {
assert!(bql_locked());
let id: i32 = id.try_into().unwrap();
unsafe {
- bindings::sysbus_mmio_map(self.as_mut_ptr(), id, addr);
+ bindings::sysbus_mmio_map(self.upcast().as_mut_ptr(), id, addr);
}
}
@@ -93,7 +103,7 @@ fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) {
let id: i32 = id.try_into().unwrap();
let irq: &IRQState = irq;
unsafe {
- bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr());
+ bindings::sysbus_connect_irq(self.upcast().as_mut_ptr(), id, irq.as_mut_ptr());
}
}
@@ -101,7 +111,10 @@ fn sysbus_realize(&self) {
// TODO: return an Error
assert!(bql_locked());
unsafe {
- bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal));
+ bindings::sysbus_realize(
+ self.upcast().as_mut_ptr(),
+ addr_of_mut!(bindings::error_fatal),
+ );
}
}
}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 10/12] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (8 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 09/12] rust: sysbus: wrap SysBusDevice with Opaque<> Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-03-03 15:25 ` Zhao Liu
2025-03-05 7:09 ` Zhao Liu
2025-02-27 14:22 ` [PATCH 11/12] rust: chardev: wrap Chardev " Paolo Bonzini
2025-02-27 14:22 ` [PATCH 12/12] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
11 siblings, 2 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/memory.rs | 35 +++++++++++++++++++++--------------
2 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index b791ca6d87f..26cc8de0cf2 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -34,9 +34,6 @@ unsafe impl Sync for CharBackend {}
unsafe impl Send for Chardev {}
unsafe impl Sync for Chardev {}
-unsafe impl Send for MemoryRegion {}
-unsafe impl Sync for MemoryRegion {}
-
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 713c494ca2e..eff9f09fd7f 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -6,9 +6,8 @@
use std::{
ffi::{CStr, CString},
- marker::{PhantomData, PhantomPinned},
+ marker::PhantomData,
os::raw::{c_uint, c_void},
- ptr::addr_of,
};
pub use bindings::{hwaddr, MemTxAttrs};
@@ -16,6 +15,7 @@
use crate::{
bindings::{self, device_endian, memory_region_init_io},
callbacks::FnCall,
+ cell::Opaque,
prelude::*,
zeroable::Zeroable,
};
@@ -132,13 +132,13 @@ fn default() -> Self {
}
}
-/// A safe wrapper around [`bindings::MemoryRegion`]. Compared to the
-/// underlying C struct it is marked as pinned because the QOM tree
-/// contains a pointer to it.
-pub struct MemoryRegion {
- inner: bindings::MemoryRegion,
- _pin: PhantomPinned,
-}
+/// A safe wrapper around [`bindings::MemoryRegion`].
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct MemoryRegion(Opaque<bindings::MemoryRegion>);
+
+unsafe impl Send for MemoryRegion {}
+unsafe impl Sync for MemoryRegion {}
impl MemoryRegion {
// inline to ensure that it is not included in tests, which only
@@ -174,13 +174,20 @@ pub fn init_io<T: IsA<Object>>(
size: u64,
) {
unsafe {
- Self::do_init_io(&mut self.inner, owner.cast::<Object>(), &ops.0, name, size);
+ Self::do_init_io(
+ // self.0.as_mut_ptr() needed because Rust tries to call
+ // ObjectDeref::as_mut_ptr() on "&mut Self", instead of coercing
+ // to "&Self" and then calling MemoryRegion::as_mut_ptr().
+ // Revisit if/when ObjectCastMut is not needed anymore; it is
+ // only used in a couple places for initialization.
+ self.0.as_mut_ptr(),
+ owner.cast::<Object>(),
+ &ops.0,
+ name,
+ size,
+ );
}
}
-
- pub(crate) const fn as_mut_ptr(&self) -> *mut bindings::MemoryRegion {
- addr_of!(self.inner) as *mut _
- }
}
unsafe impl ObjectType for MemoryRegion {
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 10/12] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-27 14:22 ` [PATCH 10/12] rust: memory: wrap MemoryRegion " Paolo Bonzini
@ 2025-03-03 15:25 ` Zhao Liu
2025-03-05 7:09 ` Zhao Liu
1 sibling, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-03 15:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Thu, Feb 27, 2025 at 03:22:17PM +0100, Paolo Bonzini wrote:
> Date: Thu, 27 Feb 2025 15:22:17 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/12] rust: memory: wrap MemoryRegion with Opaque<>
> X-Mailer: git-send-email 2.48.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/bindings.rs | 3 ---
> rust/qemu-api/src/memory.rs | 35 +++++++++++++++++++++--------------
> 2 files changed, 21 insertions(+), 17 deletions(-)
...
> impl MemoryRegion {
> // inline to ensure that it is not included in tests, which only
> @@ -174,13 +174,20 @@ pub fn init_io<T: IsA<Object>>(
> size: u64,
> ) {
> unsafe {
> - Self::do_init_io(&mut self.inner, owner.cast::<Object>(), &ops.0, name, size);
> + Self::do_init_io(
> + // self.0.as_mut_ptr() needed because Rust tries to call
> + // ObjectDeref::as_mut_ptr() on "&mut Self", instead of coercing
> + // to "&Self" and then calling MemoryRegion::as_mut_ptr().
> + // Revisit if/when ObjectCastMut is not needed anymore; it is
> + // only used in a couple places for initialization.
> + self.0.as_mut_ptr(),
> + owner.cast::<Object>(),
> + &ops.0,
> + name,
> + size,
> + );
> }
> }
The extra comment is nice, too. :-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 10/12] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-27 14:22 ` [PATCH 10/12] rust: memory: wrap MemoryRegion " Paolo Bonzini
2025-03-03 15:25 ` Zhao Liu
@ 2025-03-05 7:09 ` Zhao Liu
1 sibling, 0 replies; 27+ messages in thread
From: Zhao Liu @ 2025-03-05 7:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Sorry, when I revisit this patch, I have more thoughts..
> -/// A safe wrapper around [`bindings::MemoryRegion`]. Compared to the
> -/// underlying C struct it is marked as pinned because the QOM tree
> -/// contains a pointer to it.
> -pub struct MemoryRegion {
> - inner: bindings::MemoryRegion,
> - _pin: PhantomPinned,
> -}
> +/// A safe wrapper around [`bindings::MemoryRegion`].
> +#[repr(transparent)]
> +#[derive(qemu_api_macros::Wrapper)]
> +pub struct MemoryRegion(Opaque<bindings::MemoryRegion>);
Would MemoryRegionOps also need to be wrapped into Opaque<>?
Currently I understand it's not necessary, because init_io() requires
MemoryRegionOps reference to be static.
pub fn init_io<T: IsA<Object>>(
&mut self,
owner: *mut T,
ops: &'static MemoryRegionOps<T>,
name: &'static str,
size: u64,
)
But I wonder whether it is the most ideal implementation... or to remove
the static limitation of MemoryRegionOps and consider pin+Opaque instead?
Another thought is for init_io, it's also necessary to ensure MemoryRegion
is pinned, because callbacks also need to access the pointer of MemoryRegion,
just like you did for timer, e.g.,
pub fn init_io<T: IsA<Object>>(
- &mut self,
+ self: Pin<&mut Self>,
owner: *mut T,
ops: &'static MemoryRegionOps<T>,
name: &'static str,
size: u64,
) {
(Just disscussion without clarifying the difficulty), if you agree with
this, then I think this would be a main pattern for callback, i.e.,
accepting a pin parameter. Perhaps an ideal option would be to be able
to add the pin limit to the FnCall.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 11/12] rust: chardev: wrap Chardev with Opaque<>
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (9 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 10/12] rust: memory: wrap MemoryRegion " Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
2025-02-27 14:22 ` [PATCH 12/12] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 3 ---
rust/qemu-api/src/chardev.rs | 8 ++++++--
rust/qemu-api/src/qdev.rs | 1 +
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 26cc8de0cf2..c3f36108bd5 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -31,9 +31,6 @@ unsafe impl Sync for BusState {}
unsafe impl Send for CharBackend {}
unsafe impl Sync for CharBackend {}
-unsafe impl Send for Chardev {}
-unsafe impl Sync for Chardev {}
-
unsafe impl Send for ObjectClass {}
unsafe impl Sync for ObjectClass {}
diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs
index 74cfb634e5f..a35b9217e90 100644
--- a/rust/qemu-api/src/chardev.rs
+++ b/rust/qemu-api/src/chardev.rs
@@ -6,9 +6,13 @@
use std::ffi::CStr;
-use crate::{bindings, prelude::*};
+use crate::{bindings, cell::Opaque, prelude::*};
+
+/// A safe wrapper around [`bindings::Chardev`].
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct Chardev(Opaque<bindings::Chardev>);
-pub type Chardev = bindings::Chardev;
pub type ChardevClass = bindings::ChardevClass;
unsafe impl ObjectType for Chardev {
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 1c4a67b5728..18b4a9ba687 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -334,6 +334,7 @@ fn init_clock_out(&self, name: &str) -> Owned<Clock> {
fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
assert!(bql_locked());
let c_propname = CString::new(propname).unwrap();
+ let chr: &Chardev = chr;
unsafe {
bindings::qdev_prop_set_chr(
self.upcast().as_mut_ptr(),
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 12/12] rust: bindings: remove more unnecessary Send/Sync impls
2025-02-27 14:22 [PATCH v2 00/12] rust: wrap all C types exposed through qemu_api Paolo Bonzini
` (10 preceding siblings ...)
2025-02-27 14:22 ` [PATCH 11/12] rust: chardev: wrap Chardev " Paolo Bonzini
@ 2025-02-27 14:22 ` Paolo Bonzini
11 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2025-02-27 14:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Send and Sync are now implemented on the opaque wrappers. Remove them
from the bindings module, unless the structs are pure data containers
and/or have no C functions defined on them.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index c3f36108bd5..3c1d297581e 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -25,15 +25,11 @@
// SAFETY: these are implemented in C; the bindings need to assert that the
// BQL is taken, either directly or via `BqlCell` and `BqlRefCell`.
-unsafe impl Send for BusState {}
-unsafe impl Sync for BusState {}
-
+// When bindings for character devices are introduced, this can be
+// moved to the Opaque<> wrapper in src/chardev.rs.
unsafe impl Send for CharBackend {}
unsafe impl Sync for CharBackend {}
-unsafe impl Send for ObjectClass {}
-unsafe impl Sync for ObjectClass {}
-
// SAFETY: this is a pure data struct
unsafe impl Send for CoalescedMemoryRange {}
unsafe impl Sync for CoalescedMemoryRange {}
--
2.48.1
^ permalink raw reply related [flat|nested] 27+ messages in thread