* [PATCH 01/10] rust: qemu-api: add sub-subclass to the integration tests
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-01-20 16:40 ` Zhao Liu
2025-01-17 19:39 ` [PATCH 02/10] rust: qom: add reference counting functionality Paolo Bonzini
` (9 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
missing signed-off-by from zhao
---
rust/qemu-api/tests/tests.rs | 56 ++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 526c3f4f8ea..5c3e75ed3d5 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -14,8 +14,8 @@
cell::{self, BqlCell},
declare_properties, define_property,
prelude::*,
- qdev::{DeviceImpl, DeviceState, Property},
- qom::{ObjectImpl, ParentField},
+ qdev::{DeviceClass, DeviceImpl, DeviceState, Property},
+ qom::{ClassInitImpl, ObjectImpl, ParentField},
vmstate::VMStateDescription,
zeroable::Zeroable,
};
@@ -37,6 +37,10 @@ pub struct DummyState {
qom_isa!(DummyState: Object, DeviceState);
+pub struct DummyClass {
+ parent_class: <DeviceState as ObjectType>::Class,
+}
+
declare_properties! {
DUMMY_PROPERTIES,
define_property!(
@@ -49,7 +53,7 @@ pub struct DummyState {
}
unsafe impl ObjectType for DummyState {
- type Class = <DeviceState as ObjectType>::Class;
+ type Class = DummyClass;
const TYPE_NAME: &'static CStr = c_str!("dummy");
}
@@ -67,6 +71,51 @@ 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)]
+pub struct DummyChildState {
+ parent: ParentField<DummyState>,
+}
+
+qom_isa!(DummyChildState: Object, DeviceState, DummyState);
+
+pub struct DummyChildClass {
+ parent_class: <DummyState as ObjectType>::Class,
+}
+
+unsafe impl ObjectType for DummyChildState {
+ type Class = DummyChildClass;
+ const TYPE_NAME: &'static CStr = c_str!("dummy_child");
+}
+
+impl ObjectImpl for DummyChildState {
+ type ParentType = DummyState;
+ const ABSTRACT: bool = false;
+}
+
+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);
+ }
+}
+
fn init_qom() {
static ONCE: BqlCell<bool> = BqlCell::new(false);
@@ -85,6 +134,7 @@ fn test_object_new() {
init_qom();
unsafe {
object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast());
+ object_unref(object_new(DummyChildState::TYPE_NAME.as_ptr()).cast());
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
2025-01-17 19:39 ` [PATCH 01/10] rust: qemu-api: add sub-subclass to the integration tests Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-01-26 15:15 ` Zhao Liu
` (2 more replies)
2025-01-17 19:39 ` [PATCH 03/10] rust: qom: add object creation functionality Paolo Bonzini
` (8 subsequent siblings)
10 siblings, 3 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Add a smart pointer that allows to add and remove references from
QOM objects. It's important to note that while all QOM objects have a
reference count, in practice not all of them have their lifetime guarded
by it. Embedded objects, specifically, are confined to the lifetime of
the owner.
When writing Rust bindings this is important, because embedded objects are
*never* used through the "Owned<>" smart pointer that is introduced here.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/qom.rs | 121 ++++++++++++++++++++++++++++++++++-
rust/qemu-api/src/vmstate.rs | 6 +-
2 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 97901fb9084..c404f8a1aa7 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -56,13 +56,21 @@
use std::{
ffi::CStr,
fmt,
+ mem::ManuallyDrop,
ops::{Deref, DerefMut},
os::raw::c_void,
+ ptr::NonNull,
};
pub use bindings::{Object, ObjectClass};
-use crate::bindings::{self, object_dynamic_cast, object_get_class, object_get_typename, TypeInfo};
+use crate::{
+ bindings::{
+ self, object_dynamic_cast, object_get_class, object_get_typename, object_ref, object_unref,
+ TypeInfo,
+ },
+ cell::bql_locked,
+};
/// Marker trait: `Self` can be statically upcasted to `P` (i.e. `P` is a direct
/// or indirect parent of `Self`).
@@ -605,6 +613,110 @@ unsafe impl ObjectType for Object {
unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_OBJECT) };
}
+/// A reference-counted pointer to a QOM object.
+///
+/// `Owned<T>` wraps `T` with automatic reference counting. It increases the
+/// reference count when created via [`Owned::from`] and decreases it when
+/// dropped. This ensures that the reference count remains elevated as long as
+/// any `Owned<T>` references to it exist. Note that however the object may
+/// disappear if it is *embedded* within a larger object. For more information
+/// see the [`Owned::from`].
+///
+/// It is *not* safe to send `Owned<T>` to another thread, because currently
+/// `object_unref` requires the Big QEMU Lock.
+#[repr(transparent)]
+#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct Owned<T: ObjectType>(NonNull<T>);
+
+// SAFETY: the implementation asserts via `bql_locked` that the BQL is taken
+unsafe impl<T: ObjectType + Sync> Sync for Owned<T> {}
+
+impl<T: ObjectType> Owned<T> {
+ /// Convert a raw C pointer into an owned reference to the QOM
+ /// object it points to. The object's reference count will be
+ /// decreased when the `Owned` is dropped.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `ptr` is NULL.
+ ///
+ /// # Safety
+ ///
+ /// The caller must indeed own a reference to the QOM object.
+ /// The object must not be embedded in another unless the outer
+ /// object is guaranteed to have a longer lifetime.
+ ///
+ /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
+ /// back to `from_raw()` (assuming the original `Owned` was valid!),
+ /// since the owned reference remains there between the calls to
+ /// `into_raw()` and `from_raw()`.
+ #[allow(clippy::missing_const_for_fn)]
+ pub unsafe fn from_raw(ptr: *const T) -> Self {
+ // SAFETY NOTE: while NonNull requires a mutable pointer, only
+ // Deref is implemented so the pointer passed to from_raw
+ // remains const
+ Owned(NonNull::new(ptr as *mut T).unwrap())
+ }
+
+ /// Obtain a raw C pointer from a reference. `src` is consumed
+ /// and the reference is leaked.
+ #[allow(clippy::missing_const_for_fn)]
+ pub fn into_raw(src: Owned<T>) -> *mut T {
+ let src = ManuallyDrop::new(src);
+ src.0.as_ptr()
+ }
+
+ /// Increase the reference count of a QOM object and return
+ /// a new owned reference to it.
+ ///
+ /// # Safety
+ ///
+ /// The object must not be embedded in another, unless the outer
+ /// object is guaranteed to have a longer lifetime.
+ pub unsafe fn from(obj: &T) -> Self {
+ unsafe {
+ object_ref(obj.as_object_mut_ptr().cast::<c_void>());
+
+ // SAFETY NOTE: while NonNull requires a mutable pointer, only
+ // Deref is implemented so the reference passed to from_raw
+ // remains shared
+ Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
+ }
+ }
+}
+
+impl<T: ObjectType> Deref for Owned<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: creation method is unsafe; whoever calls it has
+ // responsibility that the pointer is valid, and remains valid
+ // throughout the lifetime of the `Owned<T>`.
+ // With that guarantee, reference counting ensures that
+ // the object remains alive.
+ unsafe { &*self.0.as_ptr() }
+ }
+}
+impl<T: ObjectType> ObjectDeref for Owned<T> {}
+
+impl<T: ObjectType> Drop for Owned<T> {
+ fn drop(&mut self) {
+ assert!(bql_locked());
+ // SAFETY: creation method is unsafe, and whoever calls it has
+ // responsibility that the pointer is valid, and remains valid
+ // throughout the lifetime of the `Owned<T>`.
+ unsafe {
+ object_unref(self.as_object_mut_ptr().cast::<c_void>());
+ }
+ }
+}
+
+impl<T: IsA<Object>> fmt::Debug for Owned<T> {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ self.deref().debug_fmt(f)
+ }
+}
+
/// Trait for methods exposed by the Object class. The methods can be
/// called on all objects that have the trait `IsA<Object>`.
///
@@ -636,6 +748,13 @@ fn get_class(&self) -> &'static <Self::Target as ObjectType>::Class {
klass
}
+
+ /// Convenience function for implementing the Debug trait
+ fn debug_fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ f.debug_tuple(&self.typename())
+ .field(&(self as *const Self))
+ .finish()
+ }
}
impl<R: ObjectDeref> ObjectMethods for R where R::Target: IsA<Object> {}
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 6ac432cf52f..11d21b8791c 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -29,6 +29,8 @@
pub use crate::bindings::{VMStateDescription, VMStateField};
use crate::{
bindings::{self, VMStateFlags},
+ prelude::*,
+ qom::Owned,
zeroable::Zeroable,
};
@@ -191,7 +193,8 @@ pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateFlags
/// * a transparent wrapper for any of the above (`Cell`, `UnsafeCell`,
/// [`BqlCell`](crate::cell::BqlCell), [`BqlRefCell`](crate::cell::BqlRefCell)
/// * a raw pointer to any of the above
-/// * a `NonNull` pointer or a `Box` for any of the above
+/// * a `NonNull` pointer, a `Box` or an [`Owned`](crate::qom::Owned) for any of
+/// the above
/// * an array of any of the above
///
/// In order to support other types, the trait `VMState` must be implemented
@@ -398,6 +401,7 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
// Unlike C pointers, Box is always non-null therefore there is no need
// to specify VMS_ALLOC.
impl_vmstate_pointer!(Box<T> where T: VMState);
+impl_vmstate_pointer!(Owned<T> where T: VMState + ObjectType);
// Arrays using the underlying type's VMState plus
// VMS_ARRAY/VMS_ARRAY_OF_POINTER
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-17 19:39 ` [PATCH 02/10] rust: qom: add reference counting functionality Paolo Bonzini
@ 2025-01-26 15:15 ` Zhao Liu
2025-01-29 10:03 ` Paolo Bonzini
2025-01-27 7:57 ` Zhao Liu
2025-02-06 3:26 ` Zhao Liu
2 siblings, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-01-26 15:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Hi Paolo,
On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> X-Mailer: git-send-email 2.47.1
>
> Add a smart pointer that allows to add and remove references from
> QOM objects. It's important to note that while all QOM objects have a
> reference count, in practice not all of them have their lifetime guarded
> by it.
About the background, I have a maybe common question...why Rust needs
extra reference count guarding?
For C side, I notice for child objects, which may be totally embedded in
parent object, or may be pointed to by a pointer member in parent object
(like pl011's clock), they usually become the Child<> property of their
parents by object_initialize_child() (for embedded child) or
object_property_add_child() (for child pointer).
And both these 2 interfaces will increase the ref count in
object_property_try_add_child(). With ref count increasing, it seems
that the Child<> property also express the meaning like "the child
object is 'owned' by its parent".
So, what are the benefits of `Owned` when we also creates Child<>
relationship?
Additionally, I felt that the ref count may be a bit confusing. After
creating Child<> property, the child object's ref count is sometimes 1,
and other times it's 2:
* With object_initialize_child(), child's ref count is 1.
* With object_property_add_child() (usually after a object_new() to
create child first):
- sometimes user will call object_unref(), and then the ref count is 1.
E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
- sometimes no object_unref(), then ref count is 2.
E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".
> Embedded objects, specifically, are confined to the lifetime of
> the owner.
>
> When writing Rust bindings this is important, because embedded objects are
> *never* used through the "Owned<>" smart pointer that is introduced here.
From this description, I understand your goal is:
* For embedded child object, its lifetimer is managed by its parent
object, through Child<> for the most cases.
* For non-embedded child - a pointer/reference in parent object, its
lifetimer is managed by `Owned<>` (and with Child<>).
Am I right?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-26 15:15 ` Zhao Liu
@ 2025-01-29 10:03 ` Paolo Bonzini
2025-02-05 8:28 ` Zhao Liu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:03 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On Sun, Jan 26, 2025 at 3:56 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi Paolo,
>
> On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> > Date: Fri, 17 Jan 2025 20:39:55 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> > X-Mailer: git-send-email 2.47.1
> >
> > Add a smart pointer that allows to add and remove references from
> > QOM objects. It's important to note that while all QOM objects have a
> > reference count, in practice not all of them have their lifetime guarded
> > by it.
>
> About the background, I have a maybe common question...why Rust needs
> extra reference count guarding?
Children properties are removed, and thus their reference is dropped,
before instance_finalize() is called (see object_finalize() in
qom/object.c). This is not valid in Rust, you need to keep the object
alive until the last line of Rust code has run - which is after
Drop::drop() has run.
> Additionally, I felt that the ref count may be a bit confusing. After
> creating Child<> property, the child object's ref count is sometimes 1,
> and other times it's 2:
>
> * With object_initialize_child(), child's ref count is 1.
>
> * With object_property_add_child() (usually after a object_new() to
> create child first):
>
> - sometimes user will call object_unref(), and then the ref count is 1.
> E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
>
> - sometimes no object_unref(), then ref count is 2.
> E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".
In C, having a ref count of 2 is usually a latent memory leak (because
most of the time there's not going to be an object_unref() in the
instance_finalize() method). In this case the leak is latent, because
TYPE_EXYNOS4210_SOC is not hot-unpluggable and thus will never really
go away once realized.
In Rust, this class of leaks simply does not exist with the right API.
ObjectMethods::property_add_child() could either:
- take an Owned<T> and consume it, thus always giving a ref count of 1
on exit. If you want to keep the object you would have to clone it.
- take "&'owner self, &'child T where 'owner: 'child", then you can
pass an embedded object like object_initialize_child().
In the latter case however you *still* need to keep the reference
count elevated until Drop runs. That is, unlike C, Rust code will
always have a ref count of 2 for children. For this reason, instead of
having a "T" in the struct you would have another wrapper---something
like Child<'owner, T>. This wrapper cannot be cloned but it does an
unref when dropped.
My expectation is that property_add_child() will be used exclusivel
for the first case, i.e. it will take an Owned<T>. If you want to
create a child property from an embedded object, something like
object_initialize_child() can be used once pinned-init is used to
rewrite how instance_init is used. It will look something like
pin_init! {
&this in MyClass {
...,
iomem <- MemoryRegion::init_io(
this,
&MY_MR_OPS,
"memory-region-name",
0x1000,
),
child_obj <- ChildClass::init().to_child(this, "prop-name")
}
}
where to_child() wraps an "impl PinInit<T>" and turns it into an "impl
PinInit<Child<'a, T>>". Or something like that. :)
> From this description, I understand your goal is:
>
> * For embedded child object, its lifetimer is managed by its parent
> object, through Child<> for the most cases.
>
> * For non-embedded child - a pointer/reference in parent object, its
> lifetimer is managed by `Owned<>` (and with Child<>).
>
> Am I right?
Yes, you're right.
I am not sure if you meant Child<> as the QOM concept, or as a Rust
struct. If the latter, you're really really right.
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-29 10:03 ` Paolo Bonzini
@ 2025-02-05 8:28 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-02-05 8:28 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:03:31AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:03:31 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 02/10] rust: qom: add reference counting functionality
>
> On Sun, Jan 26, 2025 at 3:56 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > Hi Paolo,
> >
> > On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> > > Date: Fri, 17 Jan 2025 20:39:55 +0100
> > > From: Paolo Bonzini <pbonzini@redhat.com>
> > > Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> > > X-Mailer: git-send-email 2.47.1
> > >
> > > Add a smart pointer that allows to add and remove references from
> > > QOM objects. It's important to note that while all QOM objects have a
> > > reference count, in practice not all of them have their lifetime guarded
> > > by it.
> >
> > About the background, I have a maybe common question...why Rust needs
> > extra reference count guarding?
>
> Children properties are removed, and thus their reference is dropped,
> before instance_finalize() is called (see object_finalize() in
> qom/object.c). This is not valid in Rust, you need to keep the object
> alive until the last line of Rust code has run - which is after
> Drop::drop() has run.
I see, this is also a typical effort to eliminate unsafe crossing of FFI
boundaries.
> > Additionally, I felt that the ref count may be a bit confusing. After
> > creating Child<> property, the child object's ref count is sometimes 1,
> > and other times it's 2:
> >
> > * With object_initialize_child(), child's ref count is 1.
> >
> > * With object_property_add_child() (usually after a object_new() to
> > create child first):
> >
> > - sometimes user will call object_unref(), and then the ref count is 1.
> > E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
> >
> > - sometimes no object_unref(), then ref count is 2.
> > E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".
>
> In C, having a ref count of 2 is usually a latent memory leak (because
> most of the time there's not going to be an object_unref() in the
> instance_finalize() method). In this case the leak is latent, because
> TYPE_EXYNOS4210_SOC is not hot-unpluggable and thus will never really
> go away once realized.
Further, what about doing object_unref() in object_property_add_child()
or object_property_try_add_child()?
Then we can ensure the object will have ref count of 1 on the exit of
object_property_add_child() and people will no longer miss
object_unref().
Although, there are a few more devices involved to fix similar issues.
> In Rust, this class of leaks simply does not exist with the right API.
> ObjectMethods::property_add_child() could either:
>
> - take an Owned<T> and consume it, thus always giving a ref count of 1
> on exit. If you want to keep the object you would have to clone it.
>
> - take "&'owner self, &'child T where 'owner: 'child", then you can
> pass an embedded object like object_initialize_child().
>
> In the latter case however you *still* need to keep the reference
> count elevated until Drop runs. That is, unlike C, Rust code will
> always have a ref count of 2 for children. For this reason, instead of
> having a "T" in the struct you would have another wrapper---something
> like Child<'owner, T>. This wrapper cannot be cloned but it does an
> unref when dropped.
Thanks, the whole picture is nice.
> My expectation is that property_add_child() will be used exclusivel
> for the first case, i.e. it will take an Owned<T>. If you want to
> create a child property from an embedded object, something like
> object_initialize_child() can be used once pinned-init is used to
> rewrite how instance_init is used. It will look something like
>
> pin_init! {
> &this in MyClass {
> ...,
> iomem <- MemoryRegion::init_io(
> this,
> &MY_MR_OPS,
> "memory-region-name",
> 0x1000,
> ),
> child_obj <- ChildClass::init().to_child(this, "prop-name")
> }
> }
>
> where to_child() wraps an "impl PinInit<T>" and turns it into an "impl
> PinInit<Child<'a, T>>". Or something like that. :)
Elegant code design, looking forward to pin_init.
> > From this description, I understand your goal is:
> >
> > * For embedded child object, its lifetimer is managed by its parent
> > object, through Child<> for the most cases.
> >
> > * For non-embedded child - a pointer/reference in parent object, its
> > lifetimer is managed by `Owned<>` (and with Child<>).
> >
> > Am I right?
>
> Yes, you're right.
>
> I am not sure if you meant Child<> as the QOM concept, or as a Rust
> struct. If the latter, you're really really right.
>
Thank you :-) It seems virtio device will have an embedded case to adopt
Child<> struct (virtio_instance_init_common()).
Regards,
Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-17 19:39 ` [PATCH 02/10] rust: qom: add reference counting functionality Paolo Bonzini
2025-01-26 15:15 ` Zhao Liu
@ 2025-01-27 7:57 ` Zhao Liu
2025-01-29 10:16 ` Paolo Bonzini
2025-02-06 3:26 ` Zhao Liu
2 siblings, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 7:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> +impl<T: ObjectType> Owned<T> {
> + /// Convert a raw C pointer into an owned reference to the QOM
> + /// object it points to. The object's reference count will be
> + /// decreased when the `Owned` is dropped.
> + ///
> + /// # Panics
> + ///
> + /// Panics if `ptr` is NULL.
> + ///
> + /// # Safety
> + ///
> + /// The caller must indeed own a reference to the QOM object.
> + /// The object must not be embedded in another unless the outer
> + /// object is guaranteed to have a longer lifetime.
> + ///
> + /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
> + /// back to `from_raw()` (assuming the original `Owned` was valid!),
> + /// since the owned reference remains there between the calls to
> + /// `into_raw()` and `from_raw()`.
> + #[allow(clippy::missing_const_for_fn)]
> + pub unsafe fn from_raw(ptr: *const T) -> Self {
> + // SAFETY NOTE: while NonNull requires a mutable pointer, only
> + // Deref is implemented so the pointer passed to from_raw
> + // remains const
> + Owned(NonNull::new(ptr as *mut T).unwrap())
> + }
...
> + /// Increase the reference count of a QOM object and return
> + /// a new owned reference to it.
> + ///
> + /// # Safety
> + ///
> + /// The object must not be embedded in another, unless the outer
> + /// object is guaranteed to have a longer lifetime.
> + pub unsafe fn from(obj: &T) -> Self {
> + unsafe {
> + object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> +
> + // SAFETY NOTE: while NonNull requires a mutable pointer, only
> + // Deref is implemented so the reference passed to from_raw
> + // remains shared
> + Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
> + }
> + }
> +}
> +
About the difference between from_raw() and from(), I understand if the
C side also holds a pointer, the Rust side must increase the reference
count (using Owned::from), and If the C side does not have any other
pointers, Rust can directly use Owned::from_raw. Am I right?
* The use of from():
fn do_init_clock_in(
dev: *mut DeviceState,
name: &str,
cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)>,
events: ClockEvent,
) -> Owned<Clock> {
assert!(bql_locked());
// SAFETY: the clock is heap allocated and does not have a reference, so
// Owned::from adds one. the callback is disabled automatically
// when the clock is unparented, which happens before the device is
// finalized.
unsafe {
let cstr = CString::new(name).unwrap();
let clk = bindings::qdev_init_clock_in(
dev,
cstr.as_ptr(),
cb,
dev.cast::<c_void>(),
events.0,
);
Owned::from(&*clk)
}
}
* The use of from_raw():
fn new() -> Owned<Self> {
assert!(bql_locked());
// 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>())
}
}
Comparing with these 2 use cases, I find the difference is
qdev_init_clock_in() creates a pointer in qdev_init_clocklist().
Then the comment "the clock is heap allocated and does not have
a reference" sounds like a conflict. I'm sure I'm missing something. :-(
Thanks,
Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-27 7:57 ` Zhao Liu
@ 2025-01-29 10:16 ` Paolo Bonzini
2025-02-05 9:13 ` Zhao Liu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:16 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On Mon, Jan 27, 2025 at 8:38 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> > +impl<T: ObjectType> Owned<T> {
> > + /// Convert a raw C pointer into an owned reference to the QOM
> > + /// object it points to. The object's reference count will be
> > + /// decreased when the `Owned` is dropped.
> > + ///
> > + /// # Panics
> > + ///
> > + /// Panics if `ptr` is NULL.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must indeed own a reference to the QOM object.
> > + /// The object must not be embedded in another unless the outer
> > + /// object is guaranteed to have a longer lifetime.
> > + ///
> > + /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
> > + /// back to `from_raw()` (assuming the original `Owned` was valid!),
> > + /// since the owned reference remains there between the calls to
> > + /// `into_raw()` and `from_raw()`.
> > + #[allow(clippy::missing_const_for_fn)]
> > + pub unsafe fn from_raw(ptr: *const T) -> Self {
> > + // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > + // Deref is implemented so the pointer passed to from_raw
> > + // remains const
> > + Owned(NonNull::new(ptr as *mut T).unwrap())
> > + }
>
> ...
>
> > + /// Increase the reference count of a QOM object and return
> > + /// a new owned reference to it.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The object must not be embedded in another, unless the outer
> > + /// object is guaranteed to have a longer lifetime.
> > + pub unsafe fn from(obj: &T) -> Self {
> > + unsafe {
> > + object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> > +
> > + // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > + // Deref is implemented so the reference passed to from_raw
> > + // remains shared
> > + Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
> > + }
> > + }
> > +}
> > +
>
> About the difference between from_raw() and from(), I understand if the
> C side also holds a pointer, the Rust side must increase the reference
> count (using Owned::from), and If the C side does not have any other
> pointers, Rust can directly use Owned::from_raw. Am I right?
Pretty much - more precisely you use Object::from_raw 1) if the C side
gifts a reference 2) if you got the pointer from Owned::into_raw. The
second case is similar to Arc::from_raw, which expects that you got a
reference from Arc::into_raw. The first is the more common case.
>
> * The use of from():
>
> let clk = bindings::qdev_init_clock_in(...)
> Owned::from(&*clk)
In this case the C side wants to manage the reference that
qdev_init_clock_in() returns; it is dropped in
qdev_finalize_clocklist(). So Rust code needs to increase the
refcount.
> * The use of from_raw():
>
> fn new() -> Owned<Self> {
> assert!(bql_locked());
> // 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>())
> }
> }
In this case the C side lets the caller manage the (only) reference
when object_new returns, so you must not increase the refcount.
Owned::from() is slightly less efficient, though that almost never
matters. If it does you can use ManuallyDrop::new(Owned::from_raw(p)).
> Comparing with these 2 use cases, I find the difference is
> qdev_init_clock_in() creates a pointer in qdev_init_clocklist().
That is related, but more precisely the difference is that
qdev_init_clock_in() wants to unref that pointer later.
> Then the comment "the clock is heap allocated and does not have
> a reference" sounds like a conflict. I'm sure I'm missing something. :-(
Changed:
// SAFETY: the clock is heap allocated, but qdev_init_clock_in()
// does not gift the reference to its caller; so use Owned::from to
// add one. the callback is disabled automatically when the clock
// is unparented, which happens before the device is finalized.
Thanks for the review!
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-29 10:16 ` Paolo Bonzini
@ 2025-02-05 9:13 ` Zhao Liu
2025-02-05 9:10 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-02-05 9:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> > * The use of from():
> >
> > let clk = bindings::qdev_init_clock_in(...)
> > Owned::from(&*clk)
>
> In this case the C side wants to manage the reference that
> qdev_init_clock_in() returns; it is dropped in
> qdev_finalize_clocklist(). So Rust code needs to increase the
> refcount.
Pls forgive me for one more question about qdev_init_clock_in() on the C
side. :-)
qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
so it is intentional, to make the ref count of `clk` be 2:
* 1 count is held by clocklist until qdev_finalize_clocklist().
* another 1 is held by its parent via QOM Child<>.
Am I understanding it correctly?
> > Then the comment "the clock is heap allocated and does not have
> > a reference" sounds like a conflict. I'm sure I'm missing something. :-(
>
> Changed:
>
> // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
> // does not gift the reference to its caller; so use Owned::from to
> // add one. the callback is disabled automatically when the clock
> // is unparented, which happens before the device is finalized.
LGTM.
Thank you very much for your patience. I think I understand ref count
now.
Regards,
Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-02-05 9:13 ` Zhao Liu
@ 2025-02-05 9:10 ` Paolo Bonzini
2025-02-05 9:40 ` Zhao Liu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-05 9:10 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 2/5/25 10:13, Zhao Liu wrote:
>>> * The use of from():
>>>
>>> let clk = bindings::qdev_init_clock_in(...)
>>> Owned::from(&*clk)
>>
>> In this case the C side wants to manage the reference that
>> qdev_init_clock_in() returns; it is dropped in
>> qdev_finalize_clocklist(). So Rust code needs to increase the
>> refcount.
>
> Pls forgive me for one more question about qdev_init_clock_in() on the C
> side. :-)
>
> qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
> so it is intentional, to make the ref count of `clk` be 2:
> * 1 count is held by clocklist until qdev_finalize_clocklist().
> * another 1 is held by its parent via QOM Child<>.
>
> Am I understanding it correctly?
Yes, that's more precise. In Rust it will be 3, the two above plus the
Owned<Clock>.
Ah wait... qdev_finalize_clocklist() is only called _after_ the Rust
struct is Drop::drop-ped, because device_finalize() is called after the
subclass's instance_finalize.
So the result of qdev_init_clock_in(), strictly speaking, does not have
to be an Owned<Clock>. It can also be a &'device Clock; either is
possible. Would you prefer that, or do you think it's enough to add a
comment?
Paolo
>>> Then the comment "the clock is heap allocated and does not have
>>> a reference" sounds like a conflict. I'm sure I'm missing something. :-(
>>
>> Changed:
>>
>> // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
>> // does not gift the reference to its caller; so use Owned::from to
>> // add one. the callback is disabled automatically when the clock
>> // is unparented, which happens before the device is finalized.
>
> LGTM.
>
> Thank you very much for your patience. I think I understand ref count
> now.
>
> Regards,
> Zhao
>
>
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-02-05 9:10 ` Paolo Bonzini
@ 2025-02-05 9:40 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-02-05 9:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Wed, Feb 05, 2025 at 10:10:01AM +0100, Paolo Bonzini wrote:
> Date: Wed, 5 Feb 2025 10:10:01 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 02/10] rust: qom: add reference counting functionality
>
> On 2/5/25 10:13, Zhao Liu wrote:
> > > > * The use of from():
> > > >
> > > > let clk = bindings::qdev_init_clock_in(...)
> > > > Owned::from(&*clk)
> > >
> > > In this case the C side wants to manage the reference that
> > > qdev_init_clock_in() returns; it is dropped in
> > > qdev_finalize_clocklist(). So Rust code needs to increase the
> > > refcount.
> >
> > Pls forgive me for one more question about qdev_init_clock_in() on the C
> > side. :-)
> >
> > qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
> > so it is intentional, to make the ref count of `clk` be 2:
> > * 1 count is held by clocklist until qdev_finalize_clocklist().
> > * another 1 is held by its parent via QOM Child<>.
> >
> > Am I understanding it correctly?
>
> Yes, that's more precise. In Rust it will be 3, the two above plus the
> Owned<Clock>.
Thanks!
> Ah wait... qdev_finalize_clocklist() is only called _after_ the Rust struct
> is Drop::drop-ped, because device_finalize() is called after the subclass's
> instance_finalize.
>
> So the result of qdev_init_clock_in(), strictly speaking, does not have to
> be an Owned<Clock>. It can also be a &'device Clock; either is possible.
> Would you prefer that, or do you think it's enough to add a comment?
>
I prefer the latter, i.e., keep current Owned<Clock> and add a comment
to mention something like "Although strictly speaking, it does not have
to be an Owned<Clock>. Owned<> is still worth favor to use to protect
Rust code from FFI. When unsure whether to use Owned<>, then use Owned<>."
-Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 02/10] rust: qom: add reference counting functionality
2025-01-17 19:39 ` [PATCH 02/10] rust: qom: add reference counting functionality Paolo Bonzini
2025-01-26 15:15 ` Zhao Liu
2025-01-27 7:57 ` Zhao Liu
@ 2025-02-06 3:26 ` Zhao Liu
2 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-02-06 3:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> X-Mailer: git-send-email 2.47.1
>
> Add a smart pointer that allows to add and remove references from
> QOM objects. It's important to note that while all QOM objects have a
> reference count, in practice not all of them have their lifetime guarded
> by it. Embedded objects, specifically, are confined to the lifetime of
> the owner.
>
> When writing Rust bindings this is important, because embedded objects are
> *never* used through the "Owned<>" smart pointer that is introduced here.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/qom.rs | 121 ++++++++++++++++++++++++++++++++++-
> rust/qemu-api/src/vmstate.rs | 6 +-
> 2 files changed, 125 insertions(+), 2 deletions(-)
>
Revisit this patch (with Paolo's explaination), LGTM,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 03/10] rust: qom: add object creation functionality
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
2025-01-17 19:39 ` [PATCH 01/10] rust: qemu-api: add sub-subclass to the integration tests Paolo Bonzini
2025-01-17 19:39 ` [PATCH 02/10] rust: qom: add reference counting functionality Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-02-06 7:49 ` Zhao Liu
2025-01-17 19:39 ` [PATCH 04/10] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
` (7 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
The basic object lifecycle test can now be implemented using safe code!
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 13 ++++++++-----
rust/qemu-api/src/prelude.rs | 1 +
rust/qemu-api/src/qom.rs | 23 +++++++++++++++++++++--
rust/qemu-api/tests/tests.rs | 30 +++++++++++-------------------
4 files changed, 41 insertions(+), 26 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 27563700665..d8409f3d310 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -690,15 +690,18 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
irq: qemu_irq,
chr: *mut Chardev,
) -> *mut DeviceState {
+ let pl011 = PL011State::new();
unsafe {
- let dev: *mut DeviceState = qdev_new(PL011State::TYPE_NAME.as_ptr());
- let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
-
+ let dev = pl011.as_mut_ptr::<DeviceState>();
qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr);
- sysbus_realize_and_unref(sysbus, addr_of_mut!(error_fatal));
+
+ let sysbus = pl011.as_mut_ptr::<SysBusDevice>();
+ sysbus_realize(sysbus, addr_of_mut!(error_fatal));
sysbus_mmio_map(sysbus, 0, addr);
sysbus_connect_irq(sysbus, 0, irq);
- dev
+
+ // return the pointer, which is kept alive by the QOM tree; drop owned ref
+ pl011.as_mut_ptr()
}
}
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 2dc86e19b29..3df6a5c21ec 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -12,6 +12,7 @@
pub use crate::qom::ObjectCast;
pub use crate::qom::ObjectCastMut;
pub use crate::qom::ObjectDeref;
+pub use crate::qom::ObjectClassMethods;
pub use crate::qom::ObjectMethods;
pub use crate::qom::ObjectType;
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index c404f8a1aa7..6f7db3eabcb 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -66,8 +66,8 @@
use crate::{
bindings::{
- self, object_dynamic_cast, object_get_class, object_get_typename, object_ref, object_unref,
- TypeInfo,
+ self, object_dynamic_cast, object_get_class, object_get_typename, object_new, object_ref,
+ object_unref, TypeInfo,
},
cell::bql_locked,
};
@@ -717,6 +717,24 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
}
}
+/// Trait for class methods exposed by the Object class. The methods can be
+/// called on all objects that have the trait `IsA<Object>`.
+///
+/// The trait should only be used through the blanket implementation,
+/// which guarantees safety via `IsA`
+pub trait ObjectClassMethods: IsA<Object> {
+ /// Return a new reference counted instance of this class
+ fn new() -> Owned<Self> {
+ assert!(bql_locked());
+ // 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>())
+ }
+ }
+}
+
/// Trait for methods exposed by the Object class. The methods can be
/// called on all objects that have the trait `IsA<Object>`.
///
@@ -757,4 +775,5 @@ fn debug_fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
}
}
+impl<T> ObjectClassMethods for T where T: IsA<Object> {}
impl<R: ObjectDeref> ObjectMethods for R where R::Target: IsA<Object> {}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 5c3e75ed3d5..1944e65c8f3 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -3,8 +3,7 @@
// SPDX-License-Identifier: GPL-2.0-or-later
use std::{
- ffi::CStr,
- os::raw::c_void,
+ ffi::{c_void, CStr},
ptr::{addr_of, addr_of_mut},
};
@@ -132,22 +131,16 @@ fn init_qom() {
/// Create and immediately drop an instance.
fn test_object_new() {
init_qom();
- unsafe {
- object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast());
- object_unref(object_new(DummyChildState::TYPE_NAME.as_ptr()).cast());
- }
+ drop(DummyState::new());
+ drop(DummyChildState::new());
}
#[test]
/// Try invoking a method on an object.
fn test_typename() {
init_qom();
- let p: *mut DummyState = unsafe { object_new(DummyState::TYPE_NAME.as_ptr()).cast() };
- let p_ref: &DummyState = unsafe { &*p };
- assert_eq!(p_ref.typename(), "dummy");
- unsafe {
- object_unref(p_ref.as_object_mut_ptr().cast::<c_void>());
- }
+ let p = DummyState::new();
+ assert_eq!(p.typename(), "dummy");
}
// a note on all "cast" tests: usually, especially for downcasts the desired
@@ -162,24 +155,23 @@ fn test_typename() {
/// Test casts on shared references.
fn test_cast() {
init_qom();
- let p: *mut DummyState = unsafe { object_new(DummyState::TYPE_NAME.as_ptr()).cast() };
+ let p = DummyState::new();
+ let p_ptr: *mut DummyState = unsafe { p.as_mut_ptr() };
+ let p_ref: &mut DummyState = unsafe { &mut *p_ptr };
- let p_ref: &DummyState = unsafe { &*p };
let obj_ref: &Object = p_ref.upcast();
- assert_eq!(addr_of!(*obj_ref), p.cast());
+ assert_eq!(addr_of!(*obj_ref), p_ptr.cast());
let sbd_ref: Option<&SysBusDevice> = obj_ref.dynamic_cast();
assert!(sbd_ref.is_none());
let dev_ref: Option<&DeviceState> = obj_ref.downcast();
- assert_eq!(addr_of!(*dev_ref.unwrap()), p.cast());
+ assert_eq!(addr_of!(*dev_ref.unwrap()), p_ptr.cast());
// SAFETY: the cast is wrong, but the value is only used for comparison
unsafe {
let sbd_ref: &SysBusDevice = obj_ref.unsafe_cast();
- assert_eq!(addr_of!(*sbd_ref), p.cast());
-
- object_unref(p_ref.as_object_mut_ptr().cast::<c_void>());
+ assert_eq!(addr_of!(*sbd_ref), p_ptr.cast());
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 03/10] rust: qom: add object creation functionality
2025-01-17 19:39 ` [PATCH 03/10] rust: qom: add object creation functionality Paolo Bonzini
@ 2025-02-06 7:49 ` Zhao Liu
2025-02-06 7:39 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-02-06 7:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:39:56PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:56 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/10] rust: qom: add object creation functionality
> X-Mailer: git-send-email 2.47.1
>
> The basic object lifecycle test can now be implemented using safe code!
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 13 ++++++++-----
> rust/qemu-api/src/prelude.rs | 1 +
> rust/qemu-api/src/qom.rs | 23 +++++++++++++++++++++--
> rust/qemu-api/tests/tests.rs | 30 +++++++++++-------------------
> 4 files changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 27563700665..d8409f3d310 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -690,15 +690,18 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
> irq: qemu_irq,
> chr: *mut Chardev,
> ) -> *mut DeviceState {
> + let pl011 = PL011State::new();
> unsafe {
> - let dev: *mut DeviceState = qdev_new(PL011State::TYPE_NAME.as_ptr());
> - let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
> -
> + let dev = pl011.as_mut_ptr::<DeviceState>();
> qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr);
> - sysbus_realize_and_unref(sysbus, addr_of_mut!(error_fatal));
> +
> + let sysbus = pl011.as_mut_ptr::<SysBusDevice>();
> + sysbus_realize(sysbus, addr_of_mut!(error_fatal));
> sysbus_mmio_map(sysbus, 0, addr);
> sysbus_connect_irq(sysbus, 0, irq);
> - dev
> +
> + // return the pointer, which is kept alive by the QOM tree; drop owned ref
> + pl011.as_mut_ptr()
The ref count of Owned<> is decreased on exit, so we need to use
sysbus_realize() instead of sysbus_realize_and_unref() to ensure ref
count is correct at C side.
Initially, I hesitated here for an entire morning because this didn't
seem to conform to the usual usage of sysbus_realize_and_unref() (or,
further, qdev_realize_and_unref()).
But now I realize that qdev_realize() is used for embedded objects,
while qdev_realize_and_unref() is used for non-embedded cases. Therefore,
moving forward, perhaps qdev_realize_and_unref() should not exist on the
Rust side.
Owned<> will automatically drop and thus automatically unref, while
Child<> will not unref. Based on this consideration, I am now convincing
myself that this change (using sysbus_realize()) is reasonable. :-)
In the future, maybe we need some wrappers on qdev_realize()/sysbus_realize().
> }
> }
Overall, still fine for me,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 03/10] rust: qom: add object creation functionality
2025-02-06 7:49 ` Zhao Liu
@ 2025-02-06 7:39 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-06 7:39 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On Thu, Feb 6, 2025 at 8:29 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> The ref count of Owned<> is decreased on exit, so we need to use
> sysbus_realize() instead of sysbus_realize_and_unref() to ensure ref
> count is correct at C side.
>
> Initially, I hesitated here for an entire morning because this didn't
> seem to conform to the usual usage of sysbus_realize_and_unref() (or,
> further, qdev_realize_and_unref()).
>
> But now I realize that qdev_realize() is used for embedded objects,
> while qdev_realize_and_unref() is used for non-embedded cases. Therefore,
> moving forward, perhaps qdev_realize_and_unref() should not exist on the
> Rust side.
Correct, all unref() operations are handled implicitly by the Drop
implementation of Owned<>
> Owned<> will automatically drop and thus automatically unref, while
> Child<> will not unref.
No, Child<> will also unref. The point of Child<> is to prolong the
life of the child object, so that it dies after instance_finalize
instead() of dying during object_property_del_all(). In C that has to
be done manually (as is the case in the clock creation functions), in
Rust it will be enforced by the type system.
> In the future, maybe we need some wrappers on qdev_realize()/sysbus_realize().
Yep, I'll add a conversion of pl011_create to safe Rust to clarify the
rules around usage of Owned<>.
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 04/10] rust: callbacks: allow passing optional callbacks as ()
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (2 preceding siblings ...)
2025-01-17 19:39 ` [PATCH 03/10] rust: qom: add object creation functionality Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-01-27 8:41 ` Zhao Liu
2025-01-17 19:39 ` [PATCH 05/10] rust: qdev: add clock creation Paolo Bonzini
` (6 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
In some cases, callbacks are optional. Using "Some(function)" and "None"
does not work well, because when someone writes "None" the compiler does
not know what to use for "F" in "Option<F>".
Therefore, adopt () to mean a "null" callback. It is possible to enforce
that a callback is valid by adding a "let _: () = F::ASSERT_IS_SOME" before
the invocation of F::call.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/callbacks.rs | 97 ++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/rust/qemu-api/src/callbacks.rs b/rust/qemu-api/src/callbacks.rs
index 314f9dce962..a59878bfb74 100644
--- a/rust/qemu-api/src/callbacks.rs
+++ b/rust/qemu-api/src/callbacks.rs
@@ -79,6 +79,31 @@
/// call_it(&move |_| String::from(x), "hello workd");
/// ```
///
+/// `()` can be used to indicate "no function":
+///
+/// ```
+/// # use qemu_api::callbacks::FnCall;
+/// fn optional<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> Option<String> {
+/// if F::IS_SOME {
+/// Some(F::call((s,)))
+/// } else {
+/// None
+/// }
+/// }
+///
+/// assert!(optional(&(), "hello world").is_none());
+/// ```
+///
+/// Invoking `F::call` will then be a run-time error.
+///
+/// ```should_panic
+/// # use qemu_api::callbacks::FnCall;
+/// # fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String {
+/// # F::call((s,))
+/// # }
+/// let s: String = call_it(&(), "hello world"); // panics
+/// ```
+///
/// # Safety
///
/// Because `Self` is a zero-sized type, all instances of the type are
@@ -93,10 +118,70 @@ pub unsafe trait FnCall<Args, R = ()>: 'static + Sync + Sized {
/// Rust 1.79.0+.
const ASSERT_ZERO_SIZED: () = { assert!(mem::size_of::<Self>() == 0) };
+ /// Referring to this constant asserts that the `Self` type is an actual
+ /// function type, which can be used to catch incorrect use of `()`
+ /// at compile time.
+ ///
+ /// # Examples
+ ///
+ /// ```compile_fail
+ /// # use qemu_api::callbacks::FnCall;
+ /// fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String {
+ /// let _: () = F::ASSERT_IS_SOME;
+ /// F::call((s,))
+ /// }
+ ///
+ /// let s: String = call_it((), "hello world"); // does not compile
+ /// ```
+ ///
+ /// Note that this use more simply `const { assert!(F::IS_SOME) }` in
+ /// Rust 1.79.0 or newer.
+ const ASSERT_IS_SOME: () = { assert!(Self::IS_SOME) };
+
+ /// `true` if `Self` is an actual function type and not `()`.
+ ///
+ /// # Examples
+ ///
+ /// You can use `IS_SOME` to catch this at compile time:
+ ///
+ /// ```compile_fail
+ /// # use qemu_api::callbacks::FnCall;
+ /// fn call_it<F: for<'a> FnCall<(&'a str,), String>>(_f: &F, s: &str) -> String {
+ /// const { assert!(F::IS_SOME) }
+ /// F::call((s,))
+ /// }
+ ///
+ /// let s: String = call_it((), "hello world"); // does not compile
+ /// ```
+ const IS_SOME: bool;
+
+ /// `false` if `Self` is an actual function type, `true` if it is `()`.
+ fn is_none() -> bool {
+ !Self::IS_SOME
+ }
+
+ /// `true` if `Self` is an actual function type, `false` if it is `()`.
+ fn is_some() -> bool {
+ Self::IS_SOME
+ }
+
/// Call the function with the arguments in args.
fn call(a: Args) -> R;
}
+/// `()` acts as a "null" callback. Using `()` and `function` is nicer
+/// than `None` and `Some(function)`, because the compiler is unable to
+/// infer the type of just `None`. Therefore, the trait itself acts as the
+/// option type, with functions [`FnCall::is_some`] and [`FnCall::is_none`].
+unsafe impl<Args, R> FnCall<Args, R> for () {
+ const IS_SOME: bool = false;
+
+ /// Call the function with the arguments in args.
+ fn call(_a: Args) -> R {
+ panic!("callback not specified")
+ }
+}
+
macro_rules! impl_call {
($($args:ident,)* ) => (
// SAFETY: because each function is treated as a separate type,
@@ -106,6 +191,8 @@ unsafe impl<F, $($args,)* R> FnCall<($($args,)*), R> for F
where
F: 'static + Sync + Sized + Fn($($args, )*) -> R,
{
+ const IS_SOME: bool = true;
+
#[inline(always)]
fn call(a: ($($args,)*)) -> R {
let _: () = Self::ASSERT_ZERO_SIZED;
@@ -141,4 +228,14 @@ fn do_test_call<'a, F: FnCall<(&'a str,), String>>(_f: &F) -> String {
fn test_call() {
assert_eq!(do_test_call(&str::to_owned), "hello world")
}
+
+ // The `_f` parameter is unused but it helps the compiler infer `F`.
+ fn do_test_is_some<'a, F: FnCall<(&'a str,), String>>(_f: &F) {
+ assert!(F::is_some());
+ }
+
+ #[test]
+ fn test_is_some() {
+ do_test_is_some(&str::to_owned);
+ }
}
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 04/10] rust: callbacks: allow passing optional callbacks as ()
2025-01-17 19:39 ` [PATCH 04/10] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
@ 2025-01-27 8:41 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 8:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> Date: Fri, 17 Jan 2025 20:39:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/10] rust: callbacks: allow passing optional callbacks as
> ()
> X-Mailer: git-send-email 2.47.1
>
> In some cases, callbacks are optional. Using "Some(function)" and "None"
> does not work well, because when someone writes "None" the compiler does
> not know what to use for "F" in "Option<F>".
>
> Therefore, adopt () to mean a "null" callback. It is possible to enforce
> that a callback is valid by adding a "let _: () = F::ASSERT_IS_SOME" before
> the invocation of F::call.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/callbacks.rs | 97 ++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
Awesome trick.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> + /// Referring to this constant asserts that the `Self` type is an actual
> + /// function type, which can be used to catch incorrect use of `()`
> + /// at compile time.
An extra whitespace.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 05/10] rust: qdev: add clock creation
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (3 preceding siblings ...)
2025-01-17 19:39 ` [PATCH 04/10] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-02-06 8:15 ` Zhao Liu
2025-01-17 19:39 ` [PATCH 06/10] rust: qom: allow initializing interface vtables Paolo Bonzini
` (5 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Add a Rust version of qdev_init_clock_in, which can be used in
instance_init. There are a couple differences with the C
version:
- in Rust the object keeps its own reference to the clock (in addition to
the one embedded in the NamedClockList), and the reference is dropped
automatically by instance_finalize(); this is encoded in the signature
of DeviceClassMethods::init_clock_in, which makes the lifetime of the
clock independent of that of the object it holds. This goes unnoticed
in the C version and is due to the existence of aliases.
- also, anything that happens during instance_init uses the pinned_init
framework to operate on a partially initialized object, and is done
through class methods (i.e. through DeviceClassMethods rather than
DeviceMethods) because the device does not exist yet. Therefore, Rust
code *must* create clocks from instance_init, which is stricter than C.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 28 +++++-------
rust/qemu-api/src/prelude.rs | 2 +
rust/qemu-api/src/qdev.rs | 76 ++++++++++++++++++++++++++++++--
rust/qemu-api/src/vmstate.rs | 4 +-
4 files changed, 87 insertions(+), 23 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index d8409f3d310..dfe199ad0ed 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -13,8 +13,8 @@
c_str, impl_vmstate_forward,
irq::InterruptSource,
prelude::*,
- qdev::DeviceImpl,
- qom::{ClassInitImpl, ObjectImpl, ParentField},
+ qdev::{Clock, ClockEvent, DeviceImpl},
+ qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
};
use crate::{
@@ -123,7 +123,7 @@ pub struct PL011State {
#[doc(alias = "irq")]
pub interrupts: [InterruptSource; IRQMASK.len()],
#[doc(alias = "clk")]
- pub clock: NonNull<Clock>,
+ pub clock: Owned<Clock>,
#[doc(alias = "migrate_clk")]
pub migrate_clock: bool,
}
@@ -487,8 +487,6 @@ impl PL011State {
/// location/instance. All its fields are expected to hold unitialized
/// values with the sole exception of `parent_obj`.
unsafe fn init(&mut self) {
- const CLK_NAME: &CStr = c_str!("clk");
-
// SAFETY:
//
// self and self.iomem are guaranteed to be valid at this point since callers
@@ -508,22 +506,16 @@ unsafe fn init(&mut self) {
// SAFETY:
//
- // self.clock is not initialized at this point; but since `NonNull<_>` is Copy,
- // we can overwrite the undefined value without side effects. This is
+ // self.clock is not initialized at this point; but since `NonNull<_>` is
+ // not Drop, we can overwrite the undefined value without side effects. This is
// safe since all PL011State instances are created by QOM code which
// calls this function to initialize the fields; therefore no code is
// able to access an invalid self.clock value.
- unsafe {
- let dev: &mut DeviceState = self.upcast_mut();
- self.clock = NonNull::new(qdev_init_clock_in(
- dev,
- CLK_NAME.as_ptr(),
- None, /* pl011_clock_update */
- addr_of_mut!(*self).cast::<c_void>(),
- ClockEvent::ClockUpdate.0,
- ))
- .unwrap();
- }
+ self.clock = self.init_clock_in("clk", &Self::clock_update, ClockEvent::ClockUpdate);
+ }
+
+ const fn clock_update(&self, _event: ClockEvent) {
+ /* pl011_trace_baudrate_change(s); */
}
fn post_init(&self) {
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 3df6a5c21ec..87e3ce90f26 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -7,6 +7,8 @@
pub use crate::cell::BqlCell;
pub use crate::cell::BqlRefCell;
+pub use crate::qdev::DeviceMethods;
+
pub use crate::qom::IsA;
pub use crate::qom::Object;
pub use crate::qom::ObjectCast;
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 4658409bebb..f573712550e 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -4,14 +4,19 @@
//! Bindings to create devices and access device functionality from Rust.
-use std::ffi::CStr;
+use std::{
+ ffi::{CStr, CString},
+ os::raw::c_void,
+};
-pub use bindings::{DeviceClass, DeviceState, Property};
+pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property};
use crate::{
bindings::{self, Error},
+ callbacks::FnCall,
+ cell::bql_locked,
prelude::*,
- qom::{ClassInitImpl, ObjectClass},
+ qom::{ClassInitImpl, ObjectClass, Owned},
vmstate::VMStateDescription,
};
@@ -145,3 +150,68 @@ unsafe impl ObjectType for DeviceState {
unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
}
qom_isa!(DeviceState: Object);
+
+pub trait DeviceMethods: ObjectDeref
+where
+ Self::Target: IsA<DeviceState>,
+{
+ #[inline]
+ fn init_clock_in<F: for<'a> FnCall<(&'a Self::Target, ClockEvent)>>(
+ &self,
+ name: &str,
+ _cb: &F,
+ events: ClockEvent,
+ ) -> Owned<Clock> {
+ fn do_init_clock_in(
+ dev: *mut DeviceState,
+ name: &str,
+ cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)>,
+ events: ClockEvent,
+ ) -> Owned<Clock> {
+ assert!(bql_locked());
+
+ // SAFETY: the clock is heap allocated and does not have a reference, so
+ // Owned::from adds one. the callback is disabled automatically
+ // when the clock is unparented, which happens before the device is
+ // finalized.
+ unsafe {
+ let cstr = CString::new(name).unwrap();
+ let clk = bindings::qdev_init_clock_in(
+ dev,
+ cstr.as_ptr(),
+ cb,
+ dev.cast::<c_void>(),
+ events.0,
+ );
+
+ Owned::from(&*clk)
+ }
+ }
+
+ let cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)> = if F::is_some() {
+ unsafe extern "C" fn rust_clock_cb<T, F: for<'a> FnCall<(&'a T, ClockEvent)>>(
+ opaque: *mut c_void,
+ event: ClockEvent,
+ ) {
+ // SAFETY: the opaque is "this", which is indeed a pointer to T
+ F::call((unsafe { &*(opaque.cast::<T>()) }, event))
+ }
+ Some(rust_clock_cb::<Self::Target, F>)
+ } else {
+ None
+ };
+
+ // SAFETY: self can be cast to DeviceState because its type has an
+ // IsA<DeviceState> bound.
+ do_init_clock_in(unsafe { self.as_mut_ptr() }, name, cb, events)
+ }
+}
+
+impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
+
+unsafe impl ObjectType for Clock {
+ type Class = ObjectClass;
+ const TYPE_NAME: &'static CStr =
+ unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_CLOCK) };
+}
+qom_isa!(Clock: Object);
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 11d21b8791c..164effc6553 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -470,11 +470,11 @@ macro_rules! vmstate_clock {
$crate::assert_field_type!(
$struct_name,
$field_name,
- core::ptr::NonNull<$crate::bindings::Clock>
+ $crate::qom::Owned<$crate::bindings::Clock>
);
$crate::offset_of!($struct_name, $field_name)
},
- size: ::core::mem::size_of::<*const $crate::bindings::Clock>(),
+ size: ::core::mem::size_of::<*const $crate::qdev::Clock>(),
flags: VMStateFlags(VMStateFlags::VMS_STRUCT.0 | VMStateFlags::VMS_POINTER.0),
vmsd: unsafe { ::core::ptr::addr_of!($crate::bindings::vmstate_clock) },
..$crate::zeroable::Zeroable::ZERO
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 05/10] rust: qdev: add clock creation
2025-01-17 19:39 ` [PATCH 05/10] rust: qdev: add clock creation Paolo Bonzini
@ 2025-02-06 8:15 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-02-06 8:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> // SAFETY:
> //
> - // self.clock is not initialized at this point; but since `NonNull<_>` is Copy,
> - // we can overwrite the undefined value without side effects. This is
> + // self.clock is not initialized at this point; but since `NonNull<_>` is
s/NonNull<_>/Owned<_>/
> + // not Drop, we can overwrite the undefined value without side effects. This is
> // safe since all PL011State instances are created by QOM code which
> // calls this function to initialize the fields; therefore no code is
> // able to access an invalid self.clock value.
> - unsafe {
> - let dev: &mut DeviceState = self.upcast_mut();
> - self.clock = NonNull::new(qdev_init_clock_in(
> - dev,
> - CLK_NAME.as_ptr(),
> - None, /* pl011_clock_update */
> - addr_of_mut!(*self).cast::<c_void>(),
> - ClockEvent::ClockUpdate.0,
> - ))
> - .unwrap();
> - }
> + self.clock = self.init_clock_in("clk", &Self::clock_update, ClockEvent::ClockUpdate);
> + }
> +
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 06/10] rust: qom: allow initializing interface vtables
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (4 preceding siblings ...)
2025-01-17 19:39 ` [PATCH 05/10] rust: qdev: add clock creation Paolo Bonzini
@ 2025-01-17 19:39 ` Paolo Bonzini
2025-01-27 10:33 ` Zhao Liu
2025-01-17 19:40 ` [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
` (4 subsequent siblings)
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Unlike regular classes, interface vtables can only be obtained via
object_class_dynamic_cast. Provide a wrapper that allows accessing
the vtable and pass it to a ClassInitImpl implementation, for example
ClassInitImpl<ResettableClass>.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/prelude.rs | 1 +
rust/qemu-api/src/qom.rs | 45 ++++++++++++++++++++++++++++++++++--
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 87e3ce90f26..254edb476dd 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -9,6 +9,7 @@
pub use crate::qdev::DeviceMethods;
+pub use crate::qom::InterfaceType;
pub use crate::qom::IsA;
pub use crate::qom::Object;
pub use crate::qom::ObjectCast;
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 6f7db3eabcb..8b2dbb3919c 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -66,8 +66,8 @@
use crate::{
bindings::{
- self, object_dynamic_cast, object_get_class, object_get_typename, object_new, object_ref,
- object_unref, TypeInfo,
+ self, object_class_dynamic_cast, object_dynamic_cast, object_get_class,
+ object_get_typename, object_new, object_ref, object_unref, TypeInfo,
},
cell::bql_locked,
};
@@ -256,6 +256,47 @@ unsafe fn as_object_mut_ptr(&self) -> *mut Object {
}
}
+/// Trait exposed by all structs corresponding to QOM interfaces.
+/// Unlike `ObjectType`, it is implemented on the class type (which provides
+/// the vtable for the interfaces).
+///
+/// # Safety
+///
+/// `TYPE` must match the contents of the `TypeInfo` as found in the C code;
+/// right now, interfaces can only be declared in C.
+pub unsafe trait InterfaceType: Sized {
+ /// The name of the type, which can be passed to
+ /// `object_class_dynamic_cast()` to obtain the pointer to the vtable
+ /// 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
+ /// lists the interface in its `TypeInfo`.
+ ///
+ /// # 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,
+ ) {
+ unsafe {
+ // SAFETY: upcasting to ObjectClass is always valid, and the
+ // return type is either NULL or the argument itself
+ let result: *mut Self = object_class_dynamic_cast(
+ (klass as *mut U::Class).cast(),
+ Self::TYPE_NAME.as_ptr(),
+ )
+ .cast();
+
+ <T as ClassInitImpl<Self>>::class_init(result.as_mut().unwrap())
+ }
+ }
+}
+
/// This trait provides safe casting operations for QOM objects to raw pointers,
/// to be used for example for FFI. The trait can be applied to any kind of
/// reference or smart pointers, and enforces correctness through the [`IsA`]
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 06/10] rust: qom: allow initializing interface vtables
2025-01-17 19:39 ` [PATCH 06/10] rust: qom: allow initializing interface vtables Paolo Bonzini
@ 2025-01-27 10:33 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 10:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:39:59PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:59 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 06/10] rust: qom: allow initializing interface vtables
> X-Mailer: git-send-email 2.47.1
>
> Unlike regular classes, interface vtables can only be obtained via
> object_class_dynamic_cast. Provide a wrapper that allows accessing
> the vtable and pass it to a ClassInitImpl implementation, for example
> ClassInitImpl<ResettableClass>.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/prelude.rs | 1 +
> rust/qemu-api/src/qom.rs | 45 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 44 insertions(+), 2 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (5 preceding siblings ...)
2025-01-17 19:39 ` [PATCH 06/10] rust: qom: allow initializing interface vtables Paolo Bonzini
@ 2025-01-17 19:40 ` Paolo Bonzini
2025-01-27 9:10 ` Zhao Liu
2025-02-06 8:37 ` Philippe Mathieu-Daudé
2025-01-17 19:40 ` [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
` (3 subsequent siblings)
10 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:40 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
In practice it has to be implemented always in order to access an
implementation of ClassInitImpl<ObjectClass>. Make the relationship
explicit in the code.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/qdev.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index f573712550e..ab883e2faef 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -21,7 +21,7 @@
};
/// Trait providing the contents of [`DeviceClass`].
-pub trait DeviceImpl {
+pub trait DeviceImpl: ObjectImpl {
/// _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).
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl
2025-01-17 19:40 ` [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
@ 2025-01-27 9:10 ` Zhao Liu
2025-02-06 8:37 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 9:10 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:40:00PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:40:00 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of
> DeviceImpl
> X-Mailer: git-send-email 2.47.1
>
> In practice it has to be implemented always in order to access an
> implementation of ClassInitImpl<ObjectClass>. Make the relationship
> explicit in the code.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/qdev.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl
2025-01-17 19:40 ` [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
2025-01-27 9:10 ` Zhao Liu
@ 2025-02-06 8:37 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-06 8:37 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-rust
On 17/1/25 20:40, Paolo Bonzini wrote:
> In practice it has to be implemented always in order to access an
> implementation of ClassInitImpl<ObjectClass>. Make the relationship
> explicit in the code.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/qdev.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (6 preceding siblings ...)
2025-01-17 19:40 ` [PATCH 07/10] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
@ 2025-01-17 19:40 ` Paolo Bonzini
2025-01-27 10:31 ` Zhao Liu
2025-02-06 8:31 ` Zhao Liu
2025-01-17 19:40 ` [PATCH 09/10] rust: bindings: add Sync markers to types referred to by MemoryRegionOps Paolo Bonzini
` (2 subsequent siblings)
10 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:40 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
meson.build | 1 +
rust/hw/char/pl011/src/device.rs | 10 ++-
rust/qemu-api/src/qdev.rs | 116 ++++++++++++++++++++++++-------
rust/qemu-api/tests/tests.rs | 5 +-
4 files changed, 102 insertions(+), 30 deletions(-)
diff --git a/meson.build b/meson.build
index d06f59095c6..2d9fa87af83 100644
--- a/meson.build
+++ b/meson.build
@@ -4065,6 +4065,7 @@ if have_rust
'MigrationPriority',
'QEMUChrEvent',
'QEMUClockType',
+ 'ResetType',
'device_endian',
'module_init_type',
]
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index dfe199ad0ed..259efacb046 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -13,7 +13,7 @@
c_str, impl_vmstate_forward,
irq::InterruptSource,
prelude::*,
- qdev::{Clock, ClockEvent, DeviceImpl},
+ qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
};
@@ -164,7 +164,10 @@ fn vmsd() -> Option<&'static VMStateDescription> {
Some(&device_class::VMSTATE_PL011)
}
const REALIZE: Option<fn(&mut Self)> = Some(Self::realize);
- const RESET: Option<fn(&Self)> = Some(Self::reset);
+}
+
+impl ResettablePhasesImpl for PL011State {
+ const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
}
impl PL011Registers {
@@ -601,7 +604,7 @@ pub fn realize(&mut self) {
}
}
- pub fn reset(&self) {
+ pub fn reset_hold(&self, _type: ResetType) {
self.regs.borrow_mut().reset();
}
@@ -723,3 +726,4 @@ impl ObjectImpl for PL011Luminary {
}
impl DeviceImpl for PL011Luminary {}
+impl ResettablePhasesImpl for PL011Luminary {}
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index ab883e2faef..8f0b279003a 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -9,19 +9,83 @@
os::raw::c_void,
};
-pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property};
+pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property, ResetType};
use crate::{
- bindings::{self, Error},
+ bindings::{self, Error, ResettableClass},
callbacks::FnCall,
cell::bql_locked,
prelude::*,
- qom::{ClassInitImpl, ObjectClass, Owned},
+ qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned},
vmstate::VMStateDescription,
};
+/// Trait providing the contents of [`ResettablePhases`].
+pub trait ResettablePhasesImpl {
+ /// If not None, this is called when the object enters reset. It
+ /// can reset local state of the object, but it must not do anything that
+ /// has a side-effect on other objects, such as raising or lowering a
+ /// [`qemu_irq`] line or reading or writing guest memory. It takes the
+ /// reset's type as argument.
+ const ENTER: Option<fn(&Self, ResetType)> = None;
+
+ /// If not None, this is called when the object for entry into reset, once
+ /// every object in the system which is being reset has had its
+ /// @phases.enter method called. At this point devices can do actions
+ /// that affect other objects.
+ ///
+ /// If in doubt, implement this method.
+ const HOLD: Option<fn(&Self, ResetType)> = None;
+
+ /// If not None, this phase is called when the object leaves the reset
+ /// state. Actions affecting other objects are permitted.
+ const EXIT: Option<fn(&Self, ResetType)> = None;
+}
+
+/// # Safety
+///
+/// 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_resettable_enter_fn<T: ResettablePhasesImpl>(
+ obj: *mut Object,
+ typ: ResetType,
+) {
+ assert!(!obj.is_null());
+ let state = obj.cast::<T>();
+ T::ENTER.unwrap()(unsafe { &mut *state }, typ);
+}
+
+/// # Safety
+///
+/// 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_resettable_hold_fn<T: ResettablePhasesImpl>(
+ obj: *mut Object,
+ typ: ResetType,
+) {
+ assert!(!obj.is_null());
+ let state = obj.cast::<T>();
+ T::HOLD.unwrap()(unsafe { &mut *state }, typ);
+}
+
+/// # Safety
+///
+/// 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_resettable_exit_fn<T: ResettablePhasesImpl>(
+ obj: *mut Object,
+ typ: ResetType,
+) {
+ assert!(!obj.is_null());
+ let state = obj.cast::<T>();
+ T::EXIT.unwrap()(unsafe { &mut *state }, typ);
+}
+
/// Trait providing the contents of [`DeviceClass`].
-pub trait DeviceImpl: ObjectImpl {
+pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl {
/// _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).
@@ -30,13 +94,6 @@ pub trait DeviceImpl: ObjectImpl {
/// with the function pointed to by `REALIZE`.
const REALIZE: Option<fn(&mut Self)> = None;
- /// If not `None`, the parent class's `reset` method is overridden
- /// with the function pointed to by `RESET`.
- ///
- /// Rust does not yet support the three-phase reset protocol; this is
- /// usually okay for leaf classes.
- const RESET: Option<fn(&Self)> = None;
-
/// An array providing the properties that the user can set on the
/// device. Not a `const` because referencing statics in constants
/// is unstable until Rust 1.83.0.
@@ -65,30 +122,36 @@ fn vmsd() -> Option<&'static VMStateDescription> {
T::REALIZE.unwrap()(unsafe { &mut *state });
}
-/// # Safety
-///
-/// 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_reset_fn<T: DeviceImpl>(dev: *mut DeviceState) {
- assert!(!dev.is_null());
- let state = dev.cast::<T>();
- T::RESET.unwrap()(unsafe { &mut *state });
+unsafe impl InterfaceType for ResettableClass {
+ const TYPE_NAME: &'static CStr =
+ 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) {
+ if <T as ResettablePhasesImpl>::ENTER.is_some() {
+ rc.phases.enter = Some(rust_resettable_enter_fn::<T>);
+ }
+ if <T as ResettablePhasesImpl>::HOLD.is_some() {
+ rc.phases.hold = Some(rust_resettable_hold_fn::<T>);
+ }
+ if <T as ResettablePhasesImpl>::EXIT.is_some() {
+ rc.phases.exit = Some(rust_resettable_exit_fn::<T>);
+ }
+ }
}
impl<T> ClassInitImpl<DeviceClass> for T
where
- T: ClassInitImpl<ObjectClass> + DeviceImpl,
+ T: ClassInitImpl<ObjectClass> + ClassInitImpl<ResettableClass> + DeviceImpl,
{
fn class_init(dc: &mut DeviceClass) {
if <T as DeviceImpl>::REALIZE.is_some() {
dc.realize = Some(rust_realize_fn::<T>);
}
- if <T as DeviceImpl>::RESET.is_some() {
- unsafe {
- bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
- }
- }
if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
dc.vmsd = vmsd;
}
@@ -99,6 +162,7 @@ fn class_init(dc: &mut DeviceClass) {
}
}
+ ResettableClass::interface_init::<T, DeviceState>(dc);
<T as ClassInitImpl<ObjectClass>>::class_init(&mut dc.parent_class);
}
}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 1944e65c8f3..178041ffaae 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},
+ qdev::{DeviceClass, DeviceImpl, DeviceState, Property, ResettablePhasesImpl},
qom::{ClassInitImpl, ObjectImpl, ParentField},
vmstate::VMStateDescription,
zeroable::Zeroable,
@@ -61,6 +61,8 @@ impl ObjectImpl for DummyState {
const ABSTRACT: bool = false;
}
+impl ResettablePhasesImpl for DummyState {}
+
impl DeviceImpl for DummyState {
fn properties() -> &'static [Property] {
&DUMMY_PROPERTIES
@@ -101,6 +103,7 @@ impl ObjectImpl for DummyChildState {
const ABSTRACT: bool = false;
}
+impl ResettablePhasesImpl for DummyChildState {}
impl DeviceImpl for DummyChildState {}
impl ClassInitImpl<DummyClass> for DummyChildState {
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable
2025-01-17 19:40 ` [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
@ 2025-01-27 10:31 ` Zhao Liu
2025-01-27 18:01 ` Paolo Bonzini
2025-02-06 8:31 ` Zhao Liu
1 sibling, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 10:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> +/// Trait providing the contents of [`ResettablePhases`].
> +pub trait ResettablePhasesImpl {
> + /// If not None, this is called when the object enters reset. It
> + /// can reset local state of the object, but it must not do anything that
> + /// has a side-effect on other objects, such as raising or lowering a
> + /// [`qemu_irq`] line or reading or writing guest memory. It takes the
> + /// reset's type as argument.
> + const ENTER: Option<fn(&Self, ResetType)> = None;
> +
> + /// If not None, this is called when the object for entry into reset, once
> + /// every object in the system which is being reset has had its
> + /// @phases.enter method called. At this point devices can do actions
Maybe s/@phases.enter/ResettablePhasesImpl::ENTER/?
> + /// that affect other objects.
> + ///
> + /// If in doubt, implement this method.
> + const HOLD: Option<fn(&Self, ResetType)> = None;
> +
> + /// If not None, this phase is called when the object leaves the reset
> + /// state. Actions affecting other objects are permitted.
> + const EXIT: Option<fn(&Self, ResetType)> = None;
> +}
> +
...
> impl<T> ClassInitImpl<DeviceClass> for T
> where
> - T: ClassInitImpl<ObjectClass> + DeviceImpl,
> + T: ClassInitImpl<ObjectClass> + ClassInitImpl<ResettableClass> + DeviceImpl,
This constraint requires each device to explicitly implement ResettablePhasesImpl,
even the device doesn't want to do anything for reset.
So what about the following changes:
* Define 3-Phases methods in DeviceImpl.
* Implement ResettablePhasesImpl for all devices by passing their 3-Phases
methods to ResettablePhasesImpl's.
Then device doesn't need to implement this reset trait if unnecessary.
For example:
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -84,7 +84,7 @@ pub trait ResettablePhasesImpl {
}
/// Trait providing the contents of [`DeviceClass`].
-pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl {
+pub trait DeviceImpl: ObjectImpl {
/// _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).
@@ -106,6 +106,18 @@ fn properties() -> &'static [Property] {
fn vmsd() -> Option<&'static VMStateDescription> {
None
}
+
+ const RESET_ENTER: Option<fn(&Self, ResetType)> = None;
+ const RESET_HOLD: Option<fn(&Self, ResetType)> = None;
+ const RESET_EXIT: Option<fn(&Self, ResetType)> = None;
+}
+
+impl<T> ResettablePhasesImpl for T
+where T: DeviceImpl
+{
+ const ENTER: Option<fn(&Self, ResetType)> = T::RESET_ENTER;
+ const HOLD: Option<fn(&Self, ResetType)> = T::RESET_HOLD;
+ const EXIT: Option<fn(&Self, ResetType)> = T::RESET_EXIT;
}
/// # Safety
---
Though this way add duplicate methods, it reduces the burden on the
device developer.
Regards,
Zhao
> {
> fn class_init(dc: &mut DeviceClass) {
> if <T as DeviceImpl>::REALIZE.is_some() {
> dc.realize = Some(rust_realize_fn::<T>);
> }
> - if <T as DeviceImpl>::RESET.is_some() {
> - unsafe {
> - bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
> - }
> - }
> if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
> dc.vmsd = vmsd;
> }
> @@ -99,6 +162,7 @@ fn class_init(dc: &mut DeviceClass) {
> }
> }
>
> + ResettableClass::interface_init::<T, DeviceState>(dc);
> <T as ClassInitImpl<ObjectClass>>::class_init(&mut dc.parent_class);
> }
> }
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable
2025-01-27 10:31 ` Zhao Liu
@ 2025-01-27 18:01 ` Paolo Bonzini
2025-01-28 9:25 ` Zhao Liu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-27 18:01 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On Mon, Jan 27, 2025 at 11:12 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > + /// If not None, this is called when the object for entry into reset, once
> > + /// every object in the system which is being reset has had its
> > + /// @phases.enter method called. At this point devices can do actions
>
> Maybe s/@phases.enter/ResettablePhasesImpl::ENTER/?
Yes.
> This constraint requires each device to explicitly implement ResettablePhasesImpl,
> even the device doesn't want to do anything for reset.
Yes, that's true but it's just a line of code. Almost all devices
*will* want to do
something with reset.
> So what about the following changes:
> * Define 3-Phases methods in DeviceImpl.
> * Implement ResettablePhasesImpl for all devices by passing their 3-Phases
> methods to ResettablePhasesImpl's.
>
> +impl<T> ResettablePhasesImpl for T
> +where T: DeviceImpl
> +{
> + const ENTER: Option<fn(&Self, ResetType)> = T::RESET_ENTER;
> + const HOLD: Option<fn(&Self, ResetType)> = T::RESET_HOLD;
> + const EXIT: Option<fn(&Self, ResetType)> = T::RESET_EXIT;
> }
>
> /// # Safety
>
> ---
>
> Then device doesn't need to implement this reset trait if unnecessary.
> Though this way add duplicate methods, it reduces the burden on the
> device developer.
For now I prefer to make things homogeneous... this way if someone has
to copy the code into a wrapper for a new interface they don't have to
wonder about small differences.
(This by the way will also be a reason to use function pointers for
character devices as well, instead of the trait approach that I used
in https://lists.nongnu.org/archive/html/qemu-rust/2024-12/msg00006.html).
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable
2025-01-17 19:40 ` [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
2025-01-27 10:31 ` Zhao Liu
@ 2025-02-06 8:31 ` Zhao Liu
1 sibling, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-02-06 8:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:40:01PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:40:01 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable
> X-Mailer: git-send-email 2.47.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> meson.build | 1 +
> rust/hw/char/pl011/src/device.rs | 10 ++-
> rust/qemu-api/src/qdev.rs | 116 ++++++++++++++++++++++++-------
> rust/qemu-api/tests/tests.rs | 5 +-
> 4 files changed, 102 insertions(+), 30 deletions(-)
>
Add my R/b tag,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 09/10] rust: bindings: add Sync markers to types referred to by MemoryRegionOps
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (7 preceding siblings ...)
2025-01-17 19:40 ` [PATCH 08/10] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
@ 2025-01-17 19:40 ` Paolo Bonzini
2025-01-27 10:58 ` Zhao Liu
2025-01-17 19:40 ` [PATCH 10/10] rust: bindings for MemoryRegionOps Paolo Bonzini
2025-01-24 2:46 ` [RFC PATCH 00/10] rust: remaining part of qdev bindings Zhao Liu
10 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:40 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
This is needed for the MemoryRegionOps<T> to be declared as static;
Rust requires static elements to be Sync.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 19 ++++++++++++++++++-
rust/qemu-api/src/irq.rs | 3 +++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 8a9b821bb91..3f61264ab80 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -21,7 +21,24 @@
#[cfg(not(MESON))]
include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
-unsafe impl Send for Property {}
+// 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 Sync for BusState {}
+unsafe impl Sync for CharBackend {}
+unsafe impl Sync for Chardev {}
+unsafe impl Sync for Clock {}
+unsafe impl Sync for DeviceState {}
+unsafe impl Sync for MemoryRegion {}
+unsafe impl Sync for ObjectClass {}
+unsafe impl Sync for Object {}
+unsafe impl Sync for SysBusDevice {}
+
+// SAFETY: this is a pure data struct
+unsafe impl Sync for CoalescedMemoryRange {}
+
+// SAFETY: these are constants and vtables; the Sync requirements are deferred
+// to the unsafe callbacks that they contain
+unsafe impl Sync for MemoryRegionOps {}
unsafe impl Sync for Property {}
unsafe impl Sync for TypeInfo {}
unsafe impl Sync for VMStateDescription {}
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 378e5202951..638545c3a64 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -43,6 +43,9 @@ pub struct InterruptSource<T = bool>
_marker: PhantomData<T>,
}
+// SAFETY: the implementation asserts via `BqlCell` that the BQL is taken
+unsafe impl<T> Sync for InterruptSource<T> where c_int: From<T> {}
+
impl InterruptSource<bool> {
/// Send a low (`false`) value to the interrupt sink.
pub fn lower(&self) {
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 09/10] rust: bindings: add Sync markers to types referred to by MemoryRegionOps
2025-01-17 19:40 ` [PATCH 09/10] rust: bindings: add Sync markers to types referred to by MemoryRegionOps Paolo Bonzini
@ 2025-01-27 10:58 ` Zhao Liu
0 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 10:58 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:40:02PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:40:02 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 09/10] rust: bindings: add Sync markers to types referred
> to by MemoryRegionOps
> X-Mailer: git-send-email 2.47.1
>
> This is needed for the MemoryRegionOps<T> to be declared as static;
> Rust requires static elements to be Sync.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/qemu-api/src/bindings.rs | 19 ++++++++++++++++++-
> rust/qemu-api/src/irq.rs | 3 +++
> 2 files changed, 21 insertions(+), 1 deletion(-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
> index 8a9b821bb91..3f61264ab80 100644
> --- a/rust/qemu-api/src/bindings.rs
> +++ b/rust/qemu-api/src/bindings.rs
> @@ -21,7 +21,24 @@
> #[cfg(not(MESON))]
> include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
>
> -unsafe impl Send for Property {}
I guess this is a rebase mistake since latest rust-next drops this
change :-).
> +// 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 Sync for BusState {}
We haven't used BusState but it's fine, as the basic support, and after
all bindings file has this type.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (8 preceding siblings ...)
2025-01-17 19:40 ` [PATCH 09/10] rust: bindings: add Sync markers to types referred to by MemoryRegionOps Paolo Bonzini
@ 2025-01-17 19:40 ` Paolo Bonzini
2025-01-27 12:12 ` Zhao Liu
2025-02-06 8:39 ` Philippe Mathieu-Daudé
2025-01-24 2:46 ` [RFC PATCH 00/10] rust: remaining part of qdev bindings Zhao Liu
10 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-17 19:40 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 43 +++---
rust/hw/char/pl011/src/lib.rs | 1 -
rust/hw/char/pl011/src/memory_ops.rs | 36 -----
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
rust/qemu-api/src/sysbus.rs | 7 +-
rust/qemu-api/src/zeroable.rs | 12 ++
8 files changed, 234 insertions(+), 58 deletions(-)
delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
create mode 100644 rust/qemu-api/src/memory.rs
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 259efacb046..294394c6e82 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,7 +2,7 @@
// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
// SPDX-License-Identifier: GPL-2.0-or-later
-use core::ptr::{addr_of_mut, NonNull};
+use core::ptr::{addr_of, addr_of_mut, NonNull};
use std::{
ffi::CStr,
os::raw::{c_int, c_void},
@@ -12,14 +12,14 @@
bindings::{self, *},
c_str, impl_vmstate_forward,
irq::InterruptSource,
+ memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
prelude::*,
- qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
+ qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
};
use crate::{
device_class,
- memory_ops::PL011_OPS,
registers::{self, Interrupt},
RegisterOffset,
};
@@ -490,20 +490,24 @@ impl PL011State {
/// location/instance. All its fields are expected to hold unitialized
/// values with the sole exception of `parent_obj`.
unsafe fn init(&mut self) {
+ static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
+ .read(&PL011State::read)
+ .write(&PL011State::write)
+ .native_endian()
+ .impl_sizes(4, 4)
+ .build();
+
// SAFETY:
//
// self and self.iomem are guaranteed to be valid at this point since callers
// must make sure the `self` reference is valid.
- unsafe {
- memory_region_init_io(
- addr_of_mut!(self.iomem),
- addr_of_mut!(*self).cast::<Object>(),
- &PL011_OPS,
- addr_of_mut!(*self).cast::<c_void>(),
- Self::TYPE_NAME.as_ptr(),
- 0x1000,
- );
- }
+ MemoryRegion::init_io(
+ unsafe { &mut *addr_of_mut!(self.iomem) },
+ addr_of_mut!(*self),
+ &PL011_OPS,
+ "pl011",
+ 0x1000,
+ );
self.regs = Default::default();
@@ -528,8 +532,7 @@ fn post_init(&self) {
}
}
- #[allow(clippy::needless_pass_by_ref_mut)]
- pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
+ pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
let (update_irq, result) = match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
let device_id = self.get_class().device_id;
@@ -544,16 +547,20 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
if update_irq {
self.update();
unsafe {
- qemu_chr_fe_accept_input(&mut self.char_backend);
+ qemu_chr_fe_accept_input(addr_of!(self.char_backend) as *mut _);
}
}
result.into()
}
- pub fn write(&mut self, offset: hwaddr, value: u64) {
+ pub fn write(&self, offset: hwaddr, value: u64, _size: u32) {
let mut update_irq = false;
if let Ok(field) = RegisterOffset::try_from(offset) {
- update_irq = self.regs.borrow_mut().write(field, value as u32, &mut self.char_backend);
+ update_irq = self.regs.borrow_mut().write(
+ field,
+ value as u32,
+ addr_of!(self.char_backend) as *mut _,
+ );
} else {
eprintln!("write bad offset {offset} value {value}");
}
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 300c732ae1d..5622e974cbc 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -29,7 +29,6 @@
mod device;
mod device_class;
-mod memory_ops;
pub use device::pl011_create;
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
deleted file mode 100644
index 95b4df794e4..00000000000
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ /dev/null
@@ -1,36 +0,0 @@
-// Copyright 2024, Linaro Limited
-// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
-// SPDX-License-Identifier: GPL-2.0-or-later
-
-use core::ptr::NonNull;
-use std::os::raw::{c_uint, c_void};
-
-use qemu_api::{bindings::*, zeroable::Zeroable};
-
-use crate::device::PL011State;
-
-pub static PL011_OPS: MemoryRegionOps = MemoryRegionOps {
- read: Some(pl011_read),
- write: Some(pl011_write),
- read_with_attrs: None,
- write_with_attrs: None,
- endianness: device_endian::DEVICE_NATIVE_ENDIAN,
- valid: Zeroable::ZERO,
- impl_: MemoryRegionOps__bindgen_ty_2 {
- min_access_size: 4,
- max_access_size: 4,
- ..Zeroable::ZERO
- },
-};
-
-unsafe extern "C" fn pl011_read(opaque: *mut c_void, addr: hwaddr, size: c_uint) -> u64 {
- assert!(!opaque.is_null());
- let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
- unsafe { state.as_mut() }.read(addr, size)
-}
-
-unsafe extern "C" fn pl011_write(opaque: *mut c_void, addr: hwaddr, data: u64, _size: c_uint) {
- assert!(!opaque.is_null());
- let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
- unsafe { state.as_mut() }.write(addr, data);
-}
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 60944a657de..80eafc7f6bd 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -22,6 +22,7 @@ _qemu_api_rs = static_library(
'src/cell.rs',
'src/c_str.rs',
'src/irq.rs',
+ 'src/memory.rs',
'src/module.rs',
'src/offset_of.rs',
'src/prelude.rs',
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 83c6a987c05..8cc095b13f6 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -18,6 +18,7 @@
pub mod callbacks;
pub mod cell;
pub mod irq;
+pub mod memory;
pub mod module;
pub mod offset_of;
pub mod qdev;
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
new file mode 100644
index 00000000000..963d689c27d
--- /dev/null
+++ b/rust/qemu-api/src/memory.rs
@@ -0,0 +1,191 @@
+// Copyright 2024 Red Hat, Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Bindings for `MemoryRegion` and `MemoryRegionOps`
+
+use std::{
+ ffi::{CStr, CString},
+ marker::{PhantomData, PhantomPinned},
+ os::raw::{c_uint, c_void},
+ ptr::addr_of,
+};
+
+pub use bindings::hwaddr;
+
+use crate::{
+ bindings::{self, device_endian, memory_region_init_io},
+ callbacks::FnCall,
+ prelude::*,
+ zeroable::Zeroable,
+};
+
+pub struct MemoryRegionOps<T>(
+ bindings::MemoryRegionOps,
+ // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
+ // covariance and contravariance; you don't need any of those to understand
+ // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
+ // holds callbacks that take an argument of type &T, except the type is erased
+ // before the callback is stored in the bindings::MemoryRegionOps field.
+ // The argument of PhantomData is a function pointer in order to represent
+ // that relationship; while that will also provide desirable and safe variance
+ // for T, variance is not the point but just a consequence.
+ PhantomData<fn(&T)>,
+);
+
+// SAFETY: When a *const T is passed to the callbacks, the call itself
+// is done in a thread-safe manner. The invocation is okay as long as
+// T itself is `Sync`.
+unsafe impl<T: Sync> Sync for MemoryRegionOps<T> {}
+
+#[derive(Clone)]
+pub struct MemoryRegionOpsBuilder<T>(bindings::MemoryRegionOps, PhantomData<fn(&T)>);
+
+unsafe extern "C" fn memory_region_ops_read_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ size: c_uint,
+) -> u64 {
+ F::call((unsafe { &*(opaque.cast::<T>()) }, addr, size))
+}
+
+unsafe extern "C" fn memory_region_ops_write_cb<T, F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(
+ opaque: *mut c_void,
+ addr: hwaddr,
+ data: u64,
+ size: c_uint,
+) {
+ F::call((unsafe { &*(opaque.cast::<T>()) }, addr, data, size))
+}
+
+impl<T> MemoryRegionOpsBuilder<T> {
+ #[must_use]
+ pub const fn read<F: for<'a> FnCall<(&'a T, hwaddr, u32), u64>>(mut self, _f: &F) -> Self {
+ self.0.read = Some(memory_region_ops_read_cb::<T, F>);
+ self
+ }
+
+ #[must_use]
+ pub const fn write<F: for<'a> FnCall<(&'a T, hwaddr, u64, u32)>>(mut self, _f: &F) -> Self {
+ self.0.write = Some(memory_region_ops_write_cb::<T, F>);
+ self
+ }
+
+ #[must_use]
+ pub const fn big_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_BIG_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn little_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_LITTLE_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn native_endian(mut self) -> Self {
+ self.0.endianness = device_endian::DEVICE_NATIVE_ENDIAN;
+ self
+ }
+
+ #[must_use]
+ pub const fn valid_sizes(mut self, min: u32, max: u32) -> Self {
+ self.0.valid.min_access_size = min;
+ self.0.valid.max_access_size = max;
+ self
+ }
+
+ #[must_use]
+ pub const fn valid_unaligned(mut self) -> Self {
+ self.0.valid.unaligned = true;
+ self
+ }
+
+ #[must_use]
+ pub const fn impl_sizes(mut self, min: u32, max: u32) -> Self {
+ self.0.impl_.min_access_size = min;
+ self.0.impl_.max_access_size = max;
+ self
+ }
+
+ #[must_use]
+ pub const fn impl_unaligned(mut self) -> Self {
+ self.0.impl_.unaligned = true;
+ self
+ }
+
+ #[must_use]
+ pub const fn build(self) -> MemoryRegionOps<T> {
+ MemoryRegionOps::<T>(self.0, PhantomData)
+ }
+
+ #[must_use]
+ pub const fn new() -> Self {
+ Self(bindings::MemoryRegionOps::ZERO, PhantomData)
+ }
+}
+
+impl<T> Default for MemoryRegionOpsBuilder<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+/// 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,
+}
+
+impl MemoryRegion {
+ // inline to ensure that it is not included in tests, which only
+ // link to hwcore and qom. FIXME: inlining is actually the opposite
+ // of what we want, since this is the type-erased version of the
+ // init_io function below. Look into splitting the qemu_api crate.
+ #[inline(always)]
+ unsafe fn do_init_io(
+ slot: *mut bindings::MemoryRegion,
+ owner: *mut Object,
+ ops: &'static bindings::MemoryRegionOps,
+ name: &'static str,
+ size: u64,
+ ) {
+ unsafe {
+ let cstr = CString::new(name).unwrap();
+ memory_region_init_io(
+ slot,
+ owner.cast::<Object>(),
+ ops,
+ owner.cast::<c_void>(),
+ cstr.as_ptr(),
+ size,
+ );
+ }
+ }
+
+ pub fn init_io<T: IsA<Object>>(
+ &mut self,
+ owner: *mut T,
+ ops: &'static MemoryRegionOps<T>,
+ name: &'static str,
+ size: u64,
+ ) {
+ unsafe {
+ Self::do_init_io(&mut self.inner, 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 {
+ type Class = bindings::MemoryRegionClass;
+ const TYPE_NAME: &'static CStr =
+ unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_MEMORY_REGION) };
+}
+qom_isa!(MemoryRegion: Object);
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index e6762b5c145..c27dbf79e43 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -2,7 +2,7 @@
// Author(s): Paolo Bonzini <pbonzini@redhat.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::{ffi::CStr, ptr::addr_of};
+use std::ffi::CStr;
pub use bindings::{SysBusDevice, SysBusDeviceClass};
@@ -10,6 +10,7 @@
bindings,
cell::bql_locked,
irq::InterruptSource,
+ memory::MemoryRegion,
prelude::*,
qdev::{DeviceClass, DeviceState},
qom::ClassInitImpl,
@@ -42,10 +43,10 @@ pub trait SysBusDeviceMethods: ObjectDeref
/// important, since whoever creates the sysbus device will refer to the
/// region with a number that corresponds to the order of calls to
/// `init_mmio`.
- fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
+ fn init_mmio(&self, iomem: &MemoryRegion) {
assert!(bql_locked());
unsafe {
- bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _);
+ bindings::sysbus_init_mmio(self.as_mut_ptr(), iomem.as_mut_ptr());
}
}
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 57cac96de06..0208381ee38 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -99,6 +99,18 @@ unsafe impl Zeroable for crate::bindings::VMStateDescription {
};
}
+unsafe impl Zeroable for crate::bindings::MemoryRegionOps {
+ const ZERO: Self = Self {
+ read: None,
+ write: None,
+ read_with_attrs: None,
+ write_with_attrs: None,
+ endianness: crate::bindings::device_endian::DEVICE_NATIVE_ENDIAN,
+ valid: Zeroable::ZERO,
+ impl_: Zeroable::ZERO,
+ };
+}
+
unsafe impl Zeroable for crate::bindings::MemoryRegionOps__bindgen_ty_1 {
const ZERO: Self = Self {
min_access_size: 0,
--
2.47.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-01-17 19:40 ` [PATCH 10/10] rust: bindings for MemoryRegionOps Paolo Bonzini
@ 2025-01-27 12:12 ` Zhao Liu
2025-01-27 18:11 ` Paolo Bonzini
2025-02-06 8:39 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-01-27 12:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:40:03PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:40:03 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/10] rust: bindings for MemoryRegionOps
> X-Mailer: git-send-email 2.47.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 43 +++---
> rust/hw/char/pl011/src/lib.rs | 1 -
> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> rust/qemu-api/meson.build | 1 +
> rust/qemu-api/src/lib.rs | 1 +
> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> rust/qemu-api/src/sysbus.rs | 7 +-
> rust/qemu-api/src/zeroable.rs | 12 ++
> 8 files changed, 234 insertions(+), 58 deletions(-)
> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> create mode 100644 rust/qemu-api/src/memory.rs
...
> @@ -490,20 +490,24 @@ impl PL011State {
> /// location/instance. All its fields are expected to hold unitialized
> /// values with the sole exception of `parent_obj`.
> unsafe fn init(&mut self) {
> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> + .read(&PL011State::read)
> + .write(&PL011State::write)
> + .native_endian()
> + .impl_sizes(4, 4)
> + .build();
> +
Nice design. Everything was done smoothly in one go.
> +pub struct MemoryRegionOps<T>(
> + bindings::MemoryRegionOps,
> + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> + // covariance and contravariance; you don't need any of those to understand
> + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> + // holds callbacks that take an argument of type &T, except the type is erased
> + // before the callback is stored in the bindings::MemoryRegionOps field.
> + // The argument of PhantomData is a function pointer in order to represent
> + // that relationship; while that will also provide desirable and safe variance
> + // for T, variance is not the point but just a consequence.
> + PhantomData<fn(&T)>,
> +);
Wow, it can be wrapped like this!
> +}
> +
> +/// 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,
> +}
> +
> +impl MemoryRegion {
> + // inline to ensure that it is not included in tests, which only
> + // link to hwcore and qom. FIXME: inlining is actually the opposite
> + // of what we want, since this is the type-erased version of the
> + // init_io function below. Look into splitting the qemu_api crate.
Ah, I didn't understand the issue described in this comment. Why would
inlining affect the linking of tests?
> + #[inline(always)]
> + unsafe fn do_init_io(
> + slot: *mut bindings::MemoryRegion,
> + owner: *mut Object,
> + ops: &'static bindings::MemoryRegionOps,
> + name: &'static str,
> + size: u64,
> + ) {
> + unsafe {
> + let cstr = CString::new(name).unwrap();
> + memory_region_init_io(
> + slot,
> + owner.cast::<Object>(),
> + ops,
> + owner.cast::<c_void>(),
> + cstr.as_ptr(),
> + size,
> + );
> + }
> + }
> +
> + pub fn init_io<T: IsA<Object>>(
> + &mut self,
> + owner: *mut T,
> + ops: &'static MemoryRegionOps<T>,
> + name: &'static str,
What about &'static CStr?
Then pl011 could pass `c_str!("pl011")` or `Self::TYPE_NAMSelf::TYPE_NAME`.
Otherwise,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-01-27 12:12 ` Zhao Liu
@ 2025-01-27 18:11 ` Paolo Bonzini
2025-02-06 9:15 ` Zhao Liu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-01-27 18:11 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On Mon, Jan 27, 2025 at 12:53 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > @@ -490,20 +490,24 @@ impl PL011State {
> > /// location/instance. All its fields are expected to hold unitialized
> > /// values with the sole exception of `parent_obj`.
> > unsafe fn init(&mut self) {
> > + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> > + .read(&PL011State::read)
> > + .write(&PL011State::write)
> > + .native_endian()
> > + .impl_sizes(4, 4)
> > + .build();
> > +
>
> Nice design. Everything was done smoothly in one go.
I hope something similar can be done with VMStateDescription too...
> > +pub struct MemoryRegionOps<T>(
> > + bindings::MemoryRegionOps,
> > + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> > + // covariance and contravariance; you don't need any of those to understand
> > + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> > + // holds callbacks that take an argument of type &T, except the type is erased
> > + // before the callback is stored in the bindings::MemoryRegionOps field.
> > + // The argument of PhantomData is a function pointer in order to represent
> > + // that relationship; while that will also provide desirable and safe variance
> > + // for T, variance is not the point but just a consequence.
> > + PhantomData<fn(&T)>,
> > +);
>
> Wow, it can be wrapped like this!
I like your enthusiasm but I'm not sure what you refer to. ;) Maybe
it's worth documenting this pattern, so please tell me more (after
your holidays).
> > +impl MemoryRegion {
> > + // inline to ensure that it is not included in tests, which only
> > + // link to hwcore and qom. FIXME: inlining is actually the opposite
> > + // of what we want, since this is the type-erased version of the
> > + // init_io function below. Look into splitting the qemu_api crate.
>
> Ah, I didn't understand the issue described in this comment. Why would
> inlining affect the linking of tests?
If you don't inline it, do_init_io will always be linked into the
tests because it is a non-generic function. The tests then fail to
link, because memory_region_init_io is undefined.
This is ugly because do_init_io exists *exactly* to extract the part
that is not generic. (See
https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
for an example of this; I think there's even a procedural macro crate
that does that for you, but I can't find it right now).
> > + pub fn init_io<T: IsA<Object>>(
> > + &mut self,
> > + owner: *mut T,
> > + ops: &'static MemoryRegionOps<T>,
> > + name: &'static str,
>
> What about &'static CStr?
>
> Then pl011 could pass `c_str!("pl011")` or `Self::TYPE_NAME`.
I think it's better to use a Rust string; there's no reason why the
name of the memory region has to match Self::TYPE_NAME; unlike the
name of the device, the name of the memory region is not visible on
the command line for example.
Thanks,
Paolo
> Otherwise,
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-01-27 18:11 ` Paolo Bonzini
@ 2025-02-06 9:15 ` Zhao Liu
2025-02-06 9:15 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Zhao Liu @ 2025-02-06 9:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> > > +pub struct MemoryRegionOps<T>(
> > > + bindings::MemoryRegionOps,
> > > + // Note: quite often you'll see PhantomData<fn(&T)> mentioned when discussing
> > > + // covariance and contravariance; you don't need any of those to understand
> > > + // this usage of PhantomData. Quite simply, MemoryRegionOps<T> *logically*
> > > + // holds callbacks that take an argument of type &T, except the type is erased
> > > + // before the callback is stored in the bindings::MemoryRegionOps field.
> > > + // The argument of PhantomData is a function pointer in order to represent
> > > + // that relationship; while that will also provide desirable and safe variance
> > > + // for T, variance is not the point but just a consequence.
> > > + PhantomData<fn(&T)>,
> > > +);
> >
> > Wow, it can be wrapped like this!
>
> I like your enthusiasm but I'm not sure what you refer to. ;) Maybe
> it's worth documenting this pattern, so please tell me more (after
> your holidays).
Throughout the entire holiday, I couldn't think of a better way to
express this. I find it particularly useful when wrapping multiple
callbacks. In the future, I want to explore more use cases where this
pattern can be applied.
> > > +impl MemoryRegion {
> > > + // inline to ensure that it is not included in tests, which only
> > > + // link to hwcore and qom. FIXME: inlining is actually the opposite
> > > + // of what we want, since this is the type-erased version of the
> > > + // init_io function below. Look into splitting the qemu_api crate.
> >
> > Ah, I didn't understand the issue described in this comment. Why would
> > inlining affect the linking of tests?
>
> If you don't inline it, do_init_io will always be linked into the
> tests because it is a non-generic function. The tests then fail to
> link, because memory_region_init_io is undefined.
I find even if I drop the `inline` attribution, the test.rs can still be
compiled (by `make check`), I think it's because test.rs hasn't involved
memory related tests so that do_init_io isn't linked into test.rs.
> This is ugly because do_init_io exists *exactly* to extract the part
> that is not generic. (See
> https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
> for an example of this; I think there's even a procedural macro crate
> that does that for you, but I can't find it right now).
Thanks! I see. I agree to keep `inline` anyway.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-02-06 9:15 ` Zhao Liu
@ 2025-02-06 9:15 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-06 9:15 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 2/6/25 10:15, Zhao Liu wrote:
> Throughout the entire holiday, I couldn't think of a better way to
> express this. I find it particularly useful when wrapping multiple
> callbacks. In the future, I want to explore more use cases where this
> pattern can be applied.
Thanks very much. Despite having written most of the bindings code, I
don't want to give the impression that everything that I send to the
mailing list is gold. So it matters to me that you send serious
reviews, and that you can learn something but also suggest
clarifications and improvements.
> I find even if I drop the `inline` attribution, the test.rs can still be
> compiled (by `make check`), I think it's because test.rs hasn't involved
> memory related tests so that do_init_io isn't linked into test.rs.
Hmm, maybe it's only with some linkers. I don't remember.
>> This is ugly because do_init_io exists *exactly* to extract the part
>> that is not generic. (See
>> https://users.rust-lang.org/t/soft-question-significantly-improve-rust-compile-time-via-minimizing-generics/103632/8
>> for an example of this; I think there's even a procedural macro crate
>> that does that for you, but I can't find it right now).
>
> Thanks! I see. I agree to keep `inline` anyway.
In the meanwhile I found it, it's
https://github.com/llogiq/momo/blob/master/wasm/src/lib.rs. QEMU could
add something like that macro to qemu_api_macros, so that
#[qemu_api_macros::upcast]
fn foo<T>(t: &T) where T: IsA<U> {
}
could be converted to
#[inline(always)]
fn foo<T>(&self) where T: IsA<U> {
fn inner_foo(u: &U) {
}
inner_foo(self.upcast())
}
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-01-17 19:40 ` [PATCH 10/10] rust: bindings for MemoryRegionOps Paolo Bonzini
2025-01-27 12:12 ` Zhao Liu
@ 2025-02-06 8:39 ` Philippe Mathieu-Daudé
2025-02-06 8:46 ` Paolo Bonzini
1 sibling, 1 reply; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-06 8:39 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-rust
Hi Paolo,
On 17/1/25 20:40, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 43 +++---
> rust/hw/char/pl011/src/lib.rs | 1 -
> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> rust/qemu-api/meson.build | 1 +
> rust/qemu-api/src/lib.rs | 1 +
> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> rust/qemu-api/src/sysbus.rs | 7 +-
> rust/qemu-api/src/zeroable.rs | 12 ++
> 8 files changed, 234 insertions(+), 58 deletions(-)
> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> create mode 100644 rust/qemu-api/src/memory.rs
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 259efacb046..294394c6e82 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -2,7 +2,7 @@
> // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> // SPDX-License-Identifier: GPL-2.0-or-later
>
> -use core::ptr::{addr_of_mut, NonNull};
> +use core::ptr::{addr_of, addr_of_mut, NonNull};
> use std::{
> ffi::CStr,
> os::raw::{c_int, c_void},
> @@ -12,14 +12,14 @@
> bindings::{self, *},
> c_str, impl_vmstate_forward,
> irq::InterruptSource,
> + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
> prelude::*,
> - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
> + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
> qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
> };
>
> use crate::{
> device_class,
> - memory_ops::PL011_OPS,
> registers::{self, Interrupt},
> RegisterOffset,
> };
> @@ -490,20 +490,24 @@ impl PL011State {
> /// location/instance. All its fields are expected to hold unitialized
> /// values with the sole exception of `parent_obj`.
> unsafe fn init(&mut self) {
> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> + .read(&PL011State::read)
> + .write(&PL011State::write)
> + .native_endian()
Could we always make .valid_sizes() explicit?
> + .impl_sizes(4, 4)
> + .build();
> +
> // SAFETY:
> //
> // self and self.iomem are guaranteed to be valid at this point since callers
> // must make sure the `self` reference is valid.
> - unsafe {
> - memory_region_init_io(
> - addr_of_mut!(self.iomem),
> - addr_of_mut!(*self).cast::<Object>(),
> - &PL011_OPS,
> - addr_of_mut!(*self).cast::<c_void>(),
> - Self::TYPE_NAME.as_ptr(),
> - 0x1000,
> - );
> - }
> + MemoryRegion::init_io(
> + unsafe { &mut *addr_of_mut!(self.iomem) },
> + addr_of_mut!(*self),
> + &PL011_OPS,
> + "pl011",
> + 0x1000,
> + );
> diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
> index 300c732ae1d..5622e974cbc 100644
> --- a/rust/hw/char/pl011/src/lib.rs
> +++ b/rust/hw/char/pl011/src/lib.rs
> @@ -29,7 +29,6 @@
>
> mod device;
> mod device_class;
> -mod memory_ops;
>
> pub use device::pl011_create;
>
> diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
> deleted file mode 100644
> index 95b4df794e4..00000000000
> --- a/rust/hw/char/pl011/src/memory_ops.rs
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -// Copyright 2024, Linaro Limited
> -// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -
> -use core::ptr::NonNull;
> -use std::os::raw::{c_uint, c_void};
> -
> -use qemu_api::{bindings::*, zeroable::Zeroable};
> -
> -use crate::device::PL011State;
> -
> -pub static PL011_OPS: MemoryRegionOps = MemoryRegionOps {
> - read: Some(pl011_read),
> - write: Some(pl011_write),
> - read_with_attrs: None,
> - write_with_attrs: None,
> - endianness: device_endian::DEVICE_NATIVE_ENDIAN,
> - valid: Zeroable::ZERO,
> - impl_: MemoryRegionOps__bindgen_ty_2 {
> - min_access_size: 4,
> - max_access_size: 4,
> - ..Zeroable::ZERO
> - },
> -};
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-02-06 8:39 ` Philippe Mathieu-Daudé
@ 2025-02-06 8:46 ` Paolo Bonzini
2025-02-06 10:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-06 8:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-rust
On Thu, Feb 6, 2025 at 9:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Paolo,
>
> On 17/1/25 20:40, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > rust/hw/char/pl011/src/device.rs | 43 +++---
> > rust/hw/char/pl011/src/lib.rs | 1 -
> > rust/hw/char/pl011/src/memory_ops.rs | 36 -----
> > rust/qemu-api/meson.build | 1 +
> > rust/qemu-api/src/lib.rs | 1 +
> > rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
> > rust/qemu-api/src/sysbus.rs | 7 +-
> > rust/qemu-api/src/zeroable.rs | 12 ++
> > 8 files changed, 234 insertions(+), 58 deletions(-)
> > delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> > create mode 100644 rust/qemu-api/src/memory.rs
> >
> > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> > index 259efacb046..294394c6e82 100644
> > --- a/rust/hw/char/pl011/src/device.rs
> > +++ b/rust/hw/char/pl011/src/device.rs
> > @@ -2,7 +2,7 @@
> > // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > // SPDX-License-Identifier: GPL-2.0-or-later
> >
> > -use core::ptr::{addr_of_mut, NonNull};
> > +use core::ptr::{addr_of, addr_of_mut, NonNull};
> > use std::{
> > ffi::CStr,
> > os::raw::{c_int, c_void},
> > @@ -12,14 +12,14 @@
> > bindings::{self, *},
> > c_str, impl_vmstate_forward,
> > irq::InterruptSource,
> > + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
> > prelude::*,
> > - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
> > + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
> > qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
> > };
> >
> > use crate::{
> > device_class,
> > - memory_ops::PL011_OPS,
> > registers::{self, Interrupt},
> > RegisterOffset,
> > };
> > @@ -490,20 +490,24 @@ impl PL011State {
> > /// location/instance. All its fields are expected to hold unitialized
> > /// values with the sole exception of `parent_obj`.
> > unsafe fn init(&mut self) {
> > + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
> > + .read(&PL011State::read)
> > + .write(&PL011State::write)
> > + .native_endian()
>
> Could we always make .valid_sizes() explicit?
Yes (for example build() could even fail to compile if you don't have
impl_sizes/valid_sizes set), but why do you want that? I'm not even
sure that all cases of .valid.max_access_size=4 are correct...
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-02-06 8:46 ` Paolo Bonzini
@ 2025-02-06 10:02 ` Philippe Mathieu-Daudé
2025-02-06 10:19 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-06 10:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On 6/2/25 09:46, Paolo Bonzini wrote:
> On Thu, Feb 6, 2025 at 9:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Paolo,
>>
>> On 17/1/25 20:40, Paolo Bonzini wrote:
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>> rust/hw/char/pl011/src/device.rs | 43 +++---
>>> rust/hw/char/pl011/src/lib.rs | 1 -
>>> rust/hw/char/pl011/src/memory_ops.rs | 36 -----
>>> rust/qemu-api/meson.build | 1 +
>>> rust/qemu-api/src/lib.rs | 1 +
>>> rust/qemu-api/src/memory.rs | 191 +++++++++++++++++++++++++++
>>> rust/qemu-api/src/sysbus.rs | 7 +-
>>> rust/qemu-api/src/zeroable.rs | 12 ++
>>> 8 files changed, 234 insertions(+), 58 deletions(-)
>>> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
>>> create mode 100644 rust/qemu-api/src/memory.rs
>>>
>>> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
>>> index 259efacb046..294394c6e82 100644
>>> --- a/rust/hw/char/pl011/src/device.rs
>>> +++ b/rust/hw/char/pl011/src/device.rs
>>> @@ -2,7 +2,7 @@
>>> // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> // SPDX-License-Identifier: GPL-2.0-or-later
>>>
>>> -use core::ptr::{addr_of_mut, NonNull};
>>> +use core::ptr::{addr_of, addr_of_mut, NonNull};
>>> use std::{
>>> ffi::CStr,
>>> os::raw::{c_int, c_void},
>>> @@ -12,14 +12,14 @@
>>> bindings::{self, *},
>>> c_str, impl_vmstate_forward,
>>> irq::InterruptSource,
>>> + memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
>>> prelude::*,
>>> - qdev::{Clock, ClockEvent, DeviceImpl, ResettablePhasesImpl, ResetType},
>>> + qdev::{Clock, ClockEvent, DeviceImpl, ResetType, ResettablePhasesImpl},
>>> qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
>>> };
>>>
>>> use crate::{
>>> device_class,
>>> - memory_ops::PL011_OPS,
>>> registers::{self, Interrupt},
>>> RegisterOffset,
>>> };
>>> @@ -490,20 +490,24 @@ impl PL011State {
>>> /// location/instance. All its fields are expected to hold unitialized
>>> /// values with the sole exception of `parent_obj`.
>>> unsafe fn init(&mut self) {
>>> + static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
>>> + .read(&PL011State::read)
>>> + .write(&PL011State::write)
>>> + .native_endian()
>>
>> Could we always make .valid_sizes() explicit?
>
> Yes (for example build() could even fail to compile if you don't have
> impl_sizes/valid_sizes set), but why do you want that? I'm not even
> sure that all cases of .valid.max_access_size=4 are correct...
Exactly for that :) Not have implicit default values, so correct
values are reviewed when models are added.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-02-06 10:02 ` Philippe Mathieu-Daudé
@ 2025-02-06 10:19 ` Paolo Bonzini
2025-02-10 10:38 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2025-02-06 10:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-rust
On 2/6/25 11:02, Philippe Mathieu-Daudé wrote:
>>> Could we always make .valid_sizes() explicit?
>>
>> Yes (for example build() could even fail to compile if you don't have
>> impl_sizes/valid_sizes set), but why do you want that? I'm not even
>> sure that all cases of .valid.max_access_size=4 are correct...
>
> Exactly for that :) Not have implicit default values, so correct
> values are reviewed when models are added.
But I wouldn't bet that those that we have in C are reviewed and
correct... They are incorrect if the hardware accepts 8-byte writes,
either discarding the top 4 bytes (then impl must both be 8) or writing
to both registers (then impl must be 4).
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 10/10] rust: bindings for MemoryRegionOps
2025-02-06 10:19 ` Paolo Bonzini
@ 2025-02-10 10:38 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-10 10:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Peter Maydell
On 6/2/25 11:19, Paolo Bonzini wrote:
> On 2/6/25 11:02, Philippe Mathieu-Daudé wrote:
>>>> Could we always make .valid_sizes() explicit?
>>>
>>> Yes (for example build() could even fail to compile if you don't have
>>> impl_sizes/valid_sizes set), but why do you want that? I'm not even
>>> sure that all cases of .valid.max_access_size=4 are correct...
>>
>> Exactly for that :) Not have implicit default values, so correct
>> values are reviewed when models are added.
>
> But I wouldn't bet that those that we have in C are reviewed and
> correct... They are incorrect if the hardware accepts 8-byte writes,
> either discarding the top 4 bytes (then impl must both be 8) or writing
> to both registers (then impl must be 4).
Are you saying in general or for the pl011 model?
What I'm asking is to have all rust models explicit the min/max sizes,
regardless of whether the C implementations are correct or not. For
rust models, we won't rely on default and will have to check the
valid range in the specs.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [RFC PATCH 00/10] rust: remaining part of qdev bindings
2025-01-17 19:39 [RFC PATCH 00/10] rust: remaining part of qdev bindings Paolo Bonzini
` (9 preceding siblings ...)
2025-01-17 19:40 ` [PATCH 10/10] rust: bindings for MemoryRegionOps Paolo Bonzini
@ 2025-01-24 2:46 ` Zhao Liu
10 siblings, 0 replies; 43+ messages in thread
From: Zhao Liu @ 2025-01-24 2:46 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 08:39:53PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:53 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [RFC PATCH 00/10] rust: remaining part of qdev bindings
> X-Mailer: git-send-email 2.47.1
>
> This is what I have left for qdev bindings, including 1) reference
> counting and object creation 2) clocks 3) Resettable 4) MemoryRegionOps.
> I have no rush for this series, and I expect HPET to be merged before it.
I'll get HPET v2 sorted out and sent quickly (in the this weekend), then
I'll go through this series. :-)
Regards,
Zhao
^ permalink raw reply [flat|nested] 43+ messages in thread