From ca0d60a6ad777ab617cbc4e6f328eaff60617b3f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 11:38:20 +0100 Subject: rust: qom: add ParentField Add a type that, together with the C function object_deinit, ensures the correct drop order for QOM objects relative to their superclasses. Right now it is not possible to implement the Drop trait for QOM classes that are defined in Rust, as the drop() function would not be called when the object goes away; instead what is called is ObjectImpl::INSTANCE_FINALIZE. It would be nice for INSTANCE_FINALIZE to just drop the object, but this has a problem: suppose you have pub struct MySuperclass { parent: DeviceState, field: Box, ... } impl Drop for MySuperclass { ... } pub struct MySubclass { parent: MySuperclass, ... } and an instance_finalize implementation that is like unsafe extern "C" fn drop_object(obj: *mut Object) { unsafe { std::ptr::drop_in_place(obj.cast::()) } } When instance_finalize is called for MySubclass, it will walk the struct's list of fields and call the drop method for MySuperclass. Then, object_deinit recurses to the superclass and calls the same drop method again. This will cause double-freeing of the Box. What's happening here is that QOM wants to control the drop order of MySuperclass and MySubclass's fields. To do so, the parent field must be marked ManuallyDrop<>, which is quite ugly. Instead, add a wrapper type ParentField<> that is specific to QOM. This hides the implementation detail of *what* is special about the ParentField, and will also be easy to check in the #[derive(Object)] macro. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/qom.rs | 64 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 6 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 7d5fbef..40d17a9 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -55,6 +55,7 @@ use std::{ ffi::CStr, + fmt, ops::{Deref, DerefMut}, os::raw::c_void, }; @@ -105,6 +106,52 @@ macro_rules! qom_isa { }; } +/// This is the same as [`ManuallyDrop`](std::mem::ManuallyDrop), though +/// it hides the standard methods of `ManuallyDrop`. +/// +/// The first field of an `ObjectType` must be of type `ParentField`. +/// (Technically, this is only necessary if there is at least one Rust +/// superclass in the hierarchy). This is to ensure that the parent field is +/// dropped after the subclass; this drop order is enforced by the C +/// `object_deinit` function. +/// +/// # Examples +/// +/// ```ignore +/// #[repr(C)] +/// #[derive(qemu_api_macros::Object)] +/// pub struct MyDevice { +/// parent: ParentField, +/// ... +/// } +/// ``` +#[derive(Debug)] +#[repr(transparent)] +pub struct ParentField(std::mem::ManuallyDrop); + +impl Deref for ParentField { + type Target = T; + + #[inline(always)] + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for ParentField { + #[inline(always)] + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl fmt::Display for ParentField { + #[inline(always)] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { + self.0.fmt(f) + } +} + unsafe extern "C" fn rust_instance_init(obj: *mut Object) { // SAFETY: obj is an instance of T, since rust_instance_init // is called from QOM core as the instance_init function @@ -151,11 +198,16 @@ unsafe extern "C" fn rust_class_init>( /// /// - the struct must be `#[repr(C)]`; /// -/// - the first field of the struct must be of the instance struct corresponding -/// to the superclass, which is `ObjectImpl::ParentType` +/// - the first field of the struct must be of type +/// [`ParentField`](ParentField), where `T` is the parent type +/// [`ObjectImpl::ParentType`] +/// +/// - the first field of the `Class` must be of the class struct corresponding +/// to the superclass, which is `ObjectImpl::ParentType::Class`. `ParentField` +/// is not needed here. /// -/// - likewise, the first field of the `Class` must be of the class struct -/// corresponding to the superclass, which is `ObjectImpl::ParentType::Class`. +/// In both cases, having a separate class type is not necessary if the subclass +/// does not add any field. pub unsafe trait ObjectType: Sized { /// The QOM class object corresponding to this struct. This is used /// to automatically generate a `class_init` method. @@ -384,8 +436,8 @@ impl ObjectCastMut for &mut T {} /// Trait a type must implement to be registered with QEMU. pub trait ObjectImpl: ObjectType + ClassInitImpl { - /// The parent of the type. This should match the first field of - /// the struct that implements `ObjectImpl`: + /// The parent of the type. This should match the first field of the + /// struct that implements `ObjectImpl`, minus the `ParentField<_>` wrapper. type ParentType: ObjectType; /// Whether the object can be instantiated -- cgit v1.1 From 7f65d4e58b67d6e5ee05c9381a50ef1eba3a5a1e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 12:18:06 +0100 Subject: rust: add a utility module for compile-time type checks It is relatively common in the low-level qemu_api code to assert that a field of a struct has a specific type; for example, it can be used to ensure that the fields match what the qemu_api and C code expects for safety. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/assertions.rs | 90 +++++++++++++++++++++++++++++++++++++++++ rust/qemu-api/src/lib.rs | 1 + 2 files changed, 91 insertions(+) create mode 100644 rust/qemu-api/src/assertions.rs (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs new file mode 100644 index 0000000..6e42046 --- /dev/null +++ b/rust/qemu-api/src/assertions.rs @@ -0,0 +1,90 @@ +// Copyright 2024, Red Hat Inc. +// Author(s): Paolo Bonzini +// SPDX-License-Identifier: GPL-2.0-or-later + +//! This module provides macros to check the equality of types and +//! the type of `struct` fields. This can be useful to ensure that +//! types match the expectations of C code. + +// Based on https://stackoverflow.com/questions/64251852/x/70978292#70978292 +// (stackoverflow answers are released under MIT license). + +#[doc(hidden)] +pub trait EqType { + type Itself; +} + +impl EqType for T { + type Itself = T; +} + +/// Assert that two types are the same. +/// +/// # Examples +/// +/// ``` +/// # use qemu_api::assert_same_type; +/// # use std::ops::Deref; +/// assert_same_type!(u32, u32); +/// assert_same_type!( as Deref>::Target, u32); +/// ``` +/// +/// Different types will cause a compile failure +/// +/// ```compile_fail +/// # use qemu_api::assert_same_type; +/// assert_same_type!(&Box, &u32); +/// ``` +#[macro_export] +macro_rules! assert_same_type { + ($t1:ty, $t2:ty) => { + const _: () = { + #[allow(unused)] + fn assert_same_type(v: $t1) { + fn types_must_be_equal(_: T) + where + T: $crate::assertions::EqType, + { + } + types_must_be_equal::<_, $t2>(v); + } + }; + }; +} + +/// Assert that a field of a struct has the given type. +/// +/// # Examples +/// +/// ``` +/// # use qemu_api::assert_field_type; +/// pub struct A { +/// field1: u32, +/// } +/// +/// assert_field_type!(A, field1, u32); +/// ``` +/// +/// Different types will cause a compile failure +/// +/// ```compile_fail +/// # use qemu_api::assert_field_type; +/// # pub struct A { field1: u32 } +/// assert_field_type!(A, field1, i32); +/// ``` +#[macro_export] +macro_rules! assert_field_type { + ($t:ty, $i:tt, $ti:ty) => { + const _: () = { + #[allow(unused)] + fn assert_field_type(v: $t) { + fn types_must_be_equal(_: T) + where + T: $crate::assertions::EqType, + { + } + types_must_be_equal::<_, $ti>(v.$i); + } + }; + }; +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 4b43e02..83c6a98 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -12,6 +12,7 @@ pub mod bindings; #[rustfmt::skip] pub mod prelude; +pub mod assertions; pub mod bitops; pub mod c_str; pub mod callbacks; -- cgit v1.1 From 33aa660575f7174752a7308229e5fc108921c2a6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 10:33:31 +0100 Subject: rust: qom: automatically use Drop trait to implement instance_finalize Replace the customizable INSTANCE_FINALIZE with a generic function that drops the Rust object. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/qom.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 40d17a9..b0332ba 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -180,6 +180,16 @@ unsafe extern "C" fn rust_class_init>( T::class_init(unsafe { &mut *klass.cast::() }) } +unsafe extern "C" fn drop_object(obj: *mut 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 + // superclass field separately after this function returns, `T` must + // implement the unsafe trait ObjectType; the safety rules for the + // trait mandate that the parent field is manually dropped. + unsafe { std::ptr::drop_in_place(obj.cast::()) } +} + /// Trait exposed by all structs corresponding to QOM objects. /// /// # Safety @@ -442,7 +452,6 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { /// Whether the object can be instantiated const ABSTRACT: bool = false; - const INSTANCE_FINALIZE: Option = None; /// Function that is called to initialize an object. The parent class will /// have already been initialized so the type is only responsible for @@ -478,7 +487,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { None => None, Some(_) => Some(rust_instance_post_init::), }, - instance_finalize: Self::INSTANCE_FINALIZE, + instance_finalize: Some(drop_object::), abstract_: Self::ABSTRACT, class_size: core::mem::size_of::(), class_init: Some(rust_class_init::), -- cgit v1.1 From af68b41d403b81b18de07ebab0ca4c1025c94bf7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 2 Dec 2024 13:16:19 +0100 Subject: rust: pl011: only leave embedded object initialization in instance_init Leave IRQ and MMIO initialization to instance_post_init. In Rust the two callbacks are more distinct, because only instance_post_init has a fully initialized object available. While at it, add a wrapper for sysbus_init_mmio so that accesses to the SysBusDevice correctly use shared references. Reviewed-by: Zhao Liu 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 8193734..b96eaaf 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -38,6 +38,18 @@ impl SysBusDevice { addr_of!(*self) as *mut _ } + /// Expose a memory region to the board so that it can give it an address + /// in guest memory. Note that the ordering of calls to `init_mmio` is + /// important, since whoever creates the sysbus device will refer to the + /// region with a number that corresponds to the order of calls to + /// `init_mmio`. + pub fn init_mmio(&self, iomem: &bindings::MemoryRegion) { + assert!(bql_locked()); + unsafe { + bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _); + } + } + /// Expose an interrupt source outside the device as a qdev GPIO output. /// Note that the ordering of calls to `init_irq` is important, since /// whoever creates the sysbus device will refer to the interrupts with -- cgit v1.1 From 22a18f0a98a1ca5d0cd8fff1d81cc74a269083de Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 08:48:07 +0100 Subject: rust: qom: make INSTANCE_POST_INIT take a shared reference Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/qom.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index b0332ba..97901fb 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -163,11 +163,7 @@ unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { // SAFETY: obj is an instance of T, since rust_instance_post_init // is called from QOM core as the instance_post_init function // for class T - // - // FIXME: it's not really guaranteed that there are no backpointers to - // obj; it's quite possible that they have been created by instance_init(). - // The receiver should be &self, not &mut self. - T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::() }) + T::INSTANCE_POST_INIT.unwrap()(unsafe { &*obj.cast::() }) } unsafe extern "C" fn rust_class_init>( @@ -463,7 +459,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { /// Function that is called to finish initialization of an object, once /// `INSTANCE_INIT` functions have been called. - const INSTANCE_POST_INIT: Option = None; + const INSTANCE_POST_INIT: Option = None; /// Called on descendent classes after all parent class initialization /// has occurred, but before the class itself is initialized. This -- cgit v1.1 From 559a779c6aa309853474240b01fcc2beff1f04ca Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 10:46:44 +0100 Subject: rust: qdev: expose inherited methods to subclasses of SysBusDevice The ObjectDeref trait now provides all the magic that is required to fake inheritance. Replace the "impl SysBusDevice" block of qemu_api::sysbus with a trait, so that sysbus_init_irq() can be invoked as "self.init_irq()" without any intermediate upcast. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/irq.rs | 3 +-- rust/qemu-api/src/prelude.rs | 2 ++ rust/qemu-api/src/sysbus.rs | 17 +++++++++-------- 3 files changed, 12 insertions(+), 10 deletions(-) (limited to 'rust/qemu-api/src') diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs index 6258141..378e520 100644 --- a/rust/qemu-api/src/irq.rs +++ b/rust/qemu-api/src/irq.rs @@ -24,8 +24,7 @@ use crate::{ /// /// 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 -/// [`SysBusDevice::init_irq`](crate::sysbus::SysBusDevice::init_irq), and +/// a function such as [`SysBusDeviceMethods::init_irq`], and /// initially leaves the pointer to a NULL value, representing an unconnected /// interrupt. To connect it, whoever creates the device fills the pointer with /// the sink's `IRQState *`, for example using `sysbus_connect_irq`. Because diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs index 6f32dee..4ea70b9 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -16,3 +16,5 @@ pub use crate::qom::ObjectMethods; pub use crate::qom::ObjectType; pub use crate::qom_isa; + +pub use crate::sysbus::SysBusDeviceMethods; diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs index b96eaaf..e6762b5 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -32,18 +32,17 @@ where } } -impl SysBusDevice { - /// Return `self` cast to a mutable pointer, for use in calls to C code. - const fn as_mut_ptr(&self) -> *mut SysBusDevice { - addr_of!(*self) as *mut _ - } - +/// Trait for methods of [`SysBusDevice`] and its subclasses. +pub trait SysBusDeviceMethods: ObjectDeref +where + Self::Target: IsA, +{ /// Expose a memory region to the board so that it can give it an address /// in guest memory. Note that the ordering of calls to `init_mmio` is /// important, since whoever creates the sysbus device will refer to the /// region with a number that corresponds to the order of calls to /// `init_mmio`. - pub fn init_mmio(&self, iomem: &bindings::MemoryRegion) { + fn init_mmio(&self, iomem: &bindings::MemoryRegion) { assert!(bql_locked()); unsafe { bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _); @@ -54,10 +53,12 @@ impl SysBusDevice { /// Note that the ordering of calls to `init_irq` is important, since /// whoever creates the sysbus device will refer to the interrupts with /// a number that corresponds to the order of calls to `init_irq`. - pub fn init_irq(&self, irq: &InterruptSource) { + fn init_irq(&self, irq: &InterruptSource) { assert!(bql_locked()); unsafe { bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr()); } } } + +impl SysBusDeviceMethods for R where R::Target: IsA {} -- cgit v1.1