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/qom.rs') diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 7d5fbef1e1..40d17a92e1 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 1.4.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/qom.rs') diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 40d17a92e1..b0332ba247 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 1.4.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/hw/char/pl011/src/device.rs | 4 ++-- rust/qemu-api/src/qom.rs | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) (limited to 'rust/qemu-api/src/qom.rs') diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index 72a4cea042..6792d13fb7 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -145,7 +145,7 @@ impl ObjectImpl for PL011State { type ParentType = SysBusDevice; const INSTANCE_INIT: Option = Some(Self::init); - const INSTANCE_POST_INIT: Option = Some(Self::post_init); + const INSTANCE_POST_INIT: Option = Some(Self::post_init); } impl DeviceImpl for PL011State { @@ -206,7 +206,7 @@ impl PL011State { } } - fn post_init(&mut self) { + fn post_init(&self) { let sbd: &SysBusDevice = self.upcast(); sbd.init_mmio(&self.iomem); diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index b0332ba247..97901fb908 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 1.4.1