Il gio 23 gen 2025, 06:27 Zhao Liu <zhao1....@intel.com> ha scritto:
> 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?
>
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....@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
