From 0fcccf3ff04a54d597bffcb7a42668c52a7dcec0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 17 Jan 2025 12:00:01 +0100 Subject: rust: qom: add reference counting functionality 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. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'rust/qemu-api/src/vmstate.rs') diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 6ac432cf52..11d21b8791 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -29,6 +29,8 @@ use core::{marker::PhantomData, mem, ptr::NonNull}; 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(_: PhantomData) -> 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 @@ impl_vmstate_pointer!(NonNull where T: VMState); // Unlike C pointers, Box is always non-null therefore there is no need // to specify VMS_ALLOC. impl_vmstate_pointer!(Box where T: VMState); +impl_vmstate_pointer!(Owned where T: VMState + ObjectType); // Arrays using the underlying type's VMState plus // VMS_ARRAY/VMS_ARRAY_OF_POINTER -- cgit 1.4.1 From 201ef001dd40fdb11c83f3e47604219c374590ec Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 17 Jan 2025 11:21:26 +0100 Subject: rust: qdev: add clock creation 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. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- rust/hw/char/pl011/src/device.rs | 43 +++++++--------- rust/qemu-api/src/prelude.rs | 2 + rust/qemu-api/src/qdev.rs | 107 +++++++++++++++++++++++++++++++++++++-- rust/qemu-api/src/vmstate.rs | 4 +- 4 files changed, 125 insertions(+), 31 deletions(-) (limited to 'rust/qemu-api/src/vmstate.rs') diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs index f5db114b0c..37936a328b 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -10,17 +10,16 @@ use std::{ 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, + pub clock: Owned, #[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 @@ impl PL011State { // 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::(), - 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 3df6a5c21e..87e3ce90f2 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -7,6 +7,8 @@ pub use crate::bitops::IntegerExt; 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 f4c75c752f..176c69a560 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,98 @@ 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`. +/// +/// The trait should only be used through the blanket implementation, +/// which guarantees safety via `IsA`. +pub trait DeviceMethods: ObjectDeref +where + Self::Target: IsA, +{ + /// 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 FnCall<(&'a Self::Target, ClockEvent)>>( + &self, + name: &str, + _cb: &F, + events: ClockEvent, + ) -> Owned { + fn do_init_clock_in( + dev: *mut DeviceState, + name: &str, + cb: Option, + events: ClockEvent, + ) -> Owned { + 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::(), + events.0, + ); + + Owned::from(&*clk) + } + } + + let cb: Option = if F::is_some() { + unsafe extern "C" fn rust_clock_cb 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::()) }, event)) + } + Some(rust_clock_cb::) + } else { + None + }; + + do_init_clock_in(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 { + 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 DeviceMethods for R where R::Target: IsA {} + +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 11d21b8791..164effc655 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 -- cgit 1.4.1 From ee7d3aec54a32ce53c9b5ca86c75c945a877db19 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 30 Jan 2025 10:55:16 +0100 Subject: rust: vmstate: remove redundant link targets Signed-off-by: Paolo Bonzini --- rust/qemu-api/src/vmstate.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'rust/qemu-api/src/vmstate.rs') diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index 164effc655..c6dfb60935 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -191,10 +191,9 @@ pub const fn vmstate_varray_flag(_: PhantomData) -> VMStateFlags /// * scalar types (integer and `bool`) /// * the C struct `QEMUTimer` /// * a transparent wrapper for any of the above (`Cell`, `UnsafeCell`, -/// [`BqlCell`](crate::cell::BqlCell), [`BqlRefCell`](crate::cell::BqlRefCell) +/// [`BqlCell`], [`BqlRefCell`] /// * a raw pointer to any of the above -/// * a `NonNull` pointer, a `Box` or an [`Owned`](crate::qom::Owned) for any of -/// the above +/// * a `NonNull` pointer, a `Box` or an [`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 -- cgit 1.4.1 From 16534af51bc0e9c3db94097ab37ebd3ed50e1c0f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 11 Feb 2025 13:55:53 +0100 Subject: rust: fix doctests Doctests were not being run by CI, and have broken. Fix them. Signed-off-by: Paolo Bonzini --- .gitlab-ci.d/buildtest.yml | 6 ++++++ rust/qemu-api/src/vmstate.rs | 2 +- rust/qemu-api/src/zeroable.rs | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) (limited to 'rust/qemu-api/src/vmstate.rs') diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 4265a57783..00f4bfcd9f 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -131,6 +131,12 @@ build-system-fedora-rust-nightly: CONFIGURE_ARGS: --disable-docs --enable-rust --enable-strict-rust-lints TARGETS: aarch64-softmmu MAKE_CHECK_ARGS: check-build + after_script: + - source scripts/ci/gitlab-ci-section + - section_start test "Running Rust doctests" + - cd build + - pyvenv/bin/meson devenv -w ../rust ${CARGO-cargo} test --doc -p qemu_api + allow_failure: true check-system-fedora: diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs index c6dfb60935..24a4dc81e7 100644 --- a/rust/qemu-api/src/vmstate.rs +++ b/rust/qemu-api/src/vmstate.rs @@ -294,7 +294,7 @@ impl VMStateField { /// # Examples /// /// ``` -/// # use qemu_api::vmstate::impl_vmstate_forward; +/// # use qemu_api::impl_vmstate_forward; /// pub struct Fifo([u8; 16]); /// impl_vmstate_forward!(Fifo); /// ``` diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs index a2356cb2f2..47b6977828 100644 --- a/rust/qemu-api/src/zeroable.rs +++ b/rust/qemu-api/src/zeroable.rs @@ -7,7 +7,7 @@ /// behavior. This trait in principle could be implemented as just: /// /// ``` -/// pub unsafe trait Zeroable { +/// pub unsafe trait Zeroable: Default { /// const ZERO: Self = unsafe { ::core::mem::MaybeUninit::::zeroed().assume_init() }; /// } /// ``` -- cgit 1.4.1