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)]
+#[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>,
/// QEMU interrupts
///
/// ```text
@@ -530,8 +530,8 @@ fn post_init(&self) {
}
}
+ #[allow(clippy::needless_pass_by_ref_mut)]
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();
@@ -552,7 +552,7 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
}
pub fn write(&mut self, offset: hwaddr, value: u64) {
- let regs = &mut self.regs;
+ let mut regs = self.regs.borrow_mut();
if let Ok(field) = RegisterOffset::try_from(offset) {
if regs.write(field, value as u32, &mut self.char_backend) {
self.update();
@@ -564,19 +564,19 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
pub fn can_receive(&self) -> bool {
// trace_pl011_can_receive(s->lcr, s->read_count, r);
- let regs = &self.regs;
+ let regs = self.regs.borrow();
regs.read_count < regs.fifo_depth()
}
- pub fn receive(&mut self, ch: u32) {
- let regs = &mut self.regs;
+ pub fn receive(&self, ch: u32) {
+ let mut regs = self.regs.borrow_mut();
if !regs.loopback_enabled() && regs.put_fifo(ch) {
self.update();
}
}
- pub fn event(&mut self, event: QEMUChrEvent) {
- let regs = &mut self.regs;
+ pub fn event(&self, event: QEMUChrEvent) {
+ let mut regs = self.regs.borrow_mut();
if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !regs.loopback_enabled() {
let update = regs.put_fifo(registers::Data::BREAK.into());
if update {
@@ -603,19 +603,19 @@ pub fn realize(&mut self) {
}
pub fn reset(&mut self) {
- self.regs.reset();
+ self.regs.borrow_mut().reset();
}
pub fn update(&self) {
- let regs = &self.regs;
+ let regs = self.regs.borrow();
let flags = regs.int_level & regs.int_enabled;
for (irq, i) in self.interrupts.iter().zip(IRQMASK) {
irq.set(flags & i != 0);
}
}
- pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
- self.regs.post_load()
+ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
+ self.regs.borrow_mut().post_load()
}
}
@@ -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>());
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 {
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)
}
}
@@ -700,7 +700,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
}
#[repr(C)]
-#[derive(Debug, qemu_api_macros::Object)]
+#[derive(qemu_api_macros::Object)]
/// PL011 Luminary device model.
pub struct PL011Luminary {
parent_obj: ParentField<PL011State>,
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index e25645ceb0d..06178778a8e 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -6,7 +6,7 @@
use std::os::raw::{c_int, c_void};
use qemu_api::{
- bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct,
+ bindings::*, c_str, prelude::*, vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct,
vmstate_subsections, vmstate_unused, zeroable::Zeroable,
};
@@ -35,8 +35,8 @@ extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
unsafe {
debug_assert!(!opaque.is_null());
- let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
- let result = state.as_mut().post_load(version_id as u32);
+ let state = NonNull::new_unchecked(opaque.cast::<PL011State>());
+ let result = state.as_ref().post_load(version_id as u32);
if result.is_err() {
-1
} else {
@@ -76,7 +76,7 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
post_load: Some(pl011_post_load),
fields: vmstate_fields! {
vmstate_unused!(core::mem::size_of::<u32>()),
- vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, PL011Registers),
+ vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, BqlRefCell<PL011Registers>),
},
subsections: vmstate_subsections! {
VMSTATE_PL011_CLOCK
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);
match val {
std::ops::ControlFlow::Break(val) => val,
std::ops::ControlFlow::Continue(val) => {
--
2.47.1On 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>
Il gio 23 gen 2025, 06:27 Zhao Liu <zhao1.liu@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 <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
> > > #[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.
Thank you for the clarification, I understand now (I was indeed puzzled
as to why RefCell didn't do this).
> 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.
yes, I agree here we should remove mut :-). (if needless_pass_by_ref_mut
doesn't work on this place, I think we can drop it.)
> (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.
Make sense!
> > > - 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.
>
I prefer to use NonNull::new and unwrap(). Too much assert() pattern is
not user-friendly. I also think it's unnecessary to change NonNull
interface in this patch, we can see what's left when you're done with
the most QAPI work.
Thanks,
Zhao
Il gio 23 gen 2025, 10:05 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > 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.
>
> yes, I agree here we should remove mut :-). (if needless_pass_by_ref_mut
> doesn't work on this place, I think we can drop it.)
>
&mut is not needed here, but it is needed in write(). After accept_input()
is moved to out of memory_ops.rs, the #[allow] can be removed.
I will do that in v2.
Paolo
> > (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.
>
> Make sense!
>
> > > > - 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.
> >
>
> I prefer to use NonNull::new and unwrap(). Too much assert() pattern is
> not user-friendly. I also think it's unnecessary to change NonNull
> interface in this patch, we can see what's left when you're done with
> the most QAPI work.
>
> Thanks,
> Zhao
>
>
>
© 2016 - 2026 Red Hat, Inc.