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

2025-02-10 Thread Zhao Liu
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 
> 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 
> ---
>  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 




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

2025-02-07 Thread Paolo Bonzini
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 
---
 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::();
-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 { Ownedfrom(&*irq) };
+let chr = unsafe { Ownedfrom(&*chr) };
 
-let sysbus = pl011.as_mut_ptr::();
-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 
 // 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) {
+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 SysBusDeviceMethods for R where R::Target: 
IsA {}
-- 
2.48.1