* [PATCH 01/15] rust: add IsA bounds to QOM implementation traits
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-24 14:43 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 02/15] rust: add SysBusDeviceImpl Paolo Bonzini
` (13 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 01/15] rust: add IsA bounds to QOM implementation traits
2025-02-21 17:03 ` [PATCH 01/15] rust: add IsA bounds to QOM implementation traits Paolo Bonzini
@ 2025-02-24 14:43 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-24 14:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 02/15] rust: add SysBusDeviceImpl
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
2025-02-21 17:03 ` [PATCH 01/15] rust: add IsA bounds to QOM implementation traits Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-24 14:46 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT Paolo Bonzini
` (12 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 02/15] rust: add SysBusDeviceImpl
2025-02-21 17:03 ` [PATCH 02/15] rust: add SysBusDeviceImpl Paolo Bonzini
@ 2025-02-24 14:46 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-24 14:46 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
2025-02-21 17:03 ` [PATCH 01/15] rust: add IsA bounds to QOM implementation traits Paolo Bonzini
2025-02-21 17:03 ` [PATCH 02/15] rust: add SysBusDeviceImpl Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-24 14:56 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl Paolo Bonzini
` (11 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT
2025-02-21 17:03 ` [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT Paolo Bonzini
@ 2025-02-24 14:56 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-24 14:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (2 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 03/15] rust: qom: add ObjectImpl::CLASS_INIT Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-24 15:14 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 05/15] rust: qom: get rid of ClassInitImpl Paolo Bonzini
` (10 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl
2025-02-21 17:03 ` [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl Paolo Bonzini
@ 2025-02-24 15:14 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-24 15:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 05/15] rust: qom: get rid of ClassInitImpl
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (3 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 04/15] rust: pl011, qemu_api tests: do not use ClassInitImpl Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-24 15:24 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 06/15] rust: cell: add wrapper for FFI types Paolo Bonzini
` (9 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 05/15] rust: qom: get rid of ClassInitImpl
2025-02-21 17:03 ` [PATCH 05/15] rust: qom: get rid of ClassInitImpl Paolo Bonzini
@ 2025-02-24 15:24 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-24 15:24 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 06/15] rust: cell: add wrapper for FFI types
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (4 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 05/15] rust: qom: get rid of ClassInitImpl Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 6:46 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
` (8 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 06/15] rust: cell: add wrapper for FFI types
2025-02-21 17:03 ` [PATCH 06/15] rust: cell: add wrapper for FFI types Paolo Bonzini
@ 2025-02-25 6:46 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 6:46 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> + /// 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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (5 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 06/15] rust: cell: add wrapper for FFI types Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 7:54 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<> Paolo Bonzini
` (7 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro
2025-02-21 17:03 ` [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
@ 2025-02-25 7:54 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 7:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> +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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (6 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 07/15] rust: qemu_api_macros: add Wrapper derive macro Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 7:56 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 09/15] rust: irq: wrap IRQState " Paolo Bonzini
` (6 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (7 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 08/15] rust: timer: wrap QEMUTimer with Opaque<> Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 8:26 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 10/15] rust: qom: wrap Object " Paolo Bonzini
` (5 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>
2025-02-21 17:03 ` [PATCH 09/15] rust: irq: wrap IRQState " Paolo Bonzini
@ 2025-02-25 8:26 ` Zhao Liu
2025-02-25 8:28 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 8:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> +/// 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>,
> }
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>
2025-02-25 8:26 ` Zhao Liu
@ 2025-02-25 8:28 ` Paolo Bonzini
2025-02-25 9:36 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-25 8:28 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 09/15] rust: irq: wrap IRQState with Opaque<>
2025-02-25 8:28 ` Paolo Bonzini
@ 2025-02-25 9:36 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 9:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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."
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 10/15] rust: qom: wrap Object with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (8 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 09/15] rust: irq: wrap IRQState " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 8:47 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 11/15] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
` (4 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 10/15] rust: qom: wrap Object with Opaque<>
2025-02-21 17:03 ` [PATCH 10/15] rust: qom: wrap Object " Paolo Bonzini
@ 2025-02-25 8:47 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 8:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> @@ -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>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 11/15] rust: qdev: wrap Clock and DeviceState with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (9 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 10/15] rust: qom: wrap Object " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 8:52 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 12/15] rust: sysbus: wrap SysBusDevice " Paolo Bonzini
` (3 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 11/15] rust: qdev: wrap Clock and DeviceState with Opaque<>
2025-02-21 17:03 ` [PATCH 11/15] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
@ 2025-02-25 8:52 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 8:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> -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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 12/15] rust: sysbus: wrap SysBusDevice with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (10 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 11/15] rust: qdev: wrap Clock and DeviceState " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 8:59 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 13/15] rust: memory: wrap MemoryRegion " Paolo Bonzini
` (2 subsequent siblings)
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 12/15] rust: sysbus: wrap SysBusDevice with Opaque<>
2025-02-21 17:03 ` [PATCH 12/15] rust: sysbus: wrap SysBusDevice " Paolo Bonzini
@ 2025-02-25 8:59 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 8:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (11 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 12/15] rust: sysbus: wrap SysBusDevice " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 9:14 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 14/15] rust: chardev: wrap Chardev " Paolo Bonzini
2025-02-21 17:03 ` [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-21 17:03 ` [PATCH 13/15] rust: memory: wrap MemoryRegion " Paolo Bonzini
@ 2025-02-25 9:14 ` Zhao Liu
2025-02-25 9:47 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 9:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> 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
>
>
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 13/15] rust: memory: wrap MemoryRegion with Opaque<>
2025-02-25 9:14 ` Zhao Liu
@ 2025-02-25 9:47 ` Paolo Bonzini
0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-25 9:47 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
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
>>
>>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 14/15] rust: chardev: wrap Chardev with Opaque<>
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (12 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 13/15] rust: memory: wrap MemoryRegion " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 9:19 ` Zhao Liu
2025-02-21 17:03 ` [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 14/15] rust: chardev: wrap Chardev with Opaque<>
2025-02-21 17:03 ` [PATCH 14/15] rust: chardev: wrap Chardev " Paolo Bonzini
@ 2025-02-25 9:19 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 9:19 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls
2025-02-21 17:03 [PATCH 00/15] rust: prepare for splitting crates Paolo Bonzini
` (13 preceding siblings ...)
2025-02-21 17:03 ` [PATCH 14/15] rust: chardev: wrap Chardev " Paolo Bonzini
@ 2025-02-21 17:03 ` Paolo Bonzini
2025-02-25 9:20 ` Zhao Liu
14 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-21 17:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls
2025-02-21 17:03 ` [PATCH 15/15] rust: bindings: remove more unnecessary Send/Sync impls Paolo Bonzini
@ 2025-02-25 9:20 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-25 9:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 34+ messages in thread