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?
Because it is not entirely possible to do it safely--there could be outstanding borrows that break invariants and cause debug() to fail. Maybe we could implement it on BqlRefCell<PL011Registers> with a custom derive macro...
RefCell doesn't implement Debug either for the same reason.
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. "
Yeah, the comment was inspired by this usage and not vice versa. :D
> /// 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).
I will double check. But I do see that there is no mut access inside, at least not until the qemu_chr_fe_accept_input() is moved here. Unfortunately until all MemoryRegion and CharBackend bindings are available the uses of &mut and the casts to *mut are really really wonky.
(On the other hand it wouldn't be possible to have a grip on the qemu_api code without users).
Paolo
> @@ -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`.
Yes but clippy hides it because this function is assigned to a function pointer const. At least I think so---the point is more generally that you can't change &mut to & without breaking compilation.
> - 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() };
Yeah, though that's preexisting and it's code that will go away relatively soon. I tried to minimize unrelated changes and changes to these temporary unsafe functions, but in some cases there were some that sneaked in.
Let me know what you prefer.
Paolo