rust: pl011: convert pl011_create to safe Rust

[PATCH] rust: pl011: convert pl011_create to safe Rust

Posted by Paolo Bonzini 1 month ago

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

Re: [PATCH] rust: pl011: convert pl011_create to safe Rust

Posted by Zhao Liu 1 month ago

On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote:
> Date: Thu,  6 Feb 2025 12:15:14 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] 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(-)

...

> +    fn realize(&self) {

What about renaming this as "realize_with_sysbus"?

Elsewhere, the device's own realize method is often used to set
DeviceImpl::REALIZE.

And this realize here is meant to call the realize() method defined on
the C side, so to avoid confusion we can avoid the same name? It's up to
you.

> +        // TODO: return an Error
> +        assert!(bql_locked());
> +        unsafe {
> +            bindings::sysbus_realize(self.as_mut_ptr(), addr_of_mut!(bindings::error_fatal));
> +        }
> +    }

This is a nice patch that shows more about how to use Owned<>!

(BTW, I guess this patch is not the stable material. :-) )

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

vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

Posted by Paolo Bonzini 1 month ago

On 2/10/25 10:59, Zhao Liu wrote:
> On Thu, Feb 06, 2025 at 12:15:14PM +0100, Paolo Bonzini wrote:
>> Date: Thu,  6 Feb 2025 12:15:14 +0100
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Subject: [PATCH] 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(-)
> 
> ...
> 
>> +    fn realize(&self) {
> 
> What about renaming this as "realize_with_sysbus"?
> 
> Elsewhere, the device's own realize method is often used to set
> DeviceImpl::REALIZE.

I *think* this is not a problem in practice because trait methods are 
public (if the trait is in scope) whereas the device's realize method if 
private.

I agree that the naming conflict is unfortunate though, if only because 
it can cause confusion.  I don't know if this can be solved by 
procedural macros; for example a #[vtable] attribute that changes

     #[qemu_api_macros::vtable]
     fn realize(...) {
     }

into

     const fn REALIZE: ... = Some({
         fn realize(...) {
         }
         realize
     }

This way, the REALIZE item would be included in the "impl DeviceImpl for 
PL011State" block instead of "impl PL011State".  It's always a fine line 
between procedural macros cleaning vs. messing things up, which is why 
until now I wanted to see what things look like with pure Rust code; but 
I guess now it's the time to evaluate this kind of thing.

Adding Junjie since he contributed the initial proc macro infrastructure 
and may have opinions on this.

Paolo