Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell

[Zhao Liu <zhao1.liu@intel.com>]

On Fri, Jan 17, 2025 at 10:26:54AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:54 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
> X-Mailer: git-send-email 2.47.1
> 
> This is a step towards making memory ops use a shared reference to the
> device type; it's not yet possible due to the calls to character device
> functions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs       | 38 +++++++++++++-------------
>  rust/hw/char/pl011/src/device_class.rs |  8 +++---
>  rust/hw/char/pl011/src/memory_ops.rs   |  2 +-
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 476abe765a9..1d3da59e481 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -102,14 +102,14 @@ pub struct PL011Registers {
>  }
>  
>  #[repr(C)]
> -#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]

This is the issue I also met, so why not drive "Debug" for BqlRefCell?

I tried to do this in [*]. Do we need to reconsider this?

[*]: https://lore.kernel.org/qemu-devel/20241205060714.256270-3-zhao1.liu@intel.com/

> +#[derive(qemu_api_macros::Object, qemu_api_macros::offsets)]
>  /// PL011 Device Model in QEMU
>  pub struct PL011State {
>      pub parent_obj: ParentField<SysBusDevice>,
>      pub iomem: MemoryRegion,
>      #[doc(alias = "chr")]
>      pub char_backend: CharBackend,
> -    pub regs: PL011Registers,
> +    pub regs: BqlRefCell<PL011Registers>,

This is a good example on the usage of BqlRefCell!

//! `BqlRefCell` is best suited for data that is primarily accessed by the
//! device's own methods, where multiple reads and writes can be grouped within
//! a single borrow and a mutable reference can be passed around. "

>      /// QEMU interrupts
>      ///
>      /// ```text
> @@ -530,8 +530,8 @@ fn post_init(&self) {
>          }
>      }
>  
> +    #[allow(clippy::needless_pass_by_ref_mut)]

How did you trigger this lint error? I switched to 1.84 and didn't get
any errors (I noticed that 1.84 fixed the issue of ignoring `self` [*],
but it still doesn't seem to work on my side).

[*]: https://github.com/rust-lang/rust-clippy/pull/12693

>      pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
> -        let regs = &mut self.regs;
>          match RegisterOffset::try_from(offset) {
>              Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
>                  let device_id = self.get_class().device_id;
> @@ -541,7 +541,7 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
>                  // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
>                  ControlFlow::Break(0)
>              }
> -            Ok(field) => match regs.read(field) {
> +            Ok(field) => match self.regs.borrow_mut().read(field) {
>                  ControlFlow::Break(value) => ControlFlow::Break(value.into()),
>                  ControlFlow::Continue(value) => {
>                      self.update();

[snip]

> @@ -603,19 +603,19 @@ pub fn realize(&mut self) {
>      }
>  
>      pub fn reset(&mut self) {

In principle, this place should also trigger `needless_pass_by_ref_mut`.

> -        self.regs.reset();
> +        self.regs.borrow_mut().reset();
>      }

[snip]

> @@ -657,10 +657,10 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
>  pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const u8, size: c_int) {
>      unsafe {
>          debug_assert!(!opaque.is_null());
> -        let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
> +        let state = NonNull::new_unchecked(opaque.cast::<PL011State>());

Perhaps we can use NonNull::new and unwrap()? Then debug_assert! is
unnecessary.

let state = unsafe { NonNull::new(opaque.cast::<PL011State>()).unwrap().as_ref() };

>          if size > 0 {
>              debug_assert!(!buf.is_null());
> -            state.as_mut().receive(u32::from(buf.read_volatile()));
> +            state.as_ref().receive(u32::from(buf.read_volatile()));
>          }
>      }
>  }
> @@ -673,8 +673,8 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
>  pub unsafe extern "C" fn pl011_event(opaque: *mut c_void, event: QEMUChrEvent) {
>      unsafe {

I think we could narrow the unsafe scope next.

>          debug_assert!(!opaque.is_null());
> -        let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
> -        state.as_mut().event(event)
> +        let state = NonNull::new_unchecked(opaque.cast::<PL011State>());
> +        state.as_ref().event(event)
>      }
>  }
  
[snip]

> diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
> index c4e8599ba43..8f66c8d492c 100644
> --- a/rust/hw/char/pl011/src/memory_ops.rs
> +++ b/rust/hw/char/pl011/src/memory_ops.rs
> @@ -26,7 +26,7 @@
>  unsafe extern "C" fn pl011_read(opaque: *mut c_void, addr: hwaddr, size: c_uint) -> u64 {
>      assert!(!opaque.is_null());
>      let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
> -    let val = unsafe { state.as_mut().read(addr, size) };
> +    let val = unsafe { state.as_mut() }.read(addr, size);

Nice cleanup.

>      match val {
>          std::ops::ControlFlow::Break(val) => val,
>          std::ops::ControlFlow::Continue(val) => {
> -- 

Nice transition! This is an important step. (Even my comments above
didn't affect the main work of the patch :-) )

So,

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