The basic object lifecycle test can now be implemented using safe code!

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 13 ++++++++-----
 rust/qemu-api/src/prelude.rs     |  1 +
 rust/qemu-api/src/qom.rs         | 23 +++++++++++++++++++++--
 rust/qemu-api/tests/tests.rs     | 30 +++++++++++-------------------
 4 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 27563700665..d8409f3d310 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -690,15 +690,18 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
     irq: qemu_irq,
     chr: *mut Chardev,
 ) -> *mut DeviceState {
+    let pl011 = PL011State::new();
     unsafe {
-        let dev: *mut DeviceState = qdev_new(PL011State::TYPE_NAME.as_ptr());
-        let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
-
+        let dev = pl011.as_mut_ptr::<DeviceState>();
         qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr);
-        sysbus_realize_and_unref(sysbus, addr_of_mut!(error_fatal));
+
+        let sysbus = pl011.as_mut_ptr::<SysBusDevice>();
+        sysbus_realize(sysbus, addr_of_mut!(error_fatal));
         sysbus_mmio_map(sysbus, 0, addr);
         sysbus_connect_irq(sysbus, 0, irq);
-        dev
+
+        // return the pointer, which is kept alive by the QOM tree; drop owned ref
+        pl011.as_mut_ptr()
     }
 }
 
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 2dc86e19b29..3df6a5c21ec 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -12,6 +12,7 @@
 pub use crate::qom::ObjectCast;
 pub use crate::qom::ObjectCastMut;
 pub use crate::qom::ObjectDeref;
+pub use crate::qom::ObjectClassMethods;
 pub use crate::qom::ObjectMethods;
 pub use crate::qom::ObjectType;
 
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index c404f8a1aa7..6f7db3eabcb 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -66,8 +66,8 @@
 
 use crate::{
     bindings::{
-        self, object_dynamic_cast, object_get_class, object_get_typename, object_ref, object_unref,
-        TypeInfo,
+        self, object_dynamic_cast, object_get_class, object_get_typename, object_new, object_ref,
+        object_unref, TypeInfo,
     },
     cell::bql_locked,
 };
@@ -717,6 +717,24 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
     }
 }
 
+/// Trait for class methods exposed by the Object class.  The methods can be
+/// called on all objects that have the trait `IsA<Object>`.
+///
+/// The trait should only be used through the blanket implementation,
+/// which guarantees safety via `IsA`
+pub trait ObjectClassMethods: IsA<Object> {
+    /// Return a new reference counted instance of this class
+    fn new() -> Owned<Self> {
+        assert!(bql_locked());
+        // SAFETY: the object created by object_new is allocated on
+        // the heap and has a reference count of 1
+        unsafe {
+            let obj = &*object_new(Self::TYPE_NAME.as_ptr());
+            Owned::from_raw(obj.unsafe_cast::<Self>())
+        }
+    }
+}
+
 /// Trait for methods exposed by the Object class.  The methods can be
 /// called on all objects that have the trait `IsA<Object>`.
 ///
@@ -757,4 +775,5 @@ fn debug_fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
     }
 }
 
+impl<T> ObjectClassMethods for T where T: IsA<Object> {}
 impl<R: ObjectDeref> ObjectMethods for R where R::Target: IsA<Object> {}
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 5c3e75ed3d5..1944e65c8f3 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -3,8 +3,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 use std::{
-    ffi::CStr,
-    os::raw::c_void,
+    ffi::{c_void, CStr},
     ptr::{addr_of, addr_of_mut},
 };
 
@@ -132,22 +131,16 @@ fn init_qom() {
 /// Create and immediately drop an instance.
 fn test_object_new() {
     init_qom();
-    unsafe {
-        object_unref(object_new(DummyState::TYPE_NAME.as_ptr()).cast());
-        object_unref(object_new(DummyChildState::TYPE_NAME.as_ptr()).cast());
-    }
+    drop(DummyState::new());
+    drop(DummyChildState::new());
 }
 
 #[test]
 /// Try invoking a method on an object.
 fn test_typename() {
     init_qom();
-    let p: *mut DummyState = unsafe { object_new(DummyState::TYPE_NAME.as_ptr()).cast() };
-    let p_ref: &DummyState = unsafe { &*p };
-    assert_eq!(p_ref.typename(), "dummy");
-    unsafe {
-        object_unref(p_ref.as_object_mut_ptr().cast::<c_void>());
-    }
+    let p = DummyState::new();
+    assert_eq!(p.typename(), "dummy");
 }
 
 // a note on all "cast" tests: usually, especially for downcasts the desired
@@ -162,24 +155,23 @@ fn test_typename() {
 /// Test casts on shared references.
 fn test_cast() {
     init_qom();
-    let p: *mut DummyState = unsafe { object_new(DummyState::TYPE_NAME.as_ptr()).cast() };
+    let p = DummyState::new();
+    let p_ptr: *mut DummyState = unsafe { p.as_mut_ptr() };
+    let p_ref: &mut DummyState = unsafe { &mut *p_ptr };
 
-    let p_ref: &DummyState = unsafe { &*p };
     let obj_ref: &Object = p_ref.upcast();
-    assert_eq!(addr_of!(*obj_ref), p.cast());
+    assert_eq!(addr_of!(*obj_ref), p_ptr.cast());
 
     let sbd_ref: Option<&SysBusDevice> = obj_ref.dynamic_cast();
     assert!(sbd_ref.is_none());
 
     let dev_ref: Option<&DeviceState> = obj_ref.downcast();
-    assert_eq!(addr_of!(*dev_ref.unwrap()), p.cast());
+    assert_eq!(addr_of!(*dev_ref.unwrap()), p_ptr.cast());
 
     // SAFETY: the cast is wrong, but the value is only used for comparison
     unsafe {
         let sbd_ref: &SysBusDevice = obj_ref.unsafe_cast();
-        assert_eq!(addr_of!(*sbd_ref), p.cast());
-
-        object_unref(p_ref.as_object_mut_ptr().cast::<c_void>());
+        assert_eq!(addr_of!(*sbd_ref), p_ptr.cast());
     }
 }
 
-- 
2.47.1

On Fri, Jan 17, 2025 at 08:39:56PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:56 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/10] rust: qom: add object creation functionality
> X-Mailer: git-send-email 2.47.1
> 
> The basic object lifecycle test can now be implemented using safe code!
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 13 ++++++++-----
>  rust/qemu-api/src/prelude.rs     |  1 +
>  rust/qemu-api/src/qom.rs         | 23 +++++++++++++++++++++--
>  rust/qemu-api/tests/tests.rs     | 30 +++++++++++-------------------
>  4 files changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 27563700665..d8409f3d310 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -690,15 +690,18 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
>      irq: qemu_irq,
>      chr: *mut Chardev,
>  ) -> *mut DeviceState {
> +    let pl011 = PL011State::new();
>      unsafe {
> -        let dev: *mut DeviceState = qdev_new(PL011State::TYPE_NAME.as_ptr());
> -        let sysbus: *mut SysBusDevice = dev.cast::<SysBusDevice>();
> -
> +        let dev = pl011.as_mut_ptr::<DeviceState>();
>          qdev_prop_set_chr(dev, c_str!("chardev").as_ptr(), chr);
> -        sysbus_realize_and_unref(sysbus, addr_of_mut!(error_fatal));
> +
> +        let sysbus = pl011.as_mut_ptr::<SysBusDevice>();
> +        sysbus_realize(sysbus, addr_of_mut!(error_fatal));
>          sysbus_mmio_map(sysbus, 0, addr);
>          sysbus_connect_irq(sysbus, 0, irq);
> -        dev
> +
> +        // return the pointer, which is kept alive by the QOM tree; drop owned ref
> +        pl011.as_mut_ptr()

The ref count of Owned<> is decreased on exit, so we need to use
sysbus_realize() instead of sysbus_realize_and_unref() to ensure ref
count is correct at C side.

Initially, I hesitated here for an entire morning because this didn't
seem to conform to the usual usage of sysbus_realize_and_unref() (or,
further, qdev_realize_and_unref()).

But now I realize that qdev_realize() is used for embedded objects,
while qdev_realize_and_unref() is used for non-embedded cases. Therefore,
moving forward, perhaps qdev_realize_and_unref() should not exist on the
Rust side.

Owned<> will automatically drop and thus automatically unref, while
Child<> will not unref. Based on this consideration, I am now convincing
myself that this change (using sysbus_realize()) is reasonable. :-)

In the future, maybe we need some wrappers on qdev_realize()/sysbus_realize().

>      }
>  }

Overall, still fine for me,

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

On Thu, Feb 6, 2025 at 8:29 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> The ref count of Owned<> is decreased on exit, so we need to use
> sysbus_realize() instead of sysbus_realize_and_unref() to ensure ref
> count is correct at C side.
>
> Initially, I hesitated here for an entire morning because this didn't
> seem to conform to the usual usage of sysbus_realize_and_unref() (or,
> further, qdev_realize_and_unref()).
>
> But now I realize that qdev_realize() is used for embedded objects,
> while qdev_realize_and_unref() is used for non-embedded cases. Therefore,
> moving forward, perhaps qdev_realize_and_unref() should not exist on the
> Rust side.

Correct, all unref() operations are handled implicitly by the Drop
implementation of Owned<>

> Owned<> will automatically drop and thus automatically unref, while
> Child<> will not unref.

No, Child<> will also unref. The point of Child<> is to prolong the
life of the child object, so that it dies after instance_finalize
instead() of dying during object_property_del_all(). In C that has to
be done manually (as is the case in the clock creation functions), in
Rust it will be enforced by the type system.

> In the future, maybe we need some wrappers on qdev_realize()/sysbus_realize().

Yep, I'll add a conversion of pl011_create to safe Rust to clarify the
rules around usage of Owned<>.

Paolo