[PATCH 00/15] rust: prepare for splitting crates

[PATCH 00/15] rust: prepare for splitting crates

[Paolo Bonzini]

This series is logically split in two parts.  The first one is more
strictly tied to the objective of splitting qemu_api into multiple crates,
as it breaks the QOM bindings free of the constraints imposed by Rust
orphan rules.  The second one instead completes another task that was on
my todo list, namely ensuring that rustc sees the possible changes that
C functions can make when a shared reference &bindings::Foo is passed
via *mut bindings::Foo.

More in detail, patches 1-5 redo the QOM class init mechanism so that
it does not rely on the ClassInitImpl trait.  While ClassInitImpl is
very nice in theory, it practice it does not play well with classes
defined outside qemu_api (or more precisely, outside the crate that
defines ClassInitImpl itself).  This is because ClassInitImpl relies on
a blanket implementation such as

    impl<T> ClassInitImpl<ObjectClass> for T
    where
        T: ObjectImpl

and this is only possible inside the crate that defines ClassInitImpl.
With these patches the class_init methods move from ClassInitImpl<Class>
to the Class struct itself.  More precisely, before the series you
have

    <T as ClassInitImpl<ObjectClass>>::class_init

and afterwards

    ObjectClass::class_init::<T>

which is assigned to the CLASS_INIT associated const of ObjectImpl.
The problem is that even if CLASS_INIT is always defined the same
way, i.e.

    const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;

there isn't a way to tell the compiler that the above is valid *in the
definition of ObjectImpl*.  Therefore the above line has to be duplicated
in all subclasses; this however is a small issue and in the future it
could be handled by #[derive(Object)].

Patches 6-15 instead introduce a third generic type in qemu_api::cell,
Opaque<T>.  This type is similar to a same-named type in Linux; it is
basically a "disable all undefined behavior" switch for the Rust compiler
and it helps maintaining safety at the Rust/C boundary, complementing
the existing BqlCell and BqlRefCell types.

Apart from making things more formally correct, this makes it possible
to implement methods on a struct that is distinct from the one produced
by bindgen.  This has a couple of advantages:

- you do not have to disable the Copy trait on structs where you want
  to add a Drop trait.  This was already a problem for the Timer struct.

- whether Send and Sync are implemented is entirely a decision of the
  place that implements the wrapper.  Previously, a struct with no
  pointers for example would have been always both Send and Sync,
  whereas now that can be adjusted depending on the actual
  thread-safety of the Rust methods.

- more pertinent to the "multiple crates" plan, you do not have to put
  the methods in the same crate as the bindgen-generated bindings.inc.rs.

It also makes Debug output a bit less unwieldy, and in the future one
might want to add specialized implementations of Display and Debug that
are both useful and readable.

Paolo

Paolo Bonzini (15):
  rust: add IsA bounds to QOM implementation traits
  rust: add SysBusDeviceImpl
  rust: qom: add ObjectImpl::CLASS_INIT
  rust: pl011, qemu_api tests: do not use ClassInitImpl
  rust: qom: get rid of ClassInitImpl
  rust: cell: add wrapper for FFI types
  rust: qemu_api_macros: add Wrapper derive macro
  rust: timer: wrap QEMUTimer with Opaque<>
  rust: irq: wrap IRQState with Opaque<>
  rust: qom: wrap Object with Opaque<>
  rust: qdev: wrap Clock and DeviceState with Opaque<>
  rust: sysbus: wrap SysBusDevice with Opaque<>
  rust: memory: wrap MemoryRegion with Opaque<>
  rust: chardev: wrap Chardev with Opaque<>
  rust: bindings: remove more unnecessary Send/Sync impls

 docs/devel/rust.rst              |  36 +++--
 meson.build                      |   7 -
 rust/hw/char/pl011/src/device.rs |  44 +++---
 rust/hw/timer/hpet/src/hpet.rs   |   7 +-
 rust/qemu-api-macros/src/lib.rs  |  82 +++++++++++-
 rust/qemu-api/meson.build        |   7 +-
 rust/qemu-api/src/bindings.rs    |  26 +---
 rust/qemu-api/src/cell.rs        | 222 ++++++++++++++++++++++++++++++-
 rust/qemu-api/src/chardev.rs     |   8 +-
 rust/qemu-api/src/irq.rs         |  15 ++-
 rust/qemu-api/src/memory.rs      |  32 ++---
 rust/qemu-api/src/qdev.rs        | 115 ++++++++++------
 rust/qemu-api/src/qom.rs         | 203 ++++++++++++++--------------
 rust/qemu-api/src/sysbus.rs      |  45 ++++---
 rust/qemu-api/src/timer.rs       |  24 +++-
 rust/qemu-api/src/vmstate.rs     |   2 +-
 rust/qemu-api/tests/tests.rs     |  32 ++---
 17 files changed, 620 insertions(+), 287 deletions(-)

-- 
2.48.1

[PATCH 01/15] rust: add IsA bounds to QOM implementation traits

[Paolo Bonzini]

Check that the right bounds are provided to the qom_isa! macro
whenever the class is defined to implement a certain class.
This removes the need to add IsA<> bounds together with the
*Impl trait bounds.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/qdev.rs | 2 +-
 rust/qemu-api/src/qom.rs  | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 3a7aa4def62..c4dd26b582c 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -86,7 +86,7 @@ pub trait ResettablePhasesImpl {
 }
 
 /// Trait providing the contents of [`DeviceClass`].
-pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl {
+pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl + IsA<DeviceState> {
     /// _Realization_ is the second stage of device creation. It contains
     /// all operations that depend on device properties and can fail (note:
     /// this is not yet supported for Rust devices).
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 3d5ab2d9018..10ce359becb 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -37,6 +37,8 @@
 //! * a trait for virtual method implementations, for example `DeviceImpl`.
 //!   Child classes implement this trait to provide their own behavior for
 //!   virtual methods. The trait's methods take `&self` to access instance data.
+//!   The traits have the appropriate specialization of `IsA<>` as a supertrait,
+//!   for example `IsA<DeviceState>` for `DeviceImpl`.
 //!
 //! * an implementation of [`ClassInitImpl`], for example
 //!   `ClassInitImpl<DeviceClass>`. This fills the vtable in the class struct;
@@ -497,7 +499,7 @@ impl<T: ObjectType> ObjectDeref for &mut T {}
 impl<T: ObjectType> ObjectCastMut for &mut T {}
 
 /// Trait a type must implement to be registered with QEMU.
-pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
+pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> + IsA<Object> {
     /// The parent of the type.  This should match the first field of the
     /// struct that implements `ObjectImpl`, minus the `ParentField<_>` wrapper.
     type ParentType: ObjectType;
-- 
2.48.1

[PATCH 02/15] rust: add SysBusDeviceImpl

[Paolo Bonzini]

The only function, right now, is to ensure that anything with a
SysBusDeviceClass class is a SysBusDevice.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 5 ++++-
 rust/hw/timer/hpet/src/hpet.rs   | 4 +++-
 rust/qemu-api/src/sysbus.rs      | 8 +++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index fe73771021e..7063b60c0cc 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -20,7 +20,7 @@
     prelude::*,
     qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
     qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
-    sysbus::{SysBusDevice, SysBusDeviceClass},
+    sysbus::{SysBusDevice, SysBusDeviceClass, SysBusDeviceImpl},
     vmstate::VMStateDescription,
 };
 
@@ -176,6 +176,8 @@ impl ResettablePhasesImpl for PL011State {
     const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
 }
 
+impl SysBusDeviceImpl for PL011State {}
+
 impl PL011Registers {
     pub(self) fn read(&mut self, offset: RegisterOffset) -> (bool, u32) {
         use RegisterOffset::*;
@@ -746,3 +748,4 @@ impl ObjectImpl for PL011Luminary {
 
 impl DeviceImpl for PL011Luminary {}
 impl ResettablePhasesImpl for PL011Luminary {}
+impl SysBusDeviceImpl for PL011Luminary {}
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 75ff5b3e8d6..b4ffccf815f 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -23,7 +23,7 @@
     qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
     qom::{ObjectImpl, ObjectType, ParentField},
     qom_isa,
-    sysbus::SysBusDevice,
+    sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL},
 };
 
@@ -887,3 +887,5 @@ fn properties() -> &'static [Property] {
 impl ResettablePhasesImpl for HPETState {
     const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
 }
+
+impl SysBusDeviceImpl for HPETState {}
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index fa36e12178f..fee2e3d478f 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -14,7 +14,7 @@
     irq::{IRQState, InterruptSource},
     memory::MemoryRegion,
     prelude::*,
-    qdev::{DeviceClass, DeviceState},
+    qdev::{DeviceClass, DeviceImpl, DeviceState},
     qom::{ClassInitImpl, Owned},
 };
 
@@ -25,10 +25,12 @@ unsafe impl ObjectType for SysBusDevice {
 }
 qom_isa!(SysBusDevice: DeviceState, Object);
 
-// TODO: add SysBusDeviceImpl
+// TODO: add virtual methods
+pub trait SysBusDeviceImpl: DeviceImpl + IsA<SysBusDevice> {}
+
 impl<T> ClassInitImpl<SysBusDeviceClass> for T
 where
-    T: ClassInitImpl<DeviceClass>,
+    T: SysBusDeviceImpl + ClassInitImpl<DeviceClass>,
 {
     fn class_init(sdc: &mut SysBusDeviceClass) {
         <T as ClassInitImpl<DeviceClass>>::class_init(&mut sdc.parent_class);
-- 
2.48.1

[PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT

[Paolo Bonzini]

As shown in the PL011 device, the orphan rules required a manual
implementation of ClassInitImpl for anything not in the qemu_api crate;
this gets in the way of moving system emulation-specific code (including
DeviceClass, which as a blanket ClassInitImpl<DeviceClass> implementation)
into its own crate.

Make ClassInitImpl optional, at the cost of having to specify the CLASS_INIT
member by hand in every implementation of ObjectImpl.  The next commits will
get rid of it, replacing all the "impl<T> ClassInitImpl<Class> for T" blocks
with a generic class_init<T> method on Class.

Right now the definition is always the same, but do not provide a default
as that will not be true once ClassInitImpl goes away.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs |  3 +++
 rust/hw/timer/hpet/src/hpet.rs   |  3 ++-
 rust/qemu-api/src/qom.rs         | 14 +++++++++++---
 rust/qemu-api/tests/tests.rs     |  3 +++
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 7063b60c0cc..a6da9db0bb0 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -160,6 +160,7 @@ impl ObjectImpl for PL011State {
 
     const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
+    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
 }
 
 impl DeviceImpl for PL011State {
@@ -744,6 +745,8 @@ unsafe impl ObjectType for PL011Luminary {
 
 impl ObjectImpl for PL011Luminary {
     type ParentType = PL011State;
+
+    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
 }
 
 impl DeviceImpl for PL011Luminary {}
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index b4ffccf815f..e01b4b67064 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -21,7 +21,7 @@
     },
     prelude::*,
     qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
-    qom::{ObjectImpl, ObjectType, ParentField},
+    qom::{ClassInitImpl, ObjectImpl, ObjectType, ParentField},
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL},
@@ -836,6 +836,7 @@ impl ObjectImpl for HPETState {
 
     const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
+    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
 }
 
 // TODO: Make these properties user-configurable!
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 10ce359becb..d821ac25acc 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -180,7 +180,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
     T::INSTANCE_POST_INIT.unwrap()(unsafe { state.as_ref() });
 }
 
-unsafe extern "C" fn rust_class_init<T: ObjectType + ClassInitImpl<T::Class>>(
+unsafe extern "C" fn rust_class_init<T: ObjectType + ObjectImpl>(
     klass: *mut ObjectClass,
     _data: *mut c_void,
 ) {
@@ -190,7 +190,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
     // SAFETY: klass is a T::Class, since rust_class_init<T>
     // is called from QOM core as the class_init function
     // for class T
-    T::class_init(unsafe { klass.as_mut() })
+    <T as ObjectImpl>::CLASS_INIT(unsafe { klass.as_mut() })
 }
 
 unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
@@ -499,7 +499,7 @@ impl<T: ObjectType> ObjectDeref for &mut T {}
 impl<T: ObjectType> ObjectCastMut for &mut T {}
 
 /// Trait a type must implement to be registered with QEMU.
-pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> + IsA<Object> {
+pub trait ObjectImpl: ObjectType + IsA<Object> {
     /// The parent of the type.  This should match the first field of the
     /// struct that implements `ObjectImpl`, minus the `ParentField<_>` wrapper.
     type ParentType: ObjectType;
@@ -552,6 +552,14 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> + IsA<Object> {
 
     // methods on ObjectClass
     const UNPARENT: Option<fn(&Self)> = None;
+
+    /// Store into the argument the virtual method implementations
+    /// for `Self`.  On entry, the virtual method pointers are set to
+    /// the default values coming from the parent classes; the function
+    /// can change them to override virtual methods of a parent class.
+    ///
+    /// Usually defined as `<Self as ClassInitImpl<Self::Class>::class_init`.
+    const CLASS_INIT: fn(&mut Self::Class);
 }
 
 /// Internal trait used to automatically fill in a class struct.
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 03569e4a44c..9546e9d7963 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -60,6 +60,7 @@ unsafe impl ObjectType for DummyState {
 impl ObjectImpl for DummyState {
     type ParentType = DeviceState;
     const ABSTRACT: bool = false;
+    const CLASS_INIT: fn(&mut DummyClass) = <Self as ClassInitImpl<DummyClass>>::class_init;
 }
 
 impl ResettablePhasesImpl for DummyState {}
@@ -102,6 +103,8 @@ unsafe impl ObjectType for DummyChildState {
 impl ObjectImpl for DummyChildState {
     type ParentType = DummyState;
     const ABSTRACT: bool = false;
+    const CLASS_INIT: fn(&mut DummyChildClass) =
+        <Self as ClassInitImpl<DummyChildClass>>::class_init;
 }
 
 impl ResettablePhasesImpl for DummyChildState {}
-- 
2.48.1

[PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl

[Paolo Bonzini]

Outside the qemu_api crate, orphan rules make the usage of ClassInitImpl
unwieldy.  Now that it is optional, do not use it.

For PL011Class, this makes it easier to provide a PL011Impl trait similar
to the ones in the qemu_api crate.  The device id consts are moved there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 38 ++++++++++++++++----------------
 rust/qemu-api/tests/tests.rs     | 33 ++++++++++-----------------
 2 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index a6da9db0bb0..fc1c8ec8d6a 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -50,11 +50,6 @@ fn index(&self, idx: hwaddr) -> &Self::Output {
     }
 }
 
-impl DeviceId {
-    const ARM: Self = Self(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
-    const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
-}
-
 // FIFOs use 32-bit indices instead of usize, for compatibility with
 // the migration stream produced by the C version of this device.
 #[repr(transparent)]
@@ -143,16 +138,24 @@ pub struct PL011Class {
     device_id: DeviceId,
 }
 
+trait PL011Impl: SysBusDeviceImpl + IsA<PL011State> {
+    const DEVICE_ID: DeviceId;
+}
+
+impl PL011Class {
+    fn class_init<T: PL011Impl>(&mut self) {
+        self.device_id = T::DEVICE_ID;
+        <T as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut self.parent_class);
+    }
+}
+
 unsafe impl ObjectType for PL011State {
     type Class = PL011Class;
     const TYPE_NAME: &'static CStr = crate::TYPE_PL011;
 }
 
-impl ClassInitImpl<PL011Class> for PL011State {
-    fn class_init(klass: &mut PL011Class) {
-        klass.device_id = DeviceId::ARM;
-        <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
-    }
+impl PL011Impl for PL011State {
+    const DEVICE_ID: DeviceId = DeviceId(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
 }
 
 impl ObjectImpl for PL011State {
@@ -160,7 +163,7 @@ impl ObjectImpl for PL011State {
 
     const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
-    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
+    const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
 }
 
 impl DeviceImpl for PL011State {
@@ -729,13 +732,6 @@ pub struct PL011Luminary {
     parent_obj: ParentField<PL011State>,
 }
 
-impl ClassInitImpl<PL011Class> for PL011Luminary {
-    fn class_init(klass: &mut PL011Class) {
-        klass.device_id = DeviceId::LUMINARY;
-        <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
-    }
-}
-
 qom_isa!(PL011Luminary : PL011State, SysBusDevice, DeviceState, Object);
 
 unsafe impl ObjectType for PL011Luminary {
@@ -746,7 +742,11 @@ unsafe impl ObjectType for PL011Luminary {
 impl ObjectImpl for PL011Luminary {
     type ParentType = PL011State;
 
-    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
+    const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
+}
+
+impl PL011Impl for PL011Luminary {
+    const DEVICE_ID: DeviceId = DeviceId(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
 }
 
 impl DeviceImpl for PL011Luminary {}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 9546e9d7963..93c5637bbc3 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -13,7 +13,7 @@
     cell::{self, BqlCell},
     declare_properties, define_property,
     prelude::*,
-    qdev::{DeviceClass, DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
+    qdev::{DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
     qom::{ClassInitImpl, ObjectImpl, ParentField},
     sysbus::SysBusDevice,
     vmstate::VMStateDescription,
@@ -41,6 +41,12 @@ pub struct DummyClass {
     parent_class: <DeviceState as ObjectType>::Class,
 }
 
+impl DummyClass {
+    pub fn class_init<T: DeviceImpl>(self: &mut DummyClass) {
+        <T as ClassInitImpl<DeviceClass>>::class_init(&mut self.parent_class);
+    }
+}
+
 declare_properties! {
     DUMMY_PROPERTIES,
         define_property!(
@@ -60,7 +66,7 @@ unsafe impl ObjectType for DummyState {
 impl ObjectImpl for DummyState {
     type ParentType = DeviceState;
     const ABSTRACT: bool = false;
-    const CLASS_INIT: fn(&mut DummyClass) = <Self as ClassInitImpl<DummyClass>>::class_init;
+    const CLASS_INIT: fn(&mut DummyClass) = DummyClass::class_init::<Self>;
 }
 
 impl ResettablePhasesImpl for DummyState {}
@@ -74,14 +80,6 @@ fn vmsd() -> Option<&'static VMStateDescription> {
     }
 }
 
-// `impl<T> ClassInitImpl<DummyClass> for T` doesn't work since it violates
-// orphan rule.
-impl ClassInitImpl<DummyClass> for DummyState {
-    fn class_init(klass: &mut DummyClass) {
-        <Self as ClassInitImpl<DeviceClass>>::class_init(&mut klass.parent_class);
-    }
-}
-
 #[derive(qemu_api_macros::offsets)]
 #[repr(C)]
 #[derive(qemu_api_macros::Object)]
@@ -103,22 +101,15 @@ unsafe impl ObjectType for DummyChildState {
 impl ObjectImpl for DummyChildState {
     type ParentType = DummyState;
     const ABSTRACT: bool = false;
-    const CLASS_INIT: fn(&mut DummyChildClass) =
-        <Self as ClassInitImpl<DummyChildClass>>::class_init;
+    const CLASS_INIT: fn(&mut DummyChildClass) = DummyChildClass::class_init::<Self>;
 }
 
 impl ResettablePhasesImpl for DummyChildState {}
 impl DeviceImpl for DummyChildState {}
 
-impl ClassInitImpl<DummyClass> for DummyChildState {
-    fn class_init(klass: &mut DummyClass) {
-        <Self as ClassInitImpl<DeviceClass>>::class_init(&mut klass.parent_class);
-    }
-}
-
-impl ClassInitImpl<DummyChildClass> for DummyChildState {
-    fn class_init(klass: &mut DummyChildClass) {
-        <Self as ClassInitImpl<DummyClass>>::class_init(&mut klass.parent_class);
+impl DummyChildClass {
+    pub fn class_init<T: DeviceImpl>(self: &mut DummyChildClass) {
+        self.parent_class.class_init::<T>();
     }
 }
 
-- 
2.48.1

[PATCH 05/15] rust: qom: get rid of ClassInitImpl

[Paolo Bonzini]

Complete the conversion from the ClassInitImpl trait to class_init() methods.
This will provide more freedom to split the qemu_api crate in separate parts.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs |   6 +-
 rust/hw/timer/hpet/src/hpet.rs   |   4 +-
 rust/qemu-api/src/qdev.rs        |  38 ++++---
 rust/qemu-api/src/qom.rs         | 164 +++++++++++++------------------
 rust/qemu-api/src/sysbus.rs      |  15 ++-
 rust/qemu-api/tests/tests.rs     |   4 +-
 6 files changed, 101 insertions(+), 130 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index fc1c8ec8d6a..6896b1026f6 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -19,8 +19,8 @@
     memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
     prelude::*,
     qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
-    qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
-    sysbus::{SysBusDevice, SysBusDeviceClass, SysBusDeviceImpl},
+    qom::{ObjectImpl, Owned, ParentField},
+    sysbus::{SysBusDevice, SysBusDeviceImpl},
     vmstate::VMStateDescription,
 };
 
@@ -145,7 +145,7 @@ trait PL011Impl: SysBusDeviceImpl + IsA<PL011State> {
 impl PL011Class {
     fn class_init<T: PL011Impl>(&mut self) {
         self.device_id = T::DEVICE_ID;
-        <T as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut self.parent_class);
+        self.parent_class.class_init::<T>();
     }
 }
 
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index e01b4b67064..be27eb0eff4 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -21,7 +21,7 @@
     },
     prelude::*,
     qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
-    qom::{ClassInitImpl, ObjectImpl, ObjectType, ParentField},
+    qom::{ObjectImpl, ObjectType, ParentField},
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL},
@@ -836,7 +836,7 @@ impl ObjectImpl for HPETState {
 
     const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
-    const CLASS_INIT: fn(&mut Self::Class) = <Self as ClassInitImpl<Self::Class>>::class_init;
+    const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
 }
 
 // TODO: Make these properties user-configurable!
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index c4dd26b582c..c136457090c 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -19,7 +19,7 @@
     chardev::Chardev,
     irq::InterruptSource,
     prelude::*,
-    qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned},
+    qom::{ObjectClass, ObjectImpl, Owned},
     vmstate::VMStateDescription,
 };
 
@@ -113,7 +113,7 @@ fn vmsd() -> Option<&'static VMStateDescription> {
 /// # Safety
 ///
 /// This function is only called through the QOM machinery and
-/// used by the `ClassInitImpl<DeviceClass>` trait.
+/// used by `DeviceClass::class_init`.
 /// 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.
@@ -127,43 +127,41 @@ unsafe impl InterfaceType for ResettableClass {
         unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_RESETTABLE_INTERFACE) };
 }
 
-impl<T> ClassInitImpl<ResettableClass> for T
-where
-    T: ResettablePhasesImpl,
-{
-    fn class_init(rc: &mut ResettableClass) {
+impl ResettableClass {
+    /// Fill in the virtual methods of `ResettableClass` based on the
+    /// definitions in the `ResettablePhasesImpl` trait.
+    pub fn class_init<T: ResettablePhasesImpl>(&mut self) {
         if <T as ResettablePhasesImpl>::ENTER.is_some() {
-            rc.phases.enter = Some(rust_resettable_enter_fn::<T>);
+            self.phases.enter = Some(rust_resettable_enter_fn::<T>);
         }
         if <T as ResettablePhasesImpl>::HOLD.is_some() {
-            rc.phases.hold = Some(rust_resettable_hold_fn::<T>);
+            self.phases.hold = Some(rust_resettable_hold_fn::<T>);
         }
         if <T as ResettablePhasesImpl>::EXIT.is_some() {
-            rc.phases.exit = Some(rust_resettable_exit_fn::<T>);
+            self.phases.exit = Some(rust_resettable_exit_fn::<T>);
         }
     }
 }
 
-impl<T> ClassInitImpl<DeviceClass> for T
-where
-    T: ClassInitImpl<ObjectClass> + ClassInitImpl<ResettableClass> + DeviceImpl,
-{
-    fn class_init(dc: &mut DeviceClass) {
+impl DeviceClass {
+    /// Fill in the virtual methods of `DeviceClass` based on the definitions in
+    /// the `DeviceImpl` trait.
+    pub fn class_init<T: DeviceImpl>(&mut self) {
         if <T as DeviceImpl>::REALIZE.is_some() {
-            dc.realize = Some(rust_realize_fn::<T>);
+            self.realize = Some(rust_realize_fn::<T>);
         }
         if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
-            dc.vmsd = vmsd;
+            self.vmsd = vmsd;
         }
         let prop = <T as DeviceImpl>::properties();
         if !prop.is_empty() {
             unsafe {
-                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
+                bindings::device_class_set_props_n(self, prop.as_ptr(), prop.len());
             }
         }
 
-        ResettableClass::interface_init::<T, DeviceState>(dc);
-        <T as ClassInitImpl<ObjectClass>>::class_init(&mut dc.parent_class);
+        ResettableClass::cast::<DeviceState>(self).class_init::<T>();
+        self.parent_class.class_init::<T>();
     }
 }
 
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index d821ac25acc..5488643a2fd 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -40,11 +40,6 @@
 //!   The traits have the appropriate specialization of `IsA<>` as a supertrait,
 //!   for example `IsA<DeviceState>` for `DeviceImpl`.
 //!
-//! * an implementation of [`ClassInitImpl`], for example
-//!   `ClassInitImpl<DeviceClass>`. This fills the vtable in the class struct;
-//!   the source for this is the `*Impl` trait; the associated consts and
-//!   functions if needed are wrapped to map C types into Rust types.
-//!
 //! * a trait for instance methods, for example `DeviceMethods`. This trait is
 //!   automatically implemented for any reference or smart pointer to a device
 //!   instance.  It calls into the vtable provides access across all subclasses
@@ -54,6 +49,48 @@
 //!   This provides access to class-wide functionality that doesn't depend on
 //!   instance data. Like instance methods, these are automatically inherited by
 //!   child classes.
+//!
+//! # Class structures
+//!
+//! Each QOM class that has virtual methods describes them in a
+//! _class struct_.  Class structs include a parent field corresponding
+//! to the vtable of the parent class, all the way up to [`ObjectClass`].
+//!
+//! As mentioned above, virtual methods are defined via traits such as
+//! `DeviceImpl`.  Class structs do not define any trait but, conventionally,
+//! all of them have a `class_init` method to initialize the virtual methods
+//! based on the trait and then call the same method on the superclass.
+//!
+//! ```ignore
+//! impl YourSubclassClass
+//! {
+//!     pub fn class_init<T: YourSubclassImpl>(&mut self) {
+//!         ...
+//!         klass.parent_class::class_init<T>();
+//!     }
+//! }
+//! ```
+//!
+//! If a class implements a QOM interface.  In that case, the function must
+//! contain, for each interface, an extra forwarding call as follows:
+//!
+//! ```ignore
+//! ResettableClass::cast::<Self>(self).class_init::<Self>();
+//! ```
+//!
+//! These `class_init` functions are methods on the class rather than a trait,
+//! because the bound on `T` (`DeviceImpl` in this case), will change for every
+//! class struct.  The functions are pointed to by the
+//! [`ObjectImpl::CLASS_INIT`] function pointer. While there is no default
+//! implementation, in most cases it will be enough to write it as follows:
+//!
+//! ```ignore
+//! const CLASS_INIT: fn(&mut Self::Class)> = Self::Class::class_init::<Self>;
+//! ```
+//!
+//! This design incurs a small amount of code duplication but, by not using
+//! traits, it allows the flexibility of implementing bindings in any crate,
+//! without incurring into violations of orphan rules for traits.
 
 use std::{
     ffi::CStr,
@@ -279,19 +316,25 @@ pub unsafe trait InterfaceType: Sized {
     /// for this interface.
     const TYPE_NAME: &'static CStr;
 
-    /// Initialize the vtable for the interface; the generic argument `T` is the
-    /// type being initialized, while the generic argument `U` is the type that
+    /// Return the vtable for the interface; `U` is the type that
     /// lists the interface in its `TypeInfo`.
     ///
+    /// # Examples
+    ///
+    /// This function is usually called by a `class_init` method in `U::Class`.
+    /// For example, `DeviceClass::class_init<T>` initializes its `Resettable`
+    /// interface as follows:
+    ///
+    /// ```ignore
+    /// ResettableClass::cast::<DeviceState>(self).class_init::<T>();
+    /// ```
+    ///
+    /// where `T` is the concrete subclass that is being initialized.
+    ///
     /// # Panics
     ///
     /// Panic if the incoming argument if `T` does not implement the interface.
-    fn interface_init<
-        T: ObjectType + ClassInitImpl<Self> + ClassInitImpl<U::Class>,
-        U: ObjectType,
-    >(
-        klass: &mut U::Class,
-    ) {
+    fn cast<U: ObjectType>(klass: &mut U::Class) -> &mut Self {
         unsafe {
             // SAFETY: upcasting to ObjectClass is always valid, and the
             // return type is either NULL or the argument itself
@@ -300,8 +343,7 @@ fn interface_init<
                 Self::TYPE_NAME.as_ptr(),
             )
             .cast();
-
-            <T as ClassInitImpl<Self>>::class_init(result.as_mut().unwrap())
+            result.as_mut().unwrap()
         }
     }
 }
@@ -558,87 +600,20 @@ pub trait ObjectImpl: ObjectType + IsA<Object> {
     /// the default values coming from the parent classes; the function
     /// can change them to override virtual methods of a parent class.
     ///
-    /// Usually defined as `<Self as ClassInitImpl<Self::Class>::class_init`.
-    const CLASS_INIT: fn(&mut Self::Class);
-}
-
-/// Internal trait used to automatically fill in a class struct.
-///
-/// Each QOM class that has virtual methods describes them in a
-/// _class struct_.  Class structs include a parent field corresponding
-/// to the vtable of the parent class, all the way up to [`ObjectClass`].
-/// Each QOM type has one such class struct; this trait takes care of
-/// initializing the `T` part of the class struct, for the type that
-/// implements the trait.
-///
-/// Each struct will implement this trait with `T` equal to each
-/// superclass.  For example, a device should implement at least
-/// `ClassInitImpl<`[`DeviceClass`](crate::qdev::DeviceClass)`>` and
-/// `ClassInitImpl<`[`ObjectClass`]`>`.  Such implementations are made
-/// in one of two ways.
-///
-/// For most superclasses, `ClassInitImpl` is provided by the `qemu-api`
-/// crate itself.  The Rust implementation of methods will come from a
-/// trait like [`ObjectImpl`] or [`DeviceImpl`](crate::qdev::DeviceImpl),
-/// and `ClassInitImpl` is provided by blanket implementations that
-/// operate on all implementors of the `*Impl`* trait.  For example:
-///
-/// ```ignore
-/// impl<T> ClassInitImpl<DeviceClass> for T
-/// where
-///     T: ClassInitImpl<ObjectClass> + DeviceImpl,
-/// ```
-///
-/// The bound on `ClassInitImpl<ObjectClass>` is needed so that,
-/// after initializing the `DeviceClass` part of the class struct,
-/// the parent [`ObjectClass`] is initialized as well.
-///
-/// The other case is when manual implementation of the trait is needed.
-/// This covers the following cases:
-///
-/// * if a class implements a QOM interface, the Rust code _has_ to define its
-///   own class struct `FooClass` and implement `ClassInitImpl<FooClass>`.
-///   `ClassInitImpl<FooClass>`'s `class_init` method will then forward to
-///   multiple other `class_init`s, for the interfaces as well as the
-///   superclass. (Note that there is no Rust example yet for using interfaces).
-///
-/// * for classes implemented outside the ``qemu-api`` crate, it's not possible
-///   to add blanket implementations like the above one, due to orphan rules. In
-///   that case, the easiest solution is to implement
-///   `ClassInitImpl<YourSuperclass>` for each subclass and not have a
-///   `YourSuperclassImpl` trait at all.
-///
-/// ```ignore
-/// impl ClassInitImpl<YourSuperclass> for YourSubclass {
-///     fn class_init(klass: &mut YourSuperclass) {
-///         klass.some_method = Some(Self::some_method);
-///         <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
-///     }
-/// }
-/// ```
-///
-///   While this method incurs a small amount of code duplication,
-///   it is generally limited to the recursive call on the last line.
-///   This is because classes defined in Rust do not need the same
-///   glue code that is needed when the classes are defined in C code.
-///   You may consider using a macro if you have many subclasses.
-pub trait ClassInitImpl<T> {
-    /// Initialize `klass` to point to the virtual method implementations
-    /// for `Self`.  On entry, the virtual method pointers are set to
-    /// the default values coming from the parent classes; the function
-    /// can change them to override virtual methods of a parent class.
+    /// Usually defined simply as `Self::Class::class_init::<Self>`;
+    /// however a default implementation cannot be included here, because the
+    /// bounds that the `Self::Class::class_init` method places on `Self` are
+    /// not known in advance.
     ///
-    /// The virtual method implementations usually come from another
-    /// trait, for example [`DeviceImpl`](crate::qdev::DeviceImpl)
-    /// when `T` is [`DeviceClass`](crate::qdev::DeviceClass).
+    /// # Safety
     ///
-    /// On entry, `klass`'s parent class is initialized, while the other fields
+    /// While `klass`'s parent class is initialized on entry, the other fields
     /// are all zero; it is therefore assumed that all fields in `T` can be
     /// zeroed, otherwise it would not be possible to provide the class as a
     /// `&mut T`.  TODO: add a bound of [`Zeroable`](crate::zeroable::Zeroable)
     /// to T; this is more easily done once Zeroable does not require a manual
     /// implementation (Rust 1.75.0).
-    fn class_init(klass: &mut T);
+    const CLASS_INIT: fn(&mut Self::Class);
 }
 
 /// # Safety
@@ -651,13 +626,12 @@ pub trait ClassInitImpl<T> {
     T::UNPARENT.unwrap()(unsafe { state.as_ref() });
 }
 
-impl<T> ClassInitImpl<ObjectClass> for T
-where
-    T: ObjectImpl,
-{
-    fn class_init(oc: &mut ObjectClass) {
+impl ObjectClass {
+    /// Fill in the virtual methods of `ObjectClass` based on the definitions in
+    /// the `ObjectImpl` trait.
+    pub fn class_init<T: ObjectImpl>(&mut self) {
         if <T as ObjectImpl>::UNPARENT.is_some() {
-            oc.unparent = Some(rust_unparent_fn::<T>);
+            self.unparent = Some(rust_unparent_fn::<T>);
         }
     }
 }
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index fee2e3d478f..04821a2b9b3 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -14,8 +14,8 @@
     irq::{IRQState, InterruptSource},
     memory::MemoryRegion,
     prelude::*,
-    qdev::{DeviceClass, DeviceImpl, DeviceState},
-    qom::{ClassInitImpl, Owned},
+    qdev::{DeviceImpl, DeviceState},
+    qom::Owned,
 };
 
 unsafe impl ObjectType for SysBusDevice {
@@ -28,12 +28,11 @@ unsafe impl ObjectType for SysBusDevice {
 // TODO: add virtual methods
 pub trait SysBusDeviceImpl: DeviceImpl + IsA<SysBusDevice> {}
 
-impl<T> ClassInitImpl<SysBusDeviceClass> for T
-where
-    T: SysBusDeviceImpl + ClassInitImpl<DeviceClass>,
-{
-    fn class_init(sdc: &mut SysBusDeviceClass) {
-        <T as ClassInitImpl<DeviceClass>>::class_init(&mut sdc.parent_class);
+impl SysBusDeviceClass {
+    /// Fill in the virtual methods of `SysBusDeviceClass` based on the
+    /// definitions in the `SysBusDeviceImpl` trait.
+    pub fn class_init<T: SysBusDeviceImpl>(self: &mut SysBusDeviceClass) {
+        self.parent_class.class_init::<T>();
     }
 }
 
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 93c5637bbc3..e3985782a38 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -14,7 +14,7 @@
     declare_properties, define_property,
     prelude::*,
     qdev::{DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
-    qom::{ClassInitImpl, ObjectImpl, ParentField},
+    qom::{ObjectImpl, ParentField},
     sysbus::SysBusDevice,
     vmstate::VMStateDescription,
     zeroable::Zeroable,
@@ -43,7 +43,7 @@ pub struct DummyClass {
 
 impl DummyClass {
     pub fn class_init<T: DeviceImpl>(self: &mut DummyClass) {
-        <T as ClassInitImpl<DeviceClass>>::class_init(&mut self.parent_class);
+        self.parent_class.class_init::<T>();
     }
 }
 
-- 
2.48.1

[PATCH 06/15] rust: cell: add wrapper for FFI types

[Paolo Bonzini]

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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/devel/rust.rst       |  34 +++++--
 rust/qemu-api/src/cell.rs | 191 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 210 insertions(+), 15 deletions(-)

diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index e3f9e16aacb..9a621648e72 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -295,15 +295,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..84b9eb07467 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
+//! let state = Opaque::<MyStruct>::uninit();
+//! unsafe {
+//!     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(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,109 @@ 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.
+    /// However, this function is unsafe to "force" documenting who is going
+    /// to initialize and pin the value.
+    pub const 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.
+    /// However, this function is unsafe to "force" documenting whether a
+    /// zero value is safe.
+    pub const unsafe fn zeroed() -> Self {
+        Self {
+            value: UnsafeCell::new(MaybeUninit::uninit()),
+            _pin: PhantomPinned,
+        }
+    }
+
+    /// Returns a raw 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.
+    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> Default for Opaque<T> {
+    fn default() -> Self {
+        Self {
+            value: UnsafeCell::new(MaybeUninit::new(T::default())),
+            _pin: PhantomPinned,
+        }
+    }
+}
+
+impl<T: Default> Opaque<T> {
+    /// Creates a new opaque object with default contents.
+    pub fn new() -> Self {
+        Self::default()
+    }
+}
-- 
2.48.1

[PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro

[Paolo Bonzini]

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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/devel/rust.rst             |  8 ++--
 rust/qemu-api-macros/src/lib.rs | 82 ++++++++++++++++++++++++++++++++-
 rust/qemu-api/meson.build       |  7 +--
 rust/qemu-api/src/cell.rs       | 31 +++++++++++++
 4 files changed, 119 insertions(+), 9 deletions(-)

diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 9a621648e72..db2b427ebd2 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -314,11 +314,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..781e5271562 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,44 @@ 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()
+            }
+        }
+    })
+}
+
+#[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 84b9eb07467..c39b9616969 100644
--- a/rust/qemu-api/src/cell.rs
+++ b/rust/qemu-api/src/cell.rs
@@ -1017,3 +1017,34 @@ pub fn new() -> Self {
         Self::default()
     }
 }
+
+/// 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

[PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<>

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                |  7 -------
 rust/qemu-api/src/timer.rs | 24 +++++++++++++++++-------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/meson.build b/meson.build
index 8ed10b6624e..16c76c493f3 100644
--- a/meson.build
+++ b/meson.build
@@ -4087,13 +4087,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/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..0305a0385ad 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -7,10 +7,23 @@
 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, Default, 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;
@@ -21,10 +34,6 @@ 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,
         timer_list_group: Option<&TimerListGroup>,
@@ -51,7 +60,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 {
@@ -75,6 +84,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

[PATCH 09/15] rust: irq: wrap IRQState with Opaque<>

[Paolo Bonzini]

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 d1c9dc96eff..aec2825b2f9 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -9,10 +9,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.
@@ -22,8 +28,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
@@ -41,7 +46,7 @@ pub struct InterruptSource<T = bool>
 where
     c_int: From<T>,
 {
-    cell: BqlCell<*mut IRQState>,
+    cell: BqlCell<*mut bindings::IRQState>,
     _marker: PhantomData<T>,
 }
 
@@ -80,11 +85,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

[PATCH 10/15] rust: qom: wrap Object with Opaque<>

[Paolo Bonzini]

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..0bca36336ba 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 obj = object_new(Self::TYPE_NAME.as_ptr());
+            let obj = Object::from_raw(obj).unsafe_cast::<Self>();
+            Owned::from_raw(obj)
         }
     }
 }
-- 
2.48.1

[PATCH 11/15] rust: qdev: wrap Clock and DeviceState with Opaque<>

[Paolo Bonzini]

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..ed5dce08216 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.0.as_mut_ptr(),
                     cstr.as_ptr(),
                     cb,
-                    dev.cast::<c_void>(),
+                    dev.0.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 24a4dc81e7f..b115d1f8742 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -469,7 +469,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

[PATCH 12/15] rust: sysbus: wrap SysBusDevice with Opaque<>

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/hpet.rs |  2 +-
 rust/qemu-api/src/bindings.rs  |  3 ---
 rust/qemu-api/src/sysbus.rs    | 25 ++++++++++++++++++-------
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..19e63465cff 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -741,7 +741,7 @@ fn reset_hold(&self, _type: ResetType) {
         HPETFwConfig::update_hpet_cfg(
             self.hpet_id.get(),
             self.capability.get() as u32,
-            sbd.mmio[0].addr,
+            unsafe { *sbd.as_ptr() }.mmio[0].addr,
         );
 
         // to document that the RTC lowers its output on reset as well
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 48803a655f9..78909fb9931 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,7 +68,7 @@ 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());
         }
     }
 
@@ -69,7 +77,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);
         }
     }
 
@@ -81,7 +89,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());
         }
     }
 
@@ -89,7 +97,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

[PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/bindings.rs |  3 ---
 rust/qemu-api/src/memory.rs   | 30 ++++++++++++++++--------------
 2 files changed, 16 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..fdb1ea11fcf 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,15 @@ 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(),
+                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

[PATCH 14/15] rust: chardev: wrap Chardev with Opaque<>

[Paolo Bonzini]

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 ed5dce08216..1ff6c1ca7c2 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

[PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls

[Paolo Bonzini]

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.

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

Re: [PATCH 01/15] rust: add IsA bounds to QOM implementation traits

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:28PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:28 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 01/15] rust: add IsA bounds to QOM implementation traits
> X-Mailer: git-send-email 2.48.1
> 
> Check that the right bounds are provided to the qom_isa! macro
> whenever the class is defined to implement a certain class.
> This removes the need to add IsA<> bounds together with the
> *Impl trait bounds.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/qdev.rs | 2 +-
>  rust/qemu-api/src/qom.rs  | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 02/15] rust: add SysBusDeviceImpl

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:29PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:29 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/15] rust: add SysBusDeviceImpl
> X-Mailer: git-send-email 2.48.1
> 
> The only function, right now, is to ensure that anything with a
> SysBusDeviceClass class is a SysBusDevice.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 5 ++++-
>  rust/hw/timer/hpet/src/hpet.rs   | 4 +++-
>  rust/qemu-api/src/sysbus.rs      | 8 +++++---
>  3 files changed, 12 insertions(+), 5 deletions(-)>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:30PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:30 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT
> X-Mailer: git-send-email 2.48.1
> 
> As shown in the PL011 device, the orphan rules required a manual
> implementation of ClassInitImpl for anything not in the qemu_api crate;
> this gets in the way of moving system emulation-specific code (including
> DeviceClass, which as a blanket ClassInitImpl<DeviceClass> implementation)
> into its own crate.
> 
> Make ClassInitImpl optional, at the cost of having to specify the CLASS_INIT
> member by hand in every implementation of ObjectImpl.  The next commits will
> get rid of it, replacing all the "impl<T> ClassInitImpl<Class> for T" blocks
> with a generic class_init<T> method on Class.
> 
> Right now the definition is always the same, but do not provide a default
> as that will not be true once ClassInitImpl goes away.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs |  3 +++
>  rust/hw/timer/hpet/src/hpet.rs   |  3 ++-
>  rust/qemu-api/src/qom.rs         | 14 +++++++++++---
>  rust/qemu-api/tests/tests.rs     |  3 +++
>  4 files changed, 19 insertions(+), 4 deletions(-)> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:31PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:31 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl
> X-Mailer: git-send-email 2.48.1
> 
> Outside the qemu_api crate, orphan rules make the usage of ClassInitImpl
> unwieldy.  Now that it is optional, do not use it.
> 
> For PL011Class, this makes it easier to provide a PL011Impl trait similar
> to the ones in the qemu_api crate.  The device id consts are moved there.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 38 ++++++++++++++++----------------
>  rust/qemu-api/tests/tests.rs     | 33 ++++++++++-----------------
>  2 files changed, 31 insertions(+), 40 deletions(-)>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 05/15] rust: qom: get rid of ClassInitImpl

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:32PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:32 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 05/15] rust: qom: get rid of ClassInitImpl
> X-Mailer: git-send-email 2.48.1
> 
> Complete the conversion from the ClassInitImpl trait to class_init() methods.
> This will provide more freedom to split the qemu_api crate in separate parts.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs |   6 +-
>  rust/hw/timer/hpet/src/hpet.rs   |   4 +-
>  rust/qemu-api/src/qdev.rs        |  38 ++++---
>  rust/qemu-api/src/qom.rs         | 164 +++++++++++++------------------
>  rust/qemu-api/src/sysbus.rs      |  15 ++-
>  rust/qemu-api/tests/tests.rs     |   4 +-
>  6 files changed, 101 insertions(+), 130 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 06/15] rust: cell: add wrapper for FFI types

[Zhao Liu]

> +    /// 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.
> +    /// However, this function is unsafe to "force" documenting whether a
> +    /// zero value is safe.
> +    pub const unsafe fn zeroed() -> Self {
> +        Self {
> +            value: UnsafeCell::new(MaybeUninit::uninit()),

typo? MaybeUninit::zeroed()

> +            _pin: PhantomPinned,
> +        }
> +    }
> +
> +    /// Returns a raw pointer to the opaque data.

Maybe "a raw mutable pointer".

> +    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.

What about "Returns a raw c_void type pointer"?

(Because the descriptions of these three helpers are exactly the same!
We can make them different.)
zeroed
> +    pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
> +        UnsafeCell::get(&self.value).cast()
> +    }
> +}

The whole idea is great! With the nit of zeroed fixed,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro

[Zhao Liu]

> +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()
> +            }

What about also adding as_void_ptr? Then DeviceState can benefit from
this in qdev_init_clock_in.

> +        }
> +    })
> +}
> +

Others are fine for me. Nice improvement!

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<>

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:35PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:35 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<>
> X-Mailer: git-send-email 2.48.1
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                |  7 -------
>  rust/qemu-api/src/timer.rs | 24 +++++++++++++++++-------
>  2 files changed, 17 insertions(+), 14 deletions(-)

Thanks!

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>

[Zhao Liu]

> +/// 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.
> @@ -22,8 +28,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
> @@ -41,7 +46,7 @@ pub struct InterruptSource<T = bool>
>  where
>      c_int: From<T>,
>  {
> -    cell: BqlCell<*mut IRQState>,
> +    cell: BqlCell<*mut bindings::IRQState>,

Once we've already wrapper IRQState in Opaque<>, should we still use
bindings::IRQState?

Although InterruptSource just stores a pointer. However, I think we can
use wrapped IRQState here instead of the native binding type, since this
item is also crossing the FFI boundary. What do you think?

>      _marker: PhantomData<T>,
>  }
>  

Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>

[Paolo Bonzini]

On 2/25/25 09:26, Zhao Liu wrote:
>> +/// 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.
>> @@ -22,8 +28,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
>> @@ -41,7 +46,7 @@ pub struct InterruptSource<T = bool>
>>   where
>>       c_int: From<T>,
>>   {
>> -    cell: BqlCell<*mut IRQState>,
>> +    cell: BqlCell<*mut bindings::IRQState>,
> 
> Once we've already wrapper IRQState in Opaque<>, should we still use
> bindings::IRQState?
> 
> Although InterruptSource just stores a pointer. However, I think we can
> use wrapped IRQState here instead of the native binding type, since this
> item is also crossing the FFI boundary. What do you think?

Using the wrapped IRQState would be a bit more code because you have to 
cast the pointer all the time when calling C code.  As far as 
correctness is concerned, it does not really matter because as you said 
it only stores a pointer.

However, if needed, InterruptSource could have a function that converts 
from IRQState to Option<&Opaque<irq::IRQState>>.  Since the accessor is 
needed anyway, that would be a good place to put the conversion.

Paolo

Re: [PATCH 10/15] rust: qom: wrap Object with Opaque<>

[Zhao Liu]

> @@ -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 obj = object_new(Self::TYPE_NAME.as_ptr());

Having the same name is always not ideal, so let's name the first one raw_obj.

> +            let obj = Object::from_raw(obj).unsafe_cast::<Self>();
> +            Owned::from_raw(obj)
>          }
>      }
>  }

Others look good to me,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 

Re: [PATCH 11/15] rust: qdev: wrap Clock and DeviceState with Opaque<>

[Zhao Liu]

> -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.0.as_mut_ptr(),

This can be simplfied as dev.as_mut_ptr().

>                      cstr.as_ptr(),
>                      cb,
> -                    dev.cast::<c_void>(),
> +                    dev.0.as_void_ptr(),

If Wrapper provides as_void_ptr(), then this can also be simplified.

>                      events.0,
>                  );
>  
> -                Owned::from(&*clk)
> +                let clk: &Clock = Clock::from_raw(clk);
> +                Owned::from(clk)
>              }
>          }

LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 12/15] rust: sysbus: wrap SysBusDevice with Opaque<>

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:39PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:39 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 12/15] rust: sysbus: wrap SysBusDevice with Opaque<>
> X-Mailer: git-send-email 2.48.1
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/hpet.rs |  2 +-
>  rust/qemu-api/src/bindings.rs  |  3 ---
>  rust/qemu-api/src/sysbus.rs    | 25 ++++++++++++++++++-------
>  3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
> index be27eb0eff4..19e63465cff 100644
> --- a/rust/hw/timer/hpet/src/hpet.rs
> +++ b/rust/hw/timer/hpet/src/hpet.rs
> @@ -741,7 +741,7 @@ fn reset_hold(&self, _type: ResetType) {
>          HPETFwConfig::update_hpet_cfg(
>              self.hpet_id.get(),
>              self.capability.get() as u32,
> -            sbd.mmio[0].addr,
> +            unsafe { *sbd.as_ptr() }.mmio[0].addr,

We can wrap this unsafe code into SysBusDeviceMethods...then the device
won't introduce more unsafe code.

>          );
>  
>          // to document that the RTC lowers its output on reset as well

Others, fine for me,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>

[Zhao Liu]

>  impl MemoryRegion {
>      // inline to ensure that it is not included in tests, which only
> @@ -174,13 +174,15 @@ 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(),

I'm not sure why the wrapper doesn't work here.

If I change this to `self.as_mut_ptr()`, then compiler tries to apply
`ObjectDeref::as_mut_ptr` and complains:

the trait `qom::ObjectType` is not implemented for `bindings::MemoryRegion`


But when I modify the function signature to &self like:

diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index fdb1ea11fcf9..a82348c4a564 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -167,7 +167,7 @@ unsafe fn do_init_io(
     }

     pub fn init_io<T: IsA<Object>>(
-        &mut self,
+        &self,
         owner: *mut T,
         ops: &'static MemoryRegionOps<T>,
         name: &'static str,

Then the Wrapper's as_mut_ptr() can work.

> +                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
> 
> 

Re: [PATCH 14/15] rust: chardev: wrap Chardev with Opaque<>

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:41PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:41 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 14/15] rust: chardev: wrap Chardev 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/chardev.rs  | 8 ++++++--
>  rust/qemu-api/src/qdev.rs     | 1 +
>  3 files changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls

[Zhao Liu]

On Fri, Feb 21, 2025 at 06:03:42PM +0100, Paolo Bonzini wrote:
> Date: Fri, 21 Feb 2025 18:03:42 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync
>  impls
> X-Mailer: git-send-email 2.48.1
> 
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/bindings.rs | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>

[Zhao Liu]

On Tue, Feb 25, 2025 at 09:28:52AM +0100, Paolo Bonzini wrote:
> Date: Tue, 25 Feb 2025 09:28:52 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>
> 
> On 2/25/25 09:26, Zhao Liu wrote:
> > > +/// 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.
> > > @@ -22,8 +28,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
> > > @@ -41,7 +46,7 @@ pub struct InterruptSource<T = bool>
> > >   where
> > >       c_int: From<T>,
> > >   {
> > > -    cell: BqlCell<*mut IRQState>,
> > > +    cell: BqlCell<*mut bindings::IRQState>,
> > 
> > Once we've already wrapper IRQState in Opaque<>, should we still use
> > bindings::IRQState?
> > 
> > Although InterruptSource just stores a pointer. However, I think we can
> > use wrapped IRQState here instead of the native binding type, since this
> > item is also crossing the FFI boundary. What do you think?
> 
> Using the wrapped IRQState would be a bit more code because you have to cast
> the pointer all the time when calling C code.  As far as correctness is
> concerned, it does not really matter because as you said it only stores a
> pointer.

Yes, it makes sense. This conversion doesn't block the current patch. The
correctness has been guaranteed.

> However, if needed, InterruptSource could have a function that converts from
> IRQState to Option<&Opaque<irq::IRQState>>.  Since the accessor is needed
> anyway, that would be a good place to put the conversion. 

Then I understand we still need `assert!(bql_locked())` in assessor as
the doc said: "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."

Re: [PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>

[Paolo Bonzini]

On 2/25/25 10:14, Zhao Liu wrote:
>>   impl MemoryRegion {
>>       // inline to ensure that it is not included in tests, which only
>> @@ -174,13 +174,15 @@ 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(),
> 
> I'm not sure why the wrapper doesn't work here.
> 
> If I change this to `self.as_mut_ptr()`, then compiler tries to apply
> `ObjectDeref::as_mut_ptr` and complains:
> 
> the trait `qom::ObjectType` is not implemented for `bindings::MemoryRegion`

It's because the implementation of ObjectDeref for "&mut MemoryRegion" 
wins over coercing "&mut MemoryRegion" to "&MemoryRegion" and then 
calling MemoryRegion::as_mut_ptr().

I added a comment

         // 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.

Paolo

> 
> But when I modify the function signature to &self like:
> 
> diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
> index fdb1ea11fcf9..a82348c4a564 100644
> --- a/rust/qemu-api/src/memory.rs
> +++ b/rust/qemu-api/src/memory.rs
> @@ -167,7 +167,7 @@ unsafe fn do_init_io(
>       }
> 
>       pub fn init_io<T: IsA<Object>>(
> -        &mut self,
> +        &self,
>           owner: *mut T,
>           ops: &'static MemoryRegionOps<T>,
>           name: &'static str,
> 
> Then the Wrapper's as_mut_ptr() can work.
> 
>> +                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
>>
>>
> 
>