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 <pbonz...@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 <pbonz...@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....@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....@intel.com>
