[PATCH RESEND 0/2] rust/pl011: miscellaneous cleanups

[PATCH RESEND 0/2] rust/pl011: miscellaneous cleanups

[Zhao Liu]

(Resend the series since it was missed on https://lore.kernel.org/qemu-devel/.)

Hi,

This series includes the following cleanups:

 * Patch 1: Make realize() safe to only accept immutable self
            reference, which is in prepare to introduce gpio bindings
            [1].

 * Patch 2: Clean up `bindings::*` for pl011, which is inspired by
            Paolo's comment on HPET [2].

[1]: https://lore.kernel.org/qemu-devel/Z4h3Q%2FJBxtWxi+bK@intel.com/
[2]: https://lore.kernel.org/qemu-devel/b107c5c3-9ee4-4939-a4e3-eff0cd92bad6@redhat.com/

Thanks and Best Regards,
Zhao

---
Zhao Liu (2):
  rust/qdev: Make REALIZE safe
  rust/pl011: Avoid bindings::*

 rust/hw/char/pl011/src/device.rs | 23 +++++++++++++++--------
 rust/qemu-api/src/qdev.rs        |  2 +-
 2 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH RESEND 1/2] rust/qdev: Make REALIZE safe

[Zhao Liu]

A safe REALIZE accepts immutable reference.

Since current PL011's realize() only calls a char binding function (
qemu_chr_fe_set_handlers), it is possible to convert mutable reference
(&mut self) to immutable reference (&self), which only needs to convert
the pointers passed to C to mutable pointers.

Thus, make REALIZE accept immutable reference.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/hw/char/pl011/src/device.rs | 10 +++++-----
 rust/qemu-api/src/qdev.rs        |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 994c2fc0593c..c64af2684b7d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,7 +2,7 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use core::ptr::{addr_of_mut, NonNull};
+use core::ptr::{addr_of, addr_of_mut, NonNull};
 use std::{
     ffi::CStr,
     os::raw::{c_int, c_uint, c_void},
@@ -155,7 +155,7 @@ fn properties() -> &'static [Property] {
     fn vmsd() -> Option<&'static VMStateDescription> {
         Some(&device_class::VMSTATE_PL011)
     }
-    const REALIZE: Option<fn(&mut Self)> = Some(Self::realize);
+    const REALIZE: Option<fn(&Self)> = Some(Self::realize);
     const RESET: Option<fn(&mut Self)> = Some(Self::reset);
 }
 
@@ -438,17 +438,17 @@ fn set_read_trigger(&mut self) {
         self.read_trigger = 1;
     }
 
-    pub fn realize(&mut self) {
+    pub fn realize(&self) {
         // SAFETY: self.char_backend has the correct size and alignment for a
         // CharBackend object, and its callbacks are of the correct types.
         unsafe {
             qemu_chr_fe_set_handlers(
-                addr_of_mut!(self.char_backend),
+                addr_of!(self.char_backend) as *mut CharBackend,
                 Some(pl011_can_receive),
                 Some(pl011_receive),
                 Some(pl011_event),
                 None,
-                addr_of_mut!(*self).cast::<c_void>(),
+                addr_of!(*self).cast::<c_void>() as *mut c_void,
                 core::ptr::null_mut(),
                 true,
             );
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 686054e737ac..a5121e31a37a 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -23,7 +23,7 @@ pub trait DeviceImpl {
     ///
     /// If not `None`, the parent class's `realize` method is overridden
     /// with the function pointed to by `REALIZE`.
-    const REALIZE: Option<fn(&mut Self)> = None;
+    const REALIZE: Option<fn(&Self)> = None;
 
     /// If not `None`, the parent class's `reset` method is overridden
     /// with the function pointed to by `RESET`.
-- 
2.34.1

[PATCH RESEND 2/2] rust/pl011: Avoid bindings::*

[Zhao Liu]

List all the necessary bindings to better identify gaps in rust/qapi.
And include the bindings wrapped by rust/qapi instead mapping the raw
bindings directly.

Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 rust/hw/char/pl011/src/device.rs | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index c64af2684b7d..c95ab49fd62e 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -9,12 +9,19 @@
 };
 
 use qemu_api::{
-    bindings::{self, *},
+    bindings::{
+        error_fatal, hwaddr, memory_region_init_io, qdev_init_clock_in, qdev_new,
+        qdev_prop_set_chr, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers, qemu_chr_fe_write_all,
+        qemu_irq, sysbus_connect_irq, sysbus_mmio_map, sysbus_realize_and_unref, CharBackend,
+        Chardev, Clock, ClockEvent, MemoryRegion, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
+    },
     c_str,
     irq::InterruptSource,
     prelude::*,
-    qdev::DeviceImpl,
+    qdev::{DeviceImpl, DeviceState, Property},
     qom::{ClassInitImpl, ObjectImpl, ParentField},
+    sysbus::{SysBusDevice, SysBusDeviceClass},
+    vmstate::VMStateDescription,
 };
 
 use crate::{
@@ -493,7 +500,7 @@ pub fn can_receive(&self) -> bool {
     }
 
     pub fn event(&mut self, event: QEMUChrEvent) {
-        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
+        if event == QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
             self.put_fifo(registers::Data::BREAK.into());
         }
     }
-- 
2.34.1

Re: [PATCH RESEND 0/2] rust/pl011: miscellaneous cleanups

[Paolo Bonzini]

On 1/21/25 15:04, Zhao Liu wrote:
> (Resend the series since it was missed on https://lore.kernel.org/qemu-devel/.)
> 
> Hi,
> 
> This series includes the following cleanups:
> 
>   * Patch 1: Make realize() safe to only accept immutable self
>              reference, which is in prepare to introduce gpio bindings
>              [1].
> 
>   * Patch 2: Clean up `bindings::*` for pl011, which is inspired by
>              Paolo's comment on HPET [2].

Yep, me being lazy and not doing what I preach.

Queued---please forgive me for not posting again the series that are on 
the list, just with the "use bindings::foo" conflicts resolved. :)

Also, if you have to choose please review only the final 
qdev/MemoryRegion bits; the vmstate is mostly the same and the pl011 one 
has less impact on future code.

Paolo

> [1]: https://lore.kernel.org/qemu-devel/Z4h3Q%2FJBxtWxi+bK@intel.com/
> [2]: https://lore.kernel.org/qemu-devel/b107c5c3-9ee4-4939-a4e3-eff0cd92bad6@redhat.com/
> 
> Thanks and Best Regards,
> Zhao
> 
> ---
> Zhao Liu (2):
>    rust/qdev: Make REALIZE safe
>    rust/pl011: Avoid bindings::*
> 
>   rust/hw/char/pl011/src/device.rs | 23 +++++++++++++++--------
>   rust/qemu-api/src/qdev.rs        |  2 +-
>   2 files changed, 16 insertions(+), 9 deletions(-)
> 

Re: [PATCH RESEND 0/2] rust/pl011: miscellaneous cleanups

[Zhao Liu]

On Tue, Jan 21, 2025 at 04:58:21PM +0100, Paolo Bonzini wrote:
> Date: Tue, 21 Jan 2025 16:58:21 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH RESEND 0/2] rust/pl011: miscellaneous cleanups
> 
> On 1/21/25 15:04, Zhao Liu wrote:
> > (Resend the series since it was missed on https://lore.kernel.org/qemu-devel/.)
> > 
> > Hi,
> > 
> > This series includes the following cleanups:
> > 
> >   * Patch 1: Make realize() safe to only accept immutable self
> >              reference, which is in prepare to introduce gpio bindings
> >              [1].
> > 
> >   * Patch 2: Clean up `bindings::*` for pl011, which is inspired by
> >              Paolo's comment on HPET [2].
> 
> Yep, me being lazy and not doing what I preach.
>
> Queued---please forgive me for not posting again the series that are on the
> list, just with the "use bindings::foo" conflicts resolved. :)

Thank you! I just happened to see it and clean it up.

> Also, if you have to choose please review only the final qdev/MemoryRegion
> bits; the vmstate is mostly the same and the pl011 one has less impact on
> future code.

No problem, I remember. I'll review all of them soon (I've been catching up
on some debts I owed these days).

Thanks,
Zhao