* [PATCH 01/12] rust: qom: add reference counting functionality
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-10 14:24 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 02/12] rust: qom: add object creation functionality Paolo Bonzini
` (10 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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 | 158 ++++++++++++++++++++++++++++++++++-
rust/qemu-api/src/vmstate.rs | 6 +-
rust/qemu-api/tests/tests.rs | 10 +++
3 files changed, 172 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index f50ee371aac..4a2e84c9aed 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -56,6 +56,7 @@
use std::{
ffi::CStr,
fmt,
+ mem::ManuallyDrop,
ops::{Deref, DerefMut},
os::raw::c_void,
ptr::NonNull,
@@ -63,7 +64,13 @@
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`).
@@ -610,6 +617,148 @@ 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`] or cloned, and decreases
+/// it when dropped. This ensures that the reference count remains elevated
+/// as long as any `Owned<T>` references to it exist.
+///
+/// `Owned<T>` can be used for two reasons:
+/// * because the lifetime of the QOM object is unknown and someone else could
+/// take a reference (similar to `Arc<T>`, for example): in this case, the
+/// object can escape and outlive the Rust struct that contains the `Owned<T>`
+/// field;
+///
+/// * to ensure that the object stays alive until after `Drop::drop` is called
+/// on the Rust struct: in this case, the object will always die together with
+/// the Rust struct that contains the `Owned<T>` field.
+///
+/// Child properties are an example of the second case: in C, an object that
+/// is created with `object_initialize_child` will die *before*
+/// `instance_finalize` is called, whereas Rust expects the struct to have valid
+/// contents when `Drop::drop` is called. Therefore Rust structs that have
+/// child properties need to keep a reference to the child object. Right now
+/// this can be done with `Owned<T>`; in the future one might have a separate
+/// `Child<'parent, T>` smart pointer that keeps a reference to a `T`, like
+/// `Owned`, but does not allow cloning.
+///
+/// Note that dropping an `Owned<T>` requires the big QEMU lock to be taken.
+#[repr(transparent)]
+#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct Owned<T: ObjectType>(NonNull<T>);
+
+// The following rationale for safety is taken from Linux's kernel::sync::Arc.
+
+// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying
+// `T` is `Sync` because it effectively means sharing `&T` (which is safe
+// because `T` is `Sync`); additionally, it needs `T` to be `Send` because any
+// thread that has an `Owned<T>` may ultimately access `T` using a
+// mutable reference when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: ObjectType + Send + Sync> Send for Owned<T> {}
+
+// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying
+// `T` is `Sync` because it effectively means sharing `&T` (which is safe
+// because `T` is `Sync`); additionally, it needs `T` to be `Send` because any
+// thread that has a `&Owned<T>` may clone it and get an `Owned<T>` on that
+// thread, so the thread may ultimately access `T` using a mutable reference
+// when the reference count reaches zero and `T` is dropped.
+unsafe impl<T: ObjectType + Sync + Send> 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()`.
+ 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> Clone for Owned<T> {
+ fn clone(&self) -> Self {
+ // 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>` and its clones.
+ unsafe { Owned::from(self.deref()) }
+ }
+}
+
+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>` and its clones.
+ // 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>` and its clones.
+ 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>`.
///
@@ -641,6 +790,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
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 5c3e75ed3d5..69ddac7f1c5 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -138,6 +138,16 @@ fn test_object_new() {
}
}
+#[test]
+#[allow(clippy::redundant_clone)]
+/// Create, clone and then drop an instance.
+fn test_clone() {
+ init_qom();
+ let p = DummyState::new();
+ assert_eq!(p.clone().typename(), "dummy");
+ drop(p);
+}
+
#[test]
/// Try invoking a method on an object.
fn test_typename() {
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 01/12] rust: qom: add reference counting functionality
2025-02-07 10:16 ` [PATCH 01/12] rust: qom: add reference counting functionality Paolo Bonzini
@ 2025-02-10 14:24 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-10 14:24 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:12AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:12 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 01/12] rust: qom: add reference counting functionality
> X-Mailer: git-send-email 2.48.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 | 158 ++++++++++++++++++++++++++++++++++-
> rust/qemu-api/src/vmstate.rs | 6 +-
> rust/qemu-api/tests/tests.rs | 10 +++
> 3 files changed, 172 insertions(+), 2 deletions(-)
>
> diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
> index f50ee371aac..4a2e84c9aed 100644
> --- a/rust/qemu-api/src/qom.rs
> +++ b/rust/qemu-api/src/qom.rs
> @@ -56,6 +56,7 @@
> use std::{
> ffi::CStr,
> fmt,
> + mem::ManuallyDrop,
> ops::{Deref, DerefMut},
> os::raw::c_void,
> ptr::NonNull,
> @@ -63,7 +64,13 @@
>
> 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`).
> @@ -610,6 +617,148 @@ 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`] or cloned, and decreases
> +/// it when dropped. This ensures that the reference count remains elevated
> +/// as long as any `Owned<T>` references to it exist.
> +///
> +/// `Owned<T>` can be used for two reasons:
> +/// * because the lifetime of the QOM object is unknown and someone else could
> +/// take a reference (similar to `Arc<T>`, for example): in this case, the
> +/// object can escape and outlive the Rust struct that contains the `Owned<T>`
> +/// field;
> +///
> +/// * to ensure that the object stays alive until after `Drop::drop` is called
> +/// on the Rust struct: in this case, the object will always die together with
> +/// the Rust struct that contains the `Owned<T>` field.
> +///
> +/// Child properties are an example of the second case: in C, an object that
> +/// is created with `object_initialize_child` will die *before*
> +/// `instance_finalize` is called, whereas Rust expects the struct to have valid
> +/// contents when `Drop::drop` is called. Therefore Rust structs that have
> +/// child properties need to keep a reference to the child object. Right now
> +/// this can be done with `Owned<T>`; in the future one might have a separate
> +/// `Child<'parent, T>` smart pointer that keeps a reference to a `T`, like
> +/// `Owned`, but does not allow cloning.
> +///
> +/// Note that dropping an `Owned<T>` requires the big QEMU lock to be taken.
Nice doc.
LGTM,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 02/12] rust: qom: add object creation functionality
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
2025-02-07 10:16 ` [PATCH 01/12] rust: qom: add reference counting functionality Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-10 14:30 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 03/12] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
` (9 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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 | 23 +++++++++++++----------
rust/qemu-api/src/prelude.rs | 1 +
rust/qemu-api/src/qom.rs | 23 +++++++++++++++++++++--
rust/qemu-api/tests/tests.rs | 30 +++++++++++-------------------
4 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 8050ede9c85..f5db114b0c7 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,11 +10,11 @@
use qemu_api::{
bindings::{
- error_fatal, hwaddr, memory_region_init_io, qdev_init_clock_in, qdev_new,
- qdev_prop_set_chr, qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
- qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq, sysbus_mmio_map,
- sysbus_realize_and_unref, CharBackend, Chardev, Clock, ClockEvent, MemoryRegion,
- QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
+ error_fatal, hwaddr, memory_region_init_io, qdev_init_clock_in, qdev_prop_set_chr,
+ qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
+ qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq, sysbus_mmio_map, sysbus_realize,
+ CharBackend, Chardev, Clock, ClockEvent, MemoryRegion, QEMUChrEvent,
+ CHR_IOCTL_SERIAL_SET_BREAK,
},
c_str, impl_vmstate_forward,
irq::InterruptSource,
@@ -705,15 +705,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 4a2e84c9aed..fad4759d7a6 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,
};
@@ -759,6 +759,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>`.
///
@@ -799,4 +817,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 69ddac7f1c5..9986925d71f 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,10 +131,8 @@ 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]
@@ -152,12 +149,8 @@ fn test_clone() {
/// 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
@@ -172,24 +165,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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 02/12] rust: qom: add object creation functionality
2025-02-07 10:16 ` [PATCH 02/12] rust: qom: add object creation functionality Paolo Bonzini
@ 2025-02-10 14:30 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-10 14:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:13AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:13 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/12] rust: qom: add object creation functionality
> X-Mailer: git-send-email 2.48.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 | 23 +++++++++++++----------
> rust/qemu-api/src/prelude.rs | 1 +
> rust/qemu-api/src/qom.rs | 23 +++++++++++++++++++++--
> rust/qemu-api/tests/tests.rs | 30 +++++++++++-------------------
> 4 files changed, 46 insertions(+), 31 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 03/12] rust: callbacks: allow passing optional callbacks as ()
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
2025-02-07 10:16 ` [PATCH 01/12] rust: qom: add reference counting functionality Paolo Bonzini
2025-02-07 10:16 ` [PATCH 02/12] rust: qom: add object creation functionality Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-10 14:31 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 04/12] rust: qdev: add clock creation Paolo Bonzini
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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..9642a16eb89 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 can be 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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 03/12] rust: callbacks: allow passing optional callbacks as ()
2025-02-07 10:16 ` [PATCH 03/12] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
@ 2025-02-10 14:31 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-10 14:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:14AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:14 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/12] rust: callbacks: allow passing optional callbacks as
> ()
> X-Mailer: git-send-email 2.48.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(+)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 04/12] rust: qdev: add clock creation
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (2 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 03/12] rust: callbacks: allow passing optional callbacks as () Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-10 14:40 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 05/12] rust: qom: allow initializing interface vtables Paolo Bonzini
` (7 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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 | 43 +++++-------
rust/qemu-api/src/prelude.rs | 2 +
rust/qemu-api/src/qdev.rs | 109 ++++++++++++++++++++++++++++++-
rust/qemu-api/src/vmstate.rs | 4 +-
4 files changed, 127 insertions(+), 31 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index f5db114b0c7..37936a328b8 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,17 +10,16 @@
use qemu_api::{
bindings::{
- error_fatal, hwaddr, memory_region_init_io, qdev_init_clock_in, qdev_prop_set_chr,
- qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
- qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq, sysbus_mmio_map, sysbus_realize,
- CharBackend, Chardev, Clock, ClockEvent, MemoryRegion, QEMUChrEvent,
- CHR_IOCTL_SERIAL_SET_BREAK,
+ error_fatal, hwaddr, memory_region_init_io, qdev_prop_set_chr, qemu_chr_fe_accept_input,
+ qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers, qemu_chr_fe_write_all, qemu_irq,
+ sysbus_connect_irq, sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, MemoryRegion,
+ QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
},
c_str, impl_vmstate_forward,
irq::InterruptSource,
prelude::*,
- qdev::{DeviceImpl, DeviceState, Property},
- qom::{ClassInitImpl, ObjectImpl, ParentField},
+ qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property},
+ qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
sysbus::{SysBusDevice, SysBusDeviceClass},
vmstate::VMStateDescription,
};
@@ -131,7 +130,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,
}
@@ -485,8 +484,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
@@ -506,22 +503,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
- // 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 is not initialized at this point; but since `Owned<_>` is
+ // not Drop, we can overwrite the undefined value without side effects;
+ // it's not sound but, because for all PL011State instances are created
+ // by QOM code which calls this function to initialize the fields, at
+ // leastno code is able to access an invalid self.clock value.
+ 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 f4c75c752f1..8f6744c5e26 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -4,14 +4,20 @@
//! Bindings to create devices and access device functionality from Rust.
-use std::{ffi::CStr, ptr::NonNull};
+use std::{
+ ffi::{CStr, CString},
+ os::raw::c_void,
+ ptr::NonNull,
+};
-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,
};
@@ -143,3 +149,100 @@ unsafe impl ObjectType for DeviceState {
unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
}
qom_isa!(DeviceState: Object);
+
+/// Trait for methods exposed by the [`DeviceState`] class. The methods can be
+/// called on all objects that have the trait `IsA<DeviceState>`.
+///
+/// The trait should only be used through the blanket implementation,
+/// which guarantees safety via `IsA`.
+pub trait DeviceMethods: ObjectDeref
+where
+ Self::Target: IsA<DeviceState>,
+{
+ /// Add an input clock named `name`. Invoke the callback with
+ /// `self` as the first parameter for the events that are requested.
+ ///
+ /// The resulting clock is added as a child of `self`, but it also
+ /// stays alive until after `Drop::drop` is called because C code
+ /// keeps an extra reference to it until `device_finalize()` calls
+ /// `qdev_finalize_clocklist()`. Therefore (unlike most cases in
+ /// which Rust code has a reference to a child object) it would be
+ /// possible for this function to return a `&Clock` too.
+ #[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, 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.
+ 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)
+ }
+
+ /// Add an output clock named `name`.
+ ///
+ /// The resulting clock is added as a child of `self`, but it also
+ /// stays alive until after `Drop::drop` is called because C code
+ /// keeps an extra reference to it until `device_finalize()` calls
+ /// `qdev_finalize_clocklist()`. Therefore (unlike most cases in
+ /// which Rust code has a reference to a child object) it would be
+ /// possible for this function to return a `&Clock` too.
+ #[inline]
+ fn init_clock_out(&self, name: &str) -> Owned<Clock> {
+ unsafe {
+ let cstr = CString::new(name).unwrap();
+ let clk = bindings::qdev_init_clock_out(self.as_mut_ptr(), cstr.as_ptr());
+
+ Owned::from(&*clk)
+ }
+ }
+}
+
+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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 04/12] rust: qdev: add clock creation
2025-02-07 10:16 ` [PATCH 04/12] rust: qdev: add clock creation Paolo Bonzini
@ 2025-02-10 14:40 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-10 14:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:15AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:15 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/12] rust: qdev: add clock creation
> X-Mailer: git-send-email 2.48.1
>
> 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 | 43 +++++-------
> rust/qemu-api/src/prelude.rs | 2 +
> rust/qemu-api/src/qdev.rs | 109 ++++++++++++++++++++++++++++++-
> rust/qemu-api/src/vmstate.rs | 4 +-
> 4 files changed, 127 insertions(+), 31 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 05/12] rust: qom: allow initializing interface vtables
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (3 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 04/12] rust: qdev: add clock creation Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-07 10:16 ` [PATCH 06/12] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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>.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
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 fad4759d7a6..fa69ae4c702 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,
};
@@ -263,6 +263,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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 06/12] rust: qdev: make ObjectImpl a supertrait of DeviceImpl
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (4 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 05/12] rust: qom: allow initializing interface vtables Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-07 10:16 ` [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
` (5 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
In practice it has to be implemented always in order to access an
implementation of ClassInitImpl<ObjectClass>. Make the relationship
explicit in the code.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/qdev.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 8f6744c5e26..cb6f12e726c 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -17,12 +17,12 @@
callbacks::FnCall,
cell::bql_locked,
prelude::*,
- qom::{ClassInitImpl, ObjectClass, Owned},
+ qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned},
vmstate::VMStateDescription,
};
/// 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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (5 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 06/12] rust: qdev: make ObjectImpl a supertrait of DeviceImpl Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-11 3:15 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 08/12] rust: bindings: add Send and Sync markers for types that have bindings Paolo Bonzini
` (4 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
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 | 111 ++++++++++++++++++++++++-------
rust/qemu-api/tests/tests.rs | 5 +-
4 files changed, 99 insertions(+), 28 deletions(-)
diff --git a/meson.build b/meson.build
index 131b2225ab6..32abefb7c48 100644
--- a/meson.build
+++ b/meson.build
@@ -4072,6 +4072,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 37936a328b8..1d0390b4fbe 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -18,7 +18,7 @@
c_str, impl_vmstate_forward,
irq::InterruptSource,
prelude::*,
- qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property},
+ qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
sysbus::{SysBusDevice, SysBusDeviceClass},
vmstate::VMStateDescription,
@@ -171,7 +171,10 @@ fn vmsd() -> Option<&'static VMStateDescription> {
Some(&device_class::VMSTATE_PL011)
}
const REALIZE: Option<fn(&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 {
@@ -622,7 +625,7 @@ pub fn realize(&self) {
}
}
- pub fn reset(&self) {
+ pub fn reset_hold(&self, _type: ResetType) {
self.regs.borrow_mut().reset();
}
@@ -737,3 +740,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 cb6f12e726c..2ec1ecc8489 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -10,10 +10,10 @@
ptr::NonNull,
};
-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::*,
@@ -21,8 +21,70 @@
vmstate::VMStateDescription,
};
+/// Trait providing the contents of the `ResettablePhases` struct,
+/// which is part of the QOM `Resettable` interface.
+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 an
+ /// [`InterruptSource`](crate::irq::InterruptSource), 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
+ /// `ResettablePhasesImpl::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,
+) {
+ let state = NonNull::new(obj).unwrap().cast::<T>();
+ T::ENTER.unwrap()(unsafe { state.as_ref() }, 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,
+) {
+ let state = NonNull::new(obj).unwrap().cast::<T>();
+ T::HOLD.unwrap()(unsafe { state.as_ref() }, 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,
+) {
+ let state = NonNull::new(obj).unwrap().cast::<T>();
+ T::EXIT.unwrap()(unsafe { state.as_ref() }, 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).
@@ -31,13 +93,6 @@ pub trait DeviceImpl: ObjectImpl {
/// with the function pointed to by `REALIZE`.
const REALIZE: Option<fn(&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,29 +120,36 @@ fn vmsd() -> Option<&'static VMStateDescription> {
T::REALIZE.unwrap()(unsafe { state.as_ref() });
}
-/// # 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) {
- let mut state = NonNull::new(dev).unwrap().cast::<T>();
- T::RESET.unwrap()(unsafe { state.as_mut() });
+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;
}
@@ -98,6 +160,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 9986925d71f..ccf915d64f3 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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable
2025-02-07 10:16 ` [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
@ 2025-02-11 3:15 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-11 3:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:18AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:18 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable
> X-Mailer: git-send-email 2.48.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 | 111 ++++++++++++++++++++++++-------
> rust/qemu-api/tests/tests.rs | 5 +-
> 4 files changed, 99 insertions(+), 28 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 08/12] rust: bindings: add Send and Sync markers for types that have bindings
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (6 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 07/12] rust: qdev: switch from legacy reset to Resettable Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-07 10:16 ` [PATCH 09/12] rust: bindings for MemoryRegionOps Paolo Bonzini
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
This is needed for the MemoryRegionOps<T> to be declared as static;
Rust requires static elements to be Sync.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/bindings.rs | 46 +++++++++++++++++++++++++++++++++++
rust/qemu-api/src/irq.rs | 3 +++
2 files changed, 49 insertions(+)
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 8a9b821bb91..b71220113ef 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -21,9 +21,55 @@
#[cfg(not(MESON))]
include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));
+// SAFETY: these are implemented in C; the bindings need to assert that the
+// BQL is taken, either directly or via `BqlCell` and `BqlRefCell`.
+unsafe impl Send for BusState {}
+unsafe impl Sync for BusState {}
+
+unsafe impl Send for CharBackend {}
+unsafe impl Sync for CharBackend {}
+
+unsafe impl Send for Chardev {}
+unsafe impl Sync for Chardev {}
+
+unsafe impl Send for Clock {}
+unsafe impl Sync for Clock {}
+
+unsafe impl Send for DeviceState {}
+unsafe impl Sync for DeviceState {}
+
+unsafe impl Send for MemoryRegion {}
+unsafe impl Sync for MemoryRegion {}
+
+unsafe impl Send for ObjectClass {}
+unsafe impl Sync for ObjectClass {}
+
+unsafe impl Send for Object {}
+unsafe impl Sync for Object {}
+
+unsafe impl Send for SysBusDevice {}
+unsafe impl Sync for SysBusDevice {}
+
+// SAFETY: this is a pure data struct
+unsafe impl Send for CoalescedMemoryRange {}
+unsafe impl Sync for CoalescedMemoryRange {}
+
+// SAFETY: these are constants and vtables; the Send and Sync requirements
+// are deferred to the unsafe callbacks that they contain
+unsafe impl Send for MemoryRegionOps {}
+unsafe impl Sync for MemoryRegionOps {}
+
unsafe impl Send for Property {}
unsafe impl Sync for Property {}
+
+unsafe impl Send for TypeInfo {}
unsafe impl Sync for TypeInfo {}
+
+unsafe impl Send for VMStateDescription {}
unsafe impl Sync for VMStateDescription {}
+
+unsafe impl Send for VMStateField {}
unsafe impl Sync for VMStateField {}
+
+unsafe impl Send for VMStateInfo {}
unsafe impl Sync for VMStateInfo {}
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.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 09/12] rust: bindings for MemoryRegionOps
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (7 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 08/12] rust: bindings: add Send and Sync markers for types that have bindings Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-11 3:26 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 10/12] rust: irq: define ObjectType for IRQState Paolo Bonzini
` (2 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 51 +++----
rust/hw/char/pl011/src/lib.rs | 1 -
rust/hw/char/pl011/src/memory_ops.rs | 34 -----
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 | 1 +
8 files changed, 226 insertions(+), 61 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 1d0390b4fbe..5e4e75133c8 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,13 +10,14 @@
use qemu_api::{
bindings::{
- error_fatal, hwaddr, memory_region_init_io, qdev_prop_set_chr, qemu_chr_fe_accept_input,
- qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers, qemu_chr_fe_write_all, qemu_irq,
- sysbus_connect_irq, sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, MemoryRegion,
- QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
+ error_fatal, qdev_prop_set_chr, qemu_chr_fe_accept_input, qemu_chr_fe_ioctl,
+ qemu_chr_fe_set_handlers, qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq,
+ sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, QEMUChrEvent,
+ CHR_IOCTL_SERIAL_SET_BREAK,
},
c_str, impl_vmstate_forward,
irq::InterruptSource,
+ memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
prelude::*,
qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
qom::{ClassInitImpl, ObjectImpl, Owned, ParentField},
@@ -26,7 +27,6 @@
use crate::{
device_class,
- memory_ops::PL011_OPS,
registers::{self, Interrupt},
RegisterOffset,
};
@@ -487,20 +487,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();
@@ -525,7 +529,7 @@ fn post_init(&self) {
}
}
- pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
+ pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
let device_id = self.get_class().device_id;
@@ -540,7 +544,7 @@ 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()
@@ -548,7 +552,7 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
}
}
- 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) {
// qemu_chr_fe_write_all() calls into the can_receive
@@ -561,14 +565,15 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
// XXX this blocks entire thread. Rewrite to use
// qemu_chr_fe_write and background I/O callbacks
unsafe {
- qemu_chr_fe_write_all(&mut self.char_backend, &ch, 1);
+ qemu_chr_fe_write_all(addr_of!(self.char_backend) as *mut _, &ch, 1);
}
}
- 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 e704daf6e3e..88452dc888c 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -27,7 +27,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 432d3263898..00000000000
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ /dev/null
@@ -1,34 +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 {
- let mut state = NonNull::new(opaque).unwrap().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) {
- let mut state = NonNull::new(opaque).unwrap().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 7b04947cb6c..75742b50d4e 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -100,3 +100,4 @@ fn default() -> Self {
impl_zeroable!(crate::bindings::VMStateDescription);
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
+impl_zeroable!(crate::bindings::MemoryRegionOps);
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 09/12] rust: bindings for MemoryRegionOps
2025-02-07 10:16 ` [PATCH 09/12] rust: bindings for MemoryRegionOps Paolo Bonzini
@ 2025-02-11 3:26 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-11 3:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:20AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:20 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 09/12] rust: bindings for MemoryRegionOps
> X-Mailer: git-send-email 2.48.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 51 +++----
> rust/hw/char/pl011/src/lib.rs | 1 -
> rust/hw/char/pl011/src/memory_ops.rs | 34 -----
> 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 | 1 +
> 8 files changed, 226 insertions(+), 61 deletions(-)
> delete mode 100644 rust/hw/char/pl011/src/memory_ops.rs
> create mode 100644 rust/qemu-api/src/memory.rs
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 10/12] rust: irq: define ObjectType for IRQState
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (8 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 09/12] rust: bindings for MemoryRegionOps Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-11 3:31 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 11/12] rust: chardev, qdev: add bindings to qdev_prop_set_chr Paolo Bonzini
2025-02-07 10:16 ` [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust Paolo Bonzini
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
This is a small preparation in order to use an Owned<IRQState> for the argument
to sysbus_connect_irq.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/irq.rs | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 638545c3a64..835b027d5e5 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -5,11 +5,12 @@
//! Bindings for interrupt sources
use core::ptr;
-use std::{marker::PhantomData, os::raw::c_int};
+use std::{ffi::CStr, marker::PhantomData, os::raw::c_int};
use crate::{
- bindings::{qemu_set_irq, IRQState},
+ bindings::{self, qemu_set_irq},
prelude::*,
+ qom::ObjectClass,
};
/// Interrupt sources are used by devices to pass changes to a value (typically
@@ -21,7 +22,8 @@
/// method sends a `true` value to the sink. If the guest has to see a
/// different polarity, that change is performed by the board between the
/// device and the interrupt controller.
-///
+pub type IRQState = bindings::IRQState;
+
/// Interrupts are implemented as a pointer to the interrupt "sink", which has
/// type [`IRQState`]. A device exposes its source as a QOM link property using
/// a function such as [`SysBusDeviceMethods::init_irq`], and
@@ -91,3 +93,10 @@ fn default() -> Self {
}
}
}
+
+unsafe impl ObjectType for IRQState {
+ type Class = ObjectClass;
+ const TYPE_NAME: &'static CStr =
+ unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_IRQ) };
+}
+qom_isa!(IRQState: Object);
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 11/12] rust: chardev, qdev: add bindings to qdev_prop_set_chr
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (9 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 10/12] rust: irq: define ObjectType for IRQState Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-11 3:34 ` Zhao Liu
2025-02-07 10:16 ` [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust Paolo Bonzini
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Because the argument to the function is an Owned<Chardev>, this also
adds an ObjectType implementation to Chardev.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 1 +
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/chardev.rs | 19 +++++++++++++++++++
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/qdev.rs | 9 +++++++++
5 files changed, 31 insertions(+)
create mode 100644 rust/qemu-api/src/chardev.rs
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 5e4e75133c8..22f3ca3b4e8 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -15,6 +15,7 @@
sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, QEMUChrEvent,
CHR_IOCTL_SERIAL_SET_BREAK,
},
+ chardev::Chardev,
c_str, impl_vmstate_forward,
irq::InterruptSource,
memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 80eafc7f6bd..45e30324b29 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -20,6 +20,7 @@ _qemu_api_rs = static_library(
'src/bitops.rs',
'src/callbacks.rs',
'src/cell.rs',
+ 'src/chardev.rs',
'src/c_str.rs',
'src/irq.rs',
'src/memory.rs',
diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs
new file mode 100644
index 00000000000..74cfb634e5f
--- /dev/null
+++ b/rust/qemu-api/src/chardev.rs
@@ -0,0 +1,19 @@
+// Copyright 2024 Red Hat, Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Bindings for character devices
+
+use std::ffi::CStr;
+
+use crate::{bindings, prelude::*};
+
+pub type Chardev = bindings::Chardev;
+pub type ChardevClass = bindings::ChardevClass;
+
+unsafe impl ObjectType for Chardev {
+ type Class = ChardevClass;
+ const TYPE_NAME: &'static CStr =
+ unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_CHARDEV) };
+}
+qom_isa!(Chardev: Object);
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 8cc095b13f6..1d7112445e2 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -17,6 +17,7 @@
pub mod c_str;
pub mod callbacks;
pub mod cell;
+pub mod chardev;
pub mod irq;
pub mod memory;
pub mod module;
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 2ec1ecc8489..0041c66ed0c 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -16,6 +16,7 @@
bindings::{self, Error, ResettableClass},
callbacks::FnCall,
cell::bql_locked,
+ chardev::Chardev,
prelude::*,
qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned},
vmstate::VMStateDescription,
@@ -299,6 +300,14 @@ fn init_clock_out(&self, name: &str) -> Owned<Clock> {
Owned::from(&*clk)
}
}
+
+ fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
+ assert!(bql_locked());
+ let c_propname = CString::new(propname).unwrap();
+ unsafe {
+ bindings::qdev_prop_set_chr(self.as_mut_ptr(), c_propname.as_ptr(), chr.as_mut_ptr());
+ }
+ }
}
impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 11/12] rust: chardev, qdev: add bindings to qdev_prop_set_chr
2025-02-07 10:16 ` [PATCH 11/12] rust: chardev, qdev: add bindings to qdev_prop_set_chr Paolo Bonzini
@ 2025-02-11 3:34 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-11 3:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:22AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:22 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 11/12] rust: chardev, qdev: add bindings to
> qdev_prop_set_chr
> X-Mailer: git-send-email 2.48.1
>
> Because the argument to the function is an Owned<Chardev>, this also
> adds an ObjectType implementation to Chardev.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 1 +
> rust/qemu-api/meson.build | 1 +
> rust/qemu-api/src/chardev.rs | 19 +++++++++++++++++++
> rust/qemu-api/src/lib.rs | 1 +
> rust/qemu-api/src/qdev.rs | 9 +++++++++
> 5 files changed, 31 insertions(+)
> create mode 100644 rust/qemu-api/src/chardev.rs
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust
2025-02-07 10:16 [PATCH 00/12] rust: remaining parts of qdev bindings Paolo Bonzini
` (10 preceding siblings ...)
2025-02-07 10:16 ` [PATCH 11/12] rust: chardev, qdev: add bindings to qdev_prop_set_chr Paolo Bonzini
@ 2025-02-07 10:16 ` Paolo Bonzini
2025-02-11 3:37 ` Zhao Liu
11 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2025-02-07 10:16 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, zhao1.liu
Not a major change but, as a small but significant step in creating
qdev bindings, show how pl011_create can be written without "unsafe"
calls (apart from converting pointers to references).
This also provides a starting point for creating Error** bindings.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++----------------
rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++---
2 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 22f3ca3b4e8..7e936b50eb0 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -10,14 +10,12 @@
use qemu_api::{
bindings::{
- error_fatal, qdev_prop_set_chr, qemu_chr_fe_accept_input, qemu_chr_fe_ioctl,
- qemu_chr_fe_set_handlers, qemu_chr_fe_write_all, qemu_irq, sysbus_connect_irq,
- sysbus_mmio_map, sysbus_realize, CharBackend, Chardev, QEMUChrEvent,
- CHR_IOCTL_SERIAL_SET_BREAK,
+ qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
+ qemu_chr_fe_write_all, CharBackend, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
},
chardev::Chardev,
- c_str, impl_vmstate_forward,
- irq::InterruptSource,
+ impl_vmstate_forward,
+ irq::{IRQState, InterruptSource},
memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
prelude::*,
qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
@@ -698,26 +696,27 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
/// # Safety
///
-/// We expect the FFI user of this function to pass a valid pointer for `chr`.
+/// We expect the FFI user of this function to pass a valid pointer for `chr`
+/// and `irq`.
#[no_mangle]
pub unsafe extern "C" fn pl011_create(
addr: u64,
- irq: qemu_irq,
+ irq: *mut IRQState,
chr: *mut Chardev,
) -> *mut DeviceState {
- let pl011 = PL011State::new();
- unsafe {
- let dev = pl011.as_mut_ptr::<DeviceState>();
- qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr);
+ // SAFETY: The callers promise that they have owned references.
+ // They do not gift them to pl011_create, so use `Owned::from`.
+ let irq = unsafe { Owned::<IRQState>::from(&*irq) };
+ let chr = unsafe { Owned::<Chardev>::from(&*chr) };
- 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);
-
- // return the pointer, which is kept alive by the QOM tree; drop owned ref
- pl011.as_mut_ptr()
- }
+ let dev = PL011State::new();
+ dev.prop_set_chr("chardev", &chr);
+ dev.realize();
+ dev.mmio_map(0, addr);
+ dev.connect_irq(0, &irq);
+ // SAFETY: return the pointer, which has to be mutable and is kept alive
+ // by the QOM tree; drop owned ref
+ unsafe { dev.as_mut_ptr() }
}
#[repr(C)]
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index c27dbf79e43..1f071706ce8 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -2,18 +2,18 @@
// Author(s): Paolo Bonzini <pbonzini@redhat.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::ffi::CStr;
+use std::{ffi::CStr, ptr::addr_of_mut};
pub use bindings::{SysBusDevice, SysBusDeviceClass};
use crate::{
bindings,
cell::bql_locked,
- irq::InterruptSource,
+ irq::{IRQState, InterruptSource},
memory::MemoryRegion,
prelude::*,
qdev::{DeviceClass, DeviceState},
- qom::ClassInitImpl,
+ qom::{ClassInitImpl, Owned},
};
unsafe impl ObjectType for SysBusDevice {
@@ -60,6 +60,34 @@ fn init_irq(&self, irq: &InterruptSource) {
bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr());
}
}
+
+ // TODO: do we want a type like GuestAddress here?
+ fn mmio_map(&self, id: u32, addr: u64) {
+ assert!(bql_locked());
+ let id: i32 = id.try_into().unwrap();
+ unsafe {
+ bindings::sysbus_mmio_map(self.as_mut_ptr(), id, addr);
+ }
+ }
+
+ // Owned<> is used here because sysbus_connect_irq (via
+ // object_property_set_link) adds a reference to the IRQState,
+ // which can prolong its life
+ fn connect_irq(&self, id: u32, irq: &Owned<IRQState>) {
+ assert!(bql_locked());
+ let id: i32 = id.try_into().unwrap();
+ unsafe {
+ bindings::sysbus_connect_irq(self.as_mut_ptr(), id, irq.as_mut_ptr());
+ }
+ }
+
+ fn realize(&self) {
+ // TODO: return an Error
+ assert!(bql_locked());
+ unsafe {
+ bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal));
+ }
+ }
}
impl<R: ObjectDeref> SysBusDeviceMethods for R where R::Target: IsA<SysBusDevice> {}
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust
2025-02-07 10:16 ` [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust Paolo Bonzini
@ 2025-02-11 3:37 ` Zhao Liu
0 siblings, 0 replies; 22+ messages in thread
From: Zhao Liu @ 2025-02-11 3:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 11:16:23AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 11:16:23 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 12/12] rust: pl011: convert pl011_create to safe Rust
> X-Mailer: git-send-email 2.48.1
>
> Not a major change but, as a small but significant step in creating
> qdev bindings, show how pl011_create can be written without "unsafe"
> calls (apart from converting pointers to references).
>
> This also provides a starting point for creating Error** bindings.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 39 ++++++++++++++++----------------
> rust/qemu-api/src/sysbus.rs | 34 +++++++++++++++++++++++++---
> 2 files changed, 50 insertions(+), 23 deletions(-)
>
Owned<> uses are proper, so still,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 22+ messages in thread