From 0b9d05e3c98fe168f3502ccc422b9171467314fa Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 10:18:34 +0100 Subject: rust: cell: add wrapper for FFI types 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 Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/cell.rs | 204 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 197 insertions(+), 7 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs index eae4e2c..2889abb 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`] and [`BqlRefCell`] 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` +//! ### `BqlCell` //! //! [`BqlCell`] 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` +//! ### `BqlRefCell` //! //! [`BqlRefCell`] 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`] 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::::uninit(); +//! qemu_struct_init(state.as_mut_ptr()); +//! } +//! ``` +//! +//! [`Opaque`] will usually be wrapped one level further, so that +//! bridge methods can be added to the wrapper: +//! +//! ```ignore +//! pub struct MyStruct(Opaque); +//! +//! impl MyStruct { +//! fn new() -> Pin> { +//! 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`] 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`] 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`] 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,122 @@ impl fmt::Display for BqlRefMut<'_, T> { (**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` 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` is `#[repr(transparent)]`, so that it matches the memory layout +/// of `T`. +#[repr(transparent)] +pub struct Opaque { + value: UnsafeCell>, + // PhantomPinned also allows multiple references to the `Opaque`, i.e. + // one `&mut Opaque` can coexist with a `&mut T` or any number of `&T`; + // see https://docs.rs/pinned-aliasable/latest/pinned_aliasable/. + _pin: PhantomPinned, +} + +impl Opaque { + /// 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::(); + // 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() + } + + /// Converts a raw pointer to the wrapped type. + pub const fn raw_get(slot: *mut Self) -> *mut T { + // Compare with Linux's raw_get method, which goes through an UnsafeCell + // because it takes a *const Self instead. + slot.cast() + } +} + +impl fmt::Debug for Opaque { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let mut name: String = "Opaque<".to_string(); + name += std::any::type_name::(); + name += ">"; + f.debug_tuple(&name).field(&self.as_ptr()).finish() + } +} + +impl Opaque { + /// 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, + } + } +} -- cgit v1.1 From f07a5674cf97b8473e5d06d7b1df9b51e97d553f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 11:13:53 +0100 Subject: rust: qemu_api_macros: add Wrapper derive macro 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 Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/cell.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs index 2889abb..448638e 100644 --- a/rust/qemu-api/src/cell.rs +++ b/rust/qemu-api/src/cell.rs @@ -1030,3 +1030,48 @@ impl Opaque { } } } + +/// 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, +/// } +/// +/// unsafe impl Wrapper for Example { +/// type Wrapped = String; +/// } +/// ``` +/// +/// # Safety +/// +/// `Self` must be a `#[repr(transparent)]` wrapper for the `Wrapped` type, +/// whether directly or indirectly. +/// +/// # Methods +/// +/// By convention, types that implement Wrapper also implement the following +/// methods: +/// +/// ```ignore +/// pub const unsafe fn from_raw<'a>(value: *mut Self::Wrapped) -> &'a Self; +/// pub const unsafe fn as_mut_ptr(&self) -> *mut Self::Wrapped; +/// pub const unsafe fn as_ptr(&self) -> *const Self::Wrapped; +/// pub const unsafe fn raw_get(slot: *mut Self) -> *const Self::Wrapped; +/// ``` +/// +/// They are not defined here to allow them to be `const`. +pub unsafe trait Wrapper { + type Wrapped; +} + +unsafe impl Wrapper for Opaque { + type Wrapped = T; +} -- cgit v1.1 From d7f5ae8b30cc9652a4ddcfeb52076f5aef6d78b6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 25 Feb 2025 10:28:56 +0100 Subject: rust: vmstate: add std::pin::Pin as transparent wrapper Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 24a4dc8..1e7ba53 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -330,6 +330,7 @@ macro_rules! impl_vmstate_transparent { impl_vmstate_transparent!(std::cell::Cell where T: VMState); impl_vmstate_transparent!(std::cell::UnsafeCell where T: VMState); +impl_vmstate_transparent!(std::pin::Pin where T: VMState); impl_vmstate_transparent!(crate::cell::BqlCell where T: VMState); impl_vmstate_transparent!(crate::cell::BqlRefCell where T: VMState); -- cgit v1.1 From a32b239699377f09bba08b2e8ae0d167c1488b1f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:06:13 +0100 Subject: 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 --- rust/qemu-api/src/timer.rs | 47 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 11 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs index a593538..f0b04ef 100644 --- a/rust/qemu-api/src/timer.rs +++ b/rust/qemu-api/src/timer.rs @@ -2,31 +2,51 @@ // Author(s): Zhao Liu // 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); + +unsafe impl Send for Timer {} +unsafe impl Sync for Timer {} + +#[repr(transparent)] +#[derive(qemu_api_macros::Wrapper)] +pub struct TimerListGroup(Opaque); + +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 @@ impl Timer { // 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 @@ impl Timer { } 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() -- cgit v1.1 From 9c9a6a889cb3589779b019a343892aa0e9bdb254 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:05:59 +0100 Subject: rust: irq: wrap IRQState with Opaque<> Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/irq.rs | 15 ++++++++++----- rust/qemu-api/src/sysbus.rs | 1 + 2 files changed, 11 insertions(+), 5 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs index 34c1926..1222d4f 100644 --- a/rust/qemu-api/src/irq.rs +++ b/rust/qemu-api/src/irq.rs @@ -8,10 +8,16 @@ use std::{ffi::CStr, marker::PhantomData, os::raw::c_int, ptr}; 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); + /// 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 @@ use crate::{ /// 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 where c_int: From, { - cell: BqlCell<*mut IRQState>, + cell: BqlCell<*mut bindings::IRQState>, _marker: PhantomData, } @@ -79,11 +84,11 @@ where } } - 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 04821a2..48803a6 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -79,6 +79,7 @@ where fn connect_irq(&self, id: u32, irq: &Owned) { 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()); } -- cgit v1.1 From 7fb4a99df17c8ae5f5e00d643042b9d95477a426 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:09:53 +0100 Subject: rust: qom: wrap Object with Opaque<> Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index d286863..be6dd68 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 682951a..713c494 100644 --- a/rust/qemu-api/src/memory.rs +++ b/rust/qemu-api/src/memory.rs @@ -157,7 +157,7 @@ impl MemoryRegion { let cstr = CString::new(name).unwrap(); memory_region_init_io( slot, - owner.cast::(), + owner.cast::(), ops, owner.cast::(), cstr.as_ptr(), diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs index c136457..1a4d1f3 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( - obj: *mut Object, + obj: *mut bindings::Object, typ: ResetType, ) { let state = NonNull::new(obj).unwrap().cast::(); @@ -65,7 +65,7 @@ unsafe extern "C" fn rust_resettable_enter_fn( /// 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( - obj: *mut Object, + obj: *mut bindings::Object, typ: ResetType, ) { let state = NonNull::new(obj).unwrap().cast::(); @@ -78,7 +78,7 @@ unsafe extern "C" fn rust_resettable_hold_fn( /// 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( - obj: *mut Object, + obj: *mut bindings::Object, typ: ResetType, ) { let state = NonNull::new(obj).unwrap().cast::(); diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 5488643..2defbd2 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -101,16 +101,24 @@ use std::{ 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); + +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 @@ impl fmt::Display for ParentField { } } -unsafe extern "C" fn rust_instance_init(obj: *mut Object) { +unsafe extern "C" fn rust_instance_init(obj: *mut bindings::Object) { let mut state = NonNull::new(obj).unwrap().cast::(); // SAFETY: obj is an instance of T, since rust_instance_init // is called from QOM core as the instance_init function @@ -209,7 +217,7 @@ unsafe extern "C" fn rust_instance_init(obj: *mut Object) { } } -unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { +unsafe extern "C" fn rust_instance_post_init(obj: *mut bindings::Object) { let state = NonNull::new(obj).unwrap().cast::(); // SAFETY: obj is an instance of T, since rust_instance_post_init // is called from QOM core as the instance_post_init function @@ -230,7 +238,7 @@ unsafe extern "C" fn rust_class_init( ::CLASS_INIT(unsafe { klass.as_mut() }) } -unsafe extern "C" fn drop_object(obj: *mut Object) { +unsafe extern "C" fn drop_object(obj: *mut bindings::Object) { // SAFETY: obj is an instance of T, since drop_object 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 @@ pub unsafe trait ObjectType: Sized { /// 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 { /// 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(dev: *mut Object) { +unsafe extern "C" fn rust_unparent_fn(dev: *mut bindings::Object) { let state = NonNull::new(dev).unwrap().cast::(); T::UNPARENT.unwrap()(unsafe { state.as_ref() }); } @@ -796,8 +804,9 @@ pub trait ObjectClassMethods: IsA { // 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::()) + let raw_obj = object_new(Self::TYPE_NAME.as_ptr()); + let obj = Object::from_raw(raw_obj).unsafe_cast::(); + Owned::from_raw(obj) } } } -- cgit v1.1 From fc22d650d54363b8f2bad56aea1dde773f600067 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 11:45:25 +0100 Subject: rust: qdev: wrap Clock and DeviceState with Opaque<> Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index be6dd68..6e70a75 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 1a4d1f3..1c4a67b 100644 --- a/rust/qemu-api/src/qdev.rs +++ b/rust/qemu-api/src/qdev.rs @@ -10,12 +10,12 @@ use std::{ 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 @@ use crate::{ vmstate::VMStateDescription, }; +/// A safe wrapper around [`bindings::Clock`]. +#[repr(transparent)] +#[derive(Debug, qemu_api_macros::Wrapper)] +pub struct Clock(Opaque); + +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); + +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 @@ pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl + IsA { /// 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(dev: *mut DeviceState, _errp: *mut *mut Error) { +unsafe extern "C" fn rust_realize_fn( + dev: *mut bindings::DeviceState, + _errp: *mut *mut Error, +) { let state = NonNull::new(dev).unwrap().cast::(); T::REALIZE.unwrap()(unsafe { state.as_ref() }); } @@ -251,7 +270,7 @@ where events: ClockEvent, ) -> Owned { fn do_init_clock_in( - dev: *mut DeviceState, + dev: &DeviceState, name: &str, cb: Option, events: ClockEvent, @@ -265,14 +284,15 @@ where 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::(), + dev.as_void_ptr(), events.0, ); - Owned::from(&*clk) + let clk: &Clock = Clock::from_raw(clk); + Owned::from(clk) } } @@ -289,7 +309,7 @@ where 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 @@ where fn init_clock_out(&self, name: &str) -> Owned { 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 @@ where 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 @@ where 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 FnCall<(&'a T, u32, u32)>>( opaque: *mut c_void, line: c_int, @@ -337,19 +371,13 @@ where let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) = rust_irq_handler::; - unsafe { - qdev_init_gpio_in( - self.as_mut_ptr::(), - 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::(), + 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 1e7ba53..f0510ae 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) }, -- cgit v1.1 From 09fda8f5dc925ba059aca539163d16796af6a299 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 25 Feb 2025 10:06:20 +0100 Subject: rust: hpet: do not access fields of SysBusDevice Fields of SysBusDevice must only be accessed with the BQL taken. Add a wrapper that verifies that. Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/sysbus.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs index 48803a6..0790576 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -65,6 +65,18 @@ where } // TODO: do we want a type like GuestAddress here? + fn mmio_addr(&self, id: u32) -> Option { + 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()); let id: i32 = id.try_into().unwrap(); -- cgit v1.1 From f4751c7a42b194eb4166c7f3f294bf89c3e23cd9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 11:45:36 +0100 Subject: rust: sysbus: wrap SysBusDevice with Opaque<> Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/bindings.rs | 3 --- rust/qemu-api/src/sysbus.rs | 29 +++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 11 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index 6e70a75..b791ca6 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 0790576..e92502a 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 @@ use crate::{ qom::Owned, }; +/// A safe wrapper around [`bindings::SysBusDevice`]. +#[repr(transparent)] +#[derive(Debug, qemu_api_macros::Wrapper)] +pub struct SysBusDevice(Opaque); + +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 @@ where 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 @@ where 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 { 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. + 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 @@ where 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 @@ where 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 @@ where // 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), + ); } } } -- cgit v1.1 From af0868cba33aaf327a49d642b6b0ad3ae3f01240 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:34:47 +0100 Subject: rust: memory: wrap MemoryRegion with Opaque<> Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/bindings.rs | 3 --- rust/qemu-api/src/memory.rs | 35 +++++++++++++++++++++-------------- 2 files changed, 21 insertions(+), 17 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index b791ca6..26cc8de 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 713c494..eff9f09 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 @@ pub use bindings::{hwaddr, MemTxAttrs}; use crate::{ bindings::{self, device_endian, memory_region_init_io}, callbacks::FnCall, + cell::Opaque, prelude::*, zeroable::Zeroable, }; @@ -132,13 +132,13 @@ impl Default for MemoryRegionOpsBuilder { } } -/// 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); + +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 @@ impl MemoryRegion { size: u64, ) { unsafe { - Self::do_init_io(&mut self.inner, owner.cast::(), &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::(), + &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 { -- cgit v1.1 From 48627510a7fed7a045358743e6b869a98931f85e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:34:47 +0100 Subject: rust: chardev: wrap Chardev with Opaque<> Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index 26cc8de..c3f3610 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 74cfb63..a35b921 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); -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 1c4a67b..18b4a9b 100644 --- a/rust/qemu-api/src/qdev.rs +++ b/rust/qemu-api/src/qdev.rs @@ -334,6 +334,7 @@ where fn prop_set_chr(&self, propname: &str, chr: &Owned) { 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(), -- cgit v1.1 From 2ad011d466697d69f7f9aa84662a6553049f6556 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:36:11 +0100 Subject: rust: bindings: remove more unnecessary Send/Sync impls 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 Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/bindings.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs index c3f3610..3c1d297 100644 --- a/rust/qemu-api/src/bindings.rs +++ b/rust/qemu-api/src/bindings.rs @@ -25,15 +25,11 @@ include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs")); // 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 {} -- cgit v1.1 From 2d0050cbe27fed5233561451e6de64af5ecb6571 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 12:41:14 +0100 Subject: rust: chardev: provide basic bindings to character devices Most of the character device API is pretty simple, with "0 or -errno" or "number of bytes or -errno" as the convention for return codes. Add safe wrappers for the API to the CharBackend bindgen-generated struct. The API is not complete, but it covers the parts that are used by the PL011 device, plus qemu_chr_fe_write which is needed to implement the standard library Write trait. Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/chardev.rs | 242 +++++++++++++++++++++++++++++++++++++++++- rust/qemu-api/src/zeroable.rs | 1 + 2 files changed, 241 insertions(+), 2 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs index a35b921..11e6c45 100644 --- a/rust/qemu-api/src/chardev.rs +++ b/rust/qemu-api/src/chardev.rs @@ -3,10 +3,28 @@ // SPDX-License-Identifier: GPL-2.0-or-later //! Bindings for character devices +//! +//! Character devices in QEMU can run under the big QEMU lock or in a separate +//! `GMainContext`. Here we only support the former, because the bindings +//! enforce that the BQL is taken whenever the functions in [`CharBackend`] are +//! called. -use std::ffi::CStr; +use std::{ + ffi::CStr, + fmt::{self, Debug}, + io::{self, ErrorKind, Write}, + marker::PhantomPinned, + os::raw::{c_int, c_void}, + ptr::addr_of_mut, + slice, +}; -use crate::{bindings, cell::Opaque, prelude::*}; +use crate::{ + bindings, + callbacks::FnCall, + cell::{BqlRefMut, Opaque}, + prelude::*, +}; /// A safe wrapper around [`bindings::Chardev`]. #[repr(transparent)] @@ -14,6 +32,226 @@ use crate::{bindings, cell::Opaque, prelude::*}; pub struct Chardev(Opaque); pub type ChardevClass = bindings::ChardevClass; +pub type Event = bindings::QEMUChrEvent; + +/// A safe wrapper around [`bindings::CharBackend`], denoting the character +/// back-end that is used for example by a device. Compared to the +/// underlying C struct it adds BQL protection, and is marked as pinned +/// because the QOM object ([`bindings::Chardev`]) contains a pointer to +/// the `CharBackend`. +pub struct CharBackend { + inner: BqlRefCell, + _pin: PhantomPinned, +} + +impl Write for BqlRefMut<'_, bindings::CharBackend> { + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + + fn write(&mut self, buf: &[u8]) -> io::Result { + let chr: &mut bindings::CharBackend = self; + + let len = buf.len().try_into().unwrap(); + let r = unsafe { bindings::qemu_chr_fe_write(addr_of_mut!(*chr), buf.as_ptr(), len) }; + errno::into_io_result(r).map(|cnt| cnt as usize) + } + + fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + let chr: &mut bindings::CharBackend = self; + + let len = buf.len().try_into().unwrap(); + let r = unsafe { bindings::qemu_chr_fe_write_all(addr_of_mut!(*chr), buf.as_ptr(), len) }; + errno::into_io_result(r).and_then(|cnt| { + if cnt as usize == buf.len() { + Ok(()) + } else { + Err(ErrorKind::WriteZero.into()) + } + }) + } +} + +impl Debug for CharBackend { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // SAFETY: accessed just to print the values + let chr = self.inner.as_ptr(); + Debug::fmt(unsafe { &*chr }, f) + } +} + +// FIXME: use something like PinnedDrop from the pinned_init crate +impl Drop for CharBackend { + fn drop(&mut self) { + self.disable_handlers(); + } +} + +impl CharBackend { + /// Enable the front-end's character device handlers, if there is an + /// associated `Chardev`. + pub fn enable_handlers< + 'chardev, + 'owner: 'chardev, + T, + CanReceiveFn: for<'a> FnCall<(&'a T,), u32>, + ReceiveFn: for<'a, 'b> FnCall<(&'a T, &'b [u8])>, + EventFn: for<'a> FnCall<(&'a T, Event)>, + >( + // When "self" is dropped, the handlers are automatically disabled. + // However, this is not necessarily true if the owner is dropped. + // So require the owner to outlive the character device. + &'chardev self, + owner: &'owner T, + _can_receive: CanReceiveFn, + _receive: ReceiveFn, + _event: EventFn, + ) { + unsafe extern "C" fn rust_can_receive_cb FnCall<(&'a T,), u32>>( + opaque: *mut c_void, + ) -> c_int { + // SAFETY: the values are safe according to the contract of + // enable_handlers() and qemu_chr_fe_set_handlers() + let owner: &T = unsafe { &*(opaque.cast::()) }; + let r = F::call((owner,)); + r.try_into().unwrap() + } + + unsafe extern "C" fn rust_receive_cb FnCall<(&'a T, &'b [u8])>>( + opaque: *mut c_void, + buf: *const u8, + size: c_int, + ) { + // SAFETY: the values are safe according to the contract of + // enable_handlers() and qemu_chr_fe_set_handlers() + let owner: &T = unsafe { &*(opaque.cast::()) }; + let buf = unsafe { slice::from_raw_parts(buf, size.try_into().unwrap()) }; + F::call((owner, buf)) + } + + unsafe extern "C" fn rust_event_cb FnCall<(&'a T, Event)>>( + opaque: *mut c_void, + event: Event, + ) { + // SAFETY: the values are safe according to the contract of + // enable_handlers() and qemu_chr_fe_set_handlers() + let owner: &T = unsafe { &*(opaque.cast::()) }; + F::call((owner, event)) + } + + let _: () = CanReceiveFn::ASSERT_IS_SOME; + let receive_cb: Option = + if ReceiveFn::is_some() { + Some(rust_receive_cb::) + } else { + None + }; + let event_cb: Option = if EventFn::is_some() { + Some(rust_event_cb::) + } else { + None + }; + + let mut chr = self.inner.borrow_mut(); + // SAFETY: the borrow promises that the BQL is taken + unsafe { + bindings::qemu_chr_fe_set_handlers( + addr_of_mut!(*chr), + Some(rust_can_receive_cb::), + receive_cb, + event_cb, + None, + (owner as *const T as *mut T).cast::(), + core::ptr::null_mut(), + true, + ); + } + } + + /// Disable the front-end's character device handlers. + pub fn disable_handlers(&self) { + let mut chr = self.inner.borrow_mut(); + // SAFETY: the borrow promises that the BQL is taken + unsafe { + bindings::qemu_chr_fe_set_handlers( + addr_of_mut!(*chr), + None, + None, + None, + None, + core::ptr::null_mut(), + core::ptr::null_mut(), + true, + ); + } + } + + /// Notify that the frontend is ready to receive data. + pub fn accept_input(&self) { + let mut chr = self.inner.borrow_mut(); + // SAFETY: the borrow promises that the BQL is taken + unsafe { bindings::qemu_chr_fe_accept_input(addr_of_mut!(*chr)) } + } + + /// Temporarily borrow the character device, allowing it to be used + /// as an implementor of `Write`. Note that it is not valid to drop + /// the big QEMU lock while the character device is borrowed, as + /// that might cause C code to write to the character device. + pub fn borrow_mut(&self) -> impl Write + '_ { + self.inner.borrow_mut() + } + + /// Send a continuous stream of zero bits on the line if `enabled` is + /// true, or a short stream if `enabled` is false. + pub fn send_break(&self, long: bool) -> io::Result<()> { + let mut chr = self.inner.borrow_mut(); + let mut duration: c_int = long.into(); + // SAFETY: the borrow promises that the BQL is taken + let r = unsafe { + bindings::qemu_chr_fe_ioctl( + addr_of_mut!(*chr), + bindings::CHR_IOCTL_SERIAL_SET_BREAK as i32, + addr_of_mut!(duration).cast::(), + ) + }; + + errno::into_io_result(r).map(|_| ()) + } + + /// Write data to a character backend from the front end. This function + /// will send data from the front end to the back end. Unlike + /// `write`, this function will block if the back end cannot + /// consume all of the data attempted to be written. + /// + /// Returns the number of bytes consumed (0 if no associated Chardev) or an + /// error. + pub fn write(&self, buf: &[u8]) -> io::Result { + let len = buf.len().try_into().unwrap(); + // SAFETY: qemu_chr_fe_write is thread-safe + let r = unsafe { bindings::qemu_chr_fe_write(self.inner.as_ptr(), buf.as_ptr(), len) }; + errno::into_io_result(r).map(|cnt| cnt as usize) + } + + /// Write data to a character backend from the front end. This function + /// will send data from the front end to the back end. Unlike + /// `write`, this function will block if the back end cannot + /// consume all of the data attempted to be written. + /// + /// Returns the number of bytes consumed (0 if no associated Chardev) or an + /// error. + pub fn write_all(&self, buf: &[u8]) -> io::Result<()> { + let len = buf.len().try_into().unwrap(); + // SAFETY: qemu_chr_fe_write_all is thread-safe + let r = unsafe { bindings::qemu_chr_fe_write_all(self.inner.as_ptr(), buf.as_ptr(), len) }; + errno::into_io_result(r).and_then(|cnt| { + if cnt as usize == buf.len() { + Ok(()) + } else { + Err(ErrorKind::WriteZero.into()) + } + }) + } +} unsafe impl ObjectType for Chardev { type Class = ChardevClass; diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs index 47b6977..a3415a2 100644 --- a/rust/qemu-api/src/zeroable.rs +++ b/rust/qemu-api/src/zeroable.rs @@ -106,3 +106,4 @@ impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1); impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2); impl_zeroable!(crate::bindings::MemoryRegionOps); impl_zeroable!(crate::bindings::MemTxAttrs); +impl_zeroable!(crate::bindings::CharBackend); -- cgit v1.1 From 5778ce99971f7e09952a1efbac91d0c97d7a0fee Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 14 Feb 2025 14:10:38 +0100 Subject: rust: cell: add full example of declaring a SysBusDevice Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/cell.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs index 448638e..ab0785a 100644 --- a/rust/qemu-api/src/cell.rs +++ b/rust/qemu-api/src/cell.rs @@ -73,6 +73,34 @@ //! QEMU device implementations is usually incorrect and can lead to //! thread-safety issues. //! +//! ### Example +//! +//! ``` +//! # use qemu_api::prelude::*; +//! # use qemu_api::{c_str, cell::BqlRefCell, irq::InterruptSource, irq::IRQState}; +//! # use qemu_api::{sysbus::SysBusDevice, qom::Owned, qom::ParentField}; +//! # const N_GPIOS: usize = 8; +//! # struct PL061Registers { /* ... */ } +//! # unsafe impl ObjectType for PL061State { +//! # type Class = ::Class; +//! # const TYPE_NAME: &'static std::ffi::CStr = c_str!("pl061"); +//! # } +//! struct PL061State { +//! parent_obj: ParentField, +//! +//! // Configuration is read-only after initialization +//! pullups: u32, +//! pulldowns: u32, +//! +//! // Single values shared with C code use BqlCell, in this case via InterruptSource +//! out: [InterruptSource; N_GPIOS], +//! interrupt: InterruptSource, +//! +//! // Larger state accessed by device methods uses BqlRefCell or Mutex +//! registers: BqlRefCell, +//! } +//! ``` +//! //! ### `BqlCell` //! //! [`BqlCell`] implements interior mutability by moving values in and out of -- cgit v1.1 From 094cd35913bd66228a9a3239e66b1f6f5d667d4b Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 13 Dec 2024 17:54:33 +0100 Subject: rust: qom: remove operations on &mut The dubious casts of mutable references to objects are not used anymore: the wrappers for qdev_init_clock_in and for IRQ and MMIO initialization can be called directly on the subclasses, without casts, plus they take a shared reference so they can just use "upcast()" instead of "upcast_mut()". Remove them. Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/memory.rs | 5 --- rust/qemu-api/src/prelude.rs | 1 - rust/qemu-api/src/qom.rs | 83 -------------------------------------------- 3 files changed, 89 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs index eff9f09..fdb1ea1 100644 --- a/rust/qemu-api/src/memory.rs +++ b/rust/qemu-api/src/memory.rs @@ -175,11 +175,6 @@ impl MemoryRegion { ) { unsafe { 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::(), &ops.0, diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs index 634acf3..43bfcd5 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -17,7 +17,6 @@ pub use crate::qom::InterfaceType; pub use crate::qom::IsA; pub use crate::qom::Object; pub use crate::qom::ObjectCast; -pub use crate::qom::ObjectCastMut; pub use crate::qom::ObjectDeref; pub use crate::qom::ObjectClassMethods; pub use crate::qom::ObjectMethods; diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 2defbd2..34d7bc0 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -463,90 +463,7 @@ where impl ObjectDeref for &T {} impl ObjectCast for &T {} -/// Trait for mutable type casting operations in the QOM hierarchy. -/// -/// This trait provides the mutable counterparts to [`ObjectCast`]'s conversion -/// functions. Unlike `ObjectCast`, this trait returns `Result` for fallible -/// conversions to preserve the original smart pointer if the cast fails. This -/// is necessary because mutable references cannot be copied, so a failed cast -/// must return ownership of the original reference. For example: -/// -/// ```ignore -/// let mut dev = get_device(); -/// // If this fails, we need the original `dev` back to try something else -/// match dev.dynamic_cast_mut::() { -/// Ok(foodev) => /* use foodev */, -/// Err(dev) => /* still have ownership of dev */ -/// } -/// ``` -pub trait ObjectCastMut: Sized + ObjectDeref + DerefMut -where - Self::Target: ObjectType, -{ - /// Safely convert from a derived type to one of its parent types. - /// - /// This is always safe; the [`IsA`] trait provides static verification - /// that `Self` dereferences to `U` or a child of `U`. - fn upcast_mut<'a, U: ObjectType>(self) -> &'a mut U - where - Self::Target: IsA, - Self: 'a, - { - // SAFETY: soundness is declared via IsA, which is an unsafe trait - unsafe { self.unsafe_cast_mut::() } - } - - /// Attempt to convert to a derived type. - /// - /// Returns `Ok(..)` if the object is of type `U`, or `Err(self)` if the - /// object if the conversion failed. This is verified at runtime by - /// checking the object's type information. - fn downcast_mut<'a, U: IsA>(self) -> Result<&'a mut U, Self> - where - Self: 'a, - { - self.dynamic_cast_mut::() - } - - /// Attempt to convert between any two types in the QOM hierarchy. - /// - /// Returns `Ok(..)` if the object is of type `U`, or `Err(self)` if the - /// object if the conversion failed. This is verified at runtime by - /// checking the object's type information. - fn dynamic_cast_mut<'a, U: ObjectType>(self) -> Result<&'a mut U, Self> - where - Self: 'a, - { - unsafe { - // SAFETY: upcasting to Object is always valid, and the - // return type is either NULL or the argument itself - let result: *mut U = - object_dynamic_cast(self.as_object_mut_ptr(), U::TYPE_NAME.as_ptr()).cast(); - - result.as_mut().ok_or(self) - } - } - - /// Convert to any QOM type without verification. - /// - /// # Safety - /// - /// What safety? You need to know yourself that the cast is correct; only - /// use when performance is paramount. It is still better than a raw - /// pointer `cast()`, which does not even check that you remain in the - /// realm of QOM `ObjectType`s. - /// - /// `unsafe_cast::()` is always safe. - unsafe fn unsafe_cast_mut<'a, U: ObjectType>(self) -> &'a mut U - where - Self: 'a, - { - unsafe { &mut *self.as_mut_ptr::().cast::() } - } -} - impl ObjectDeref for &mut T {} -impl ObjectCastMut for &mut T {} /// Trait a type must implement to be registered with QEMU. pub trait ObjectImpl: ObjectType + IsA { -- cgit v1.1