Re: [PATCH 03/10] rust: qom: add object creation functionality

[Zhao Liu]

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>