* [PATCH 00/10] rust: pl011: correctly use interior mutability
@ 2025-01-17 9:26 Paolo Bonzini
2025-01-17 9:26 ` [PATCH 01/10] rust: pl011: remove unnecessary "extern crate" Paolo Bonzini
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
QOM devices are aliased from the moment that they are added to the
QOM tree, and therefore must not use &mut. This has been a known
issue since the beginning of the Rust in QEMU project, and since
then a solution was developed in the form of BqlCell and BqlRefCell.
This series moves the MMIO code and registers from PL011State to
a new struct PL011Registers, which is wrapped with BqlRefCell.
This also allows to remove device-specific code from the device's
MemoryRegionOps callbacks, paving the way for MemoryRegionOps
bindings.
I am making this series a prerequisite to the usage of Resettable,
because it allows reset to reset take a shared reference to the
device. Thus the Resettable implementation will not have to
temporarily take a mut reference.
Paolo
Paolo Bonzini (10):
rust: pl011: remove unnecessary "extern crate"
rust: pl011: hide unnecessarily "pub" items from outside pl011::device
rust: pl011: extract conversion to RegisterOffset
rust: pl011: extract CharBackend receive logic into a separate function
rust: pl011: pull interrupt updates out of read/write ops
rust: pl011: extract PL011Registers
rust: pl011: wrap registers with BqlRefCell
rust: pl011: remove duplicate definitions
rust: pl011: pull device-specific code out of MemoryRegionOps callbacks
rust: qdev: make reset take a shared reference
rust/hw/char/pl011/src/device.rs | 458 ++++++++++++++-----------
rust/hw/char/pl011/src/device_class.rs | 52 +--
rust/hw/char/pl011/src/lib.rs | 61 ++--
rust/hw/char/pl011/src/memory_ops.rs | 23 +-
rust/qemu-api/src/qdev.rs | 2 +-
5 files changed, 314 insertions(+), 282 deletions(-)
--
2.47.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 01/10] rust: pl011: remove unnecessary "extern crate"
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-22 13:37 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device Paolo Bonzini
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/lib.rs | 4 ----
1 file changed, 4 deletions(-)
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index f30f9850ad4..d10f0805aac 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -25,10 +25,6 @@
#![allow(clippy::upper_case_acronyms)]
#![allow(clippy::result_unit_err)]
-extern crate bilge;
-extern crate bilge_impl;
-extern crate qemu_api;
-
use qemu_api::c_str;
pub mod device;
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
2025-01-17 9:26 ` [PATCH 01/10] rust: pl011: remove unnecessary "extern crate" Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-22 13:39 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset Paolo Bonzini
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
The only public interfaces for pl011 are TYPE_PL011 and pl011_create.
Remove pub from everything else.
Note: the "allow(dead_code)" is removed later.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 2 +-
rust/hw/char/pl011/src/device_class.rs | 2 +-
rust/hw/char/pl011/src/lib.rs | 13 ++++++++-----
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 11a87664c7a..e85e46ba0bb 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -565,7 +565,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
}
/// Which bits in the interrupt status matter for each outbound IRQ line ?
-pub const IRQMASK: [u32; 6] = [
+const IRQMASK: [u32; 6] = [
/* combined IRQ */
Interrupt::E
| Interrupt::MS
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index b052d98803f..2fd805fd12d 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -21,7 +21,7 @@ extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
}
/// Migration subsection for [`PL011State`] clock.
-pub static VMSTATE_PL011_CLOCK: VMStateDescription = VMStateDescription {
+static VMSTATE_PL011_CLOCK: VMStateDescription = VMStateDescription {
name: c_str!("pl011/clock").as_ptr(),
version_id: 1,
minimum_version_id: 1,
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index d10f0805aac..2baacba2306 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -27,9 +27,11 @@
use qemu_api::c_str;
-pub mod device;
-pub mod device_class;
-pub mod memory_ops;
+mod device;
+mod device_class;
+mod memory_ops;
+
+pub use device::pl011_create;
pub const TYPE_PL011: &::std::ffi::CStr = c_str!("pl011");
pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c_str!("pl011_luminary");
@@ -42,7 +44,7 @@
#[allow(non_camel_case_types)]
#[repr(u64)]
#[derive(Debug, qemu_api_macros::TryInto)]
-pub enum RegisterOffset {
+enum RegisterOffset {
/// Data Register
///
/// A write to this register initiates the actual data transmission
@@ -98,7 +100,8 @@ pub enum RegisterOffset {
//Reserved = 0x04C,
}
-pub mod registers {
+#[allow(dead_code)]
+mod registers {
//! Device registers exposed as typed structs which are backed by arbitrary
//! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
use bilge::prelude::*;
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
2025-01-17 9:26 ` [PATCH 01/10] rust: pl011: remove unnecessary "extern crate" Paolo Bonzini
2025-01-17 9:26 ` [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-22 14:34 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function Paolo Bonzini
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
As an added bonus, this also makes the new function return u32 instead
of u64, thus factoring some casts into a single place.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 114 +++++++++++++++++--------------
1 file changed, 63 insertions(+), 51 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index e85e46ba0bb..6d662865182 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -5,6 +5,7 @@
use core::ptr::{addr_of_mut, NonNull};
use std::{
ffi::CStr,
+ ops::ControlFlow,
os::raw::{c_int, c_uint, c_void},
};
@@ -214,19 +215,11 @@ fn post_init(&self) {
}
}
- pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
+ fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
use RegisterOffset::*;
- let value = match RegisterOffset::try_from(offset) {
- Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
- let device_id = self.get_class().device_id;
- u32::from(device_id[(offset - 0xfe0) >> 2])
- }
- Err(_) => {
- // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
- 0
- }
- Ok(DR) => {
+ std::ops::ControlFlow::Break(match offset {
+ DR => {
self.flags.set_receive_fifo_full(false);
let c = self.read_fifo[self.read_pos];
if self.read_count > 0 {
@@ -243,39 +236,33 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
self.receive_status_error_clear.set_from_data(c);
self.update();
// Must call qemu_chr_fe_accept_input, so return Continue:
- let c = u32::from(c);
- return std::ops::ControlFlow::Continue(u64::from(c));
- }
- Ok(RSR) => u32::from(self.receive_status_error_clear),
- Ok(FR) => u32::from(self.flags),
- Ok(FBRD) => self.fbrd,
- Ok(ILPR) => self.ilpr,
- Ok(IBRD) => self.ibrd,
- Ok(LCR_H) => u32::from(self.line_control),
- Ok(CR) => u32::from(self.control),
- Ok(FLS) => self.ifl,
- Ok(IMSC) => self.int_enabled,
- Ok(RIS) => self.int_level,
- Ok(MIS) => self.int_level & self.int_enabled,
- Ok(ICR) => {
+ return ControlFlow::Continue(u32::from(c));
+ },
+ RSR => u32::from(self.receive_status_error_clear),
+ FR => u32::from(self.flags),
+ FBRD => self.fbrd,
+ ILPR => self.ilpr,
+ IBRD => self.ibrd,
+ LCR_H => u32::from(self.line_control),
+ CR => u32::from(self.control),
+ FLS => self.ifl,
+ IMSC => self.int_enabled,
+ RIS => self.int_level,
+ MIS => self.int_level & self.int_enabled,
+ ICR => {
// "The UARTICR Register is the interrupt clear register and is write-only"
// Source: ARM DDI 0183G 3.3.13 Interrupt Clear Register, UARTICR
0
- }
- Ok(DMACR) => self.dmacr,
- };
- std::ops::ControlFlow::Break(value.into())
+ },
+ DMACR => self.dmacr,
+ })
}
- pub fn write(&mut self, offset: hwaddr, value: u64) {
+ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
// eprintln!("write offset {offset} value {value}");
use RegisterOffset::*;
- let value: u32 = value as u32;
- match RegisterOffset::try_from(offset) {
- Err(_bad_offset) => {
- eprintln!("write bad offset {offset} value {value}");
- }
- Ok(DR) => {
+ match offset {
+ DR => {
// ??? Check if transmitter is enabled.
let ch: u8 = value as u8;
// XXX this blocks entire thread. Rewrite to use
@@ -290,22 +277,22 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.int_level |= registers::INT_TX;
self.update();
}
- Ok(RSR) => {
- self.receive_status_error_clear.reset();
+ RSR => {
+ self.receive_status_error_clear = 0.into();
}
- Ok(FR) => {
+ FR => {
// flag writes are ignored
}
- Ok(ILPR) => {
+ ILPR => {
self.ilpr = value;
}
- Ok(IBRD) => {
+ IBRD => {
self.ibrd = value;
}
- Ok(FBRD) => {
+ FBRD => {
self.fbrd = value;
}
- Ok(LCR_H) => {
+ LCR_H => {
let new_val: registers::LineControl = value.into();
// Reset the FIFO state on FIFO enable or disable
if self.line_control.fifos_enabled() != new_val.fifos_enabled() {
@@ -328,26 +315,26 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.line_control = new_val;
self.set_read_trigger();
}
- Ok(CR) => {
+ CR => {
// ??? Need to implement the enable bit.
self.control = value.into();
self.loopback_mdmctrl();
}
- Ok(FLS) => {
+ FLS => {
self.ifl = value;
self.set_read_trigger();
}
- Ok(IMSC) => {
+ IMSC => {
self.int_enabled = value;
self.update();
}
- Ok(RIS) => {}
- Ok(MIS) => {}
- Ok(ICR) => {
+ RIS => {}
+ MIS => {}
+ ICR => {
self.int_level &= !value;
self.update();
}
- Ok(DMACR) => {
+ DMACR => {
self.dmacr = value;
if value & 3 > 0 {
// qemu_log_mask(LOG_UNIMP, "pl011: DMA not implemented\n");
@@ -562,6 +549,31 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
Ok(())
}
+
+ pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
+ match RegisterOffset::try_from(offset) {
+ Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
+ let device_id = self.get_class().device_id;
+ ControlFlow::Break(u64::from(device_id[(offset - 0xfe0) >> 2]))
+ }
+ Err(_) => {
+ // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
+ ControlFlow::Break(0)
+ }
+ Ok(field) => match self.regs_read(field) {
+ ControlFlow::Break(value) => ControlFlow::Break(value.into()),
+ ControlFlow::Continue(value) => ControlFlow::Continue(value.into()),
+ }
+ }
+ }
+
+ pub fn write(&mut self, offset: hwaddr, value: u64) {
+ if let Ok(field) = RegisterOffset::try_from(offset) {
+ self.regs_write(field, value as u32);
+ } else {
+ eprintln!("write bad offset {offset} value {value}");
+ }
+ }
}
/// Which bits in the interrupt status matter for each outbound IRQ line ?
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (2 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-22 14:59 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops Paolo Bonzini
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Prepare for moving all references to the registers and the FIFO into a
separate struct.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 6d662865182..2e8707aef97 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -6,7 +6,7 @@
use std::{
ffi::CStr,
ops::ControlFlow,
- os::raw::{c_int, c_uint, c_void},
+ os::raw::{c_int, c_void},
};
use qemu_api::{
@@ -480,6 +480,12 @@ pub fn can_receive(&self) -> bool {
self.read_count < self.fifo_depth()
}
+ pub fn receive(&mut self, ch: u32) {
+ if !self.loopback_enabled() {
+ self.put_fifo(ch)
+ }
+ }
+
pub fn event(&mut self, event: QEMUChrEvent) {
if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
self.put_fifo(registers::Data::BREAK.into());
@@ -505,7 +511,7 @@ pub fn fifo_depth(&self) -> u32 {
1
}
- pub fn put_fifo(&mut self, value: c_uint) {
+ pub fn put_fifo(&mut self, value: u32) {
let depth = self.fifo_depth();
assert!(depth > 0);
let slot = (self.read_pos + self.read_count) & (depth - 1);
@@ -615,12 +621,9 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
unsafe {
debug_assert!(!opaque.is_null());
let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
- if state.as_ref().loopback_enabled() {
- return;
- }
if size > 0 {
debug_assert!(!buf.is_null());
- state.as_mut().put_fifo(c_uint::from(buf.read_volatile()))
+ state.as_mut().receive(u32::from(buf.read_volatile()));
}
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (3 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-22 16:50 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 06/10] rust: pl011: extract PL011Registers Paolo Bonzini
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
qemu_irqs are not part of the vmstate, therefore they will remain in
PL011State. Update them if needed after regs_read()/regs_write().
Apply #[must_use] to functions that return whether the interrupt state
could have changed, so that it's harder to forget the call to update().
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 68 ++++++++++++++++++--------------
1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2e8707aef97..67c3e63baa1 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -234,7 +234,6 @@ fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
}
// Update error bits.
self.receive_status_error_clear.set_from_data(c);
- self.update();
// Must call qemu_chr_fe_accept_input, so return Continue:
return ControlFlow::Continue(u32::from(c));
},
@@ -258,7 +257,7 @@ fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
})
}
- fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
+ fn regs_write(&mut self, offset: RegisterOffset, value: u32) -> bool {
// eprintln!("write offset {offset} value {value}");
use RegisterOffset::*;
match offset {
@@ -273,9 +272,10 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
unsafe {
qemu_chr_fe_write_all(addr_of_mut!(self.char_backend), &ch, 1);
}
- self.loopback_tx(value);
+ // interrupts always checked
+ let _ = self.loopback_tx(value);
self.int_level |= registers::INT_TX;
- self.update();
+ return true;
}
RSR => {
self.receive_status_error_clear = 0.into();
@@ -299,7 +299,7 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
self.reset_rx_fifo();
self.reset_tx_fifo();
}
- if self.line_control.send_break() ^ new_val.send_break() {
+ let update = (self.line_control.send_break() != new_val.send_break()) && {
let mut break_enable: c_int = new_val.send_break().into();
// SAFETY: self.char_backend is a valid CharBackend instance after it's been
// initialized in realize().
@@ -310,15 +310,16 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
addr_of_mut!(break_enable).cast::<c_void>(),
);
}
- self.loopback_break(break_enable > 0);
- }
+ self.loopback_break(break_enable > 0)
+ };
self.line_control = new_val;
self.set_read_trigger();
+ return update;
}
CR => {
// ??? Need to implement the enable bit.
self.control = value.into();
- self.loopback_mdmctrl();
+ return self.loopback_mdmctrl();
}
FLS => {
self.ifl = value;
@@ -326,13 +327,13 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
}
IMSC => {
self.int_enabled = value;
- self.update();
+ return true;
}
RIS => {}
MIS => {}
ICR => {
self.int_level &= !value;
- self.update();
+ return true;
}
DMACR => {
self.dmacr = value;
@@ -342,14 +343,12 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
}
}
}
+ false
}
#[inline]
- fn loopback_tx(&mut self, value: u32) {
- if !self.loopback_enabled() {
- return;
- }
-
+ #[must_use]
+ fn loopback_tx(&mut self, value: u32) -> bool {
// Caveat:
//
// In real hardware, TX loopback happens at the serial-bit level
@@ -367,12 +366,13 @@ fn loopback_tx(&mut self, value: u32) {
// hardware flow-control is enabled.
//
// For simplicity, the above described is not emulated.
- self.put_fifo(value);
+ self.loopback_enabled() && self.put_fifo(value)
}
- fn loopback_mdmctrl(&mut self) {
+ #[must_use]
+ fn loopback_mdmctrl(&mut self) -> bool {
if !self.loopback_enabled() {
- return;
+ return false;
}
/*
@@ -413,13 +413,11 @@ fn loopback_mdmctrl(&mut self) {
il |= Interrupt::RI as u32;
}
self.int_level = il;
- self.update();
+ true
}
- fn loopback_break(&mut self, enable: bool) {
- if enable {
- self.loopback_tx(registers::Data::BREAK.into());
- }
+ fn loopback_break(&mut self, enable: bool) -> bool {
+ enable && self.loopback_tx(registers::Data::BREAK.into())
}
fn set_read_trigger(&mut self) {
@@ -481,14 +479,17 @@ pub fn can_receive(&self) -> bool {
}
pub fn receive(&mut self, ch: u32) {
- if !self.loopback_enabled() {
- self.put_fifo(ch)
+ if !self.loopback_enabled() && self.put_fifo(ch) {
+ self.update();
}
}
pub fn event(&mut self, event: QEMUChrEvent) {
if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
- self.put_fifo(registers::Data::BREAK.into());
+ let update = self.put_fifo(registers::Data::BREAK.into());
+ if update {
+ self.update();
+ }
}
}
@@ -511,7 +512,8 @@ pub fn fifo_depth(&self) -> u32 {
1
}
- pub fn put_fifo(&mut self, value: u32) {
+ #[must_use]
+ pub fn put_fifo(&mut self, value: u32) -> bool {
let depth = self.fifo_depth();
assert!(depth > 0);
let slot = (self.read_pos + self.read_count) & (depth - 1);
@@ -524,8 +526,9 @@ pub fn put_fifo(&mut self, value: u32) {
if self.read_count == self.read_trigger {
self.int_level |= registers::INT_RX;
- self.update();
+ return true;
}
+ false
}
pub fn update(&self) {
@@ -568,14 +571,19 @@ pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
}
Ok(field) => match self.regs_read(field) {
ControlFlow::Break(value) => ControlFlow::Break(value.into()),
- ControlFlow::Continue(value) => ControlFlow::Continue(value.into()),
+ ControlFlow::Continue(value) => {
+ self.update();
+ ControlFlow::Continue(value.into())
+ },
}
}
}
pub fn write(&mut self, offset: hwaddr, value: u64) {
if let Ok(field) = RegisterOffset::try_from(offset) {
- self.regs_write(field, value as u32);
+ if self.regs_write(field, value as u32) {
+ self.update();
+ }
} else {
eprintln!("write bad offset {offset} value {value}");
}
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 06/10] rust: pl011: extract PL011Registers
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (4 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-23 3:44 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell Paolo Bonzini
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Pull all the mutable fields of PL011State into a separate struct.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 251 ++++++++++++++-----------
rust/hw/char/pl011/src/device_class.rs | 46 +++--
2 files changed, 168 insertions(+), 129 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 67c3e63baa1..476abe765a9 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -78,11 +78,8 @@ fn index(&self, idx: u32) -> &Self::Output {
}
#[repr(C)]
-#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
-/// PL011 Device Model in QEMU
-pub struct PL011State {
- pub parent_obj: ParentField<SysBusDevice>,
- pub iomem: MemoryRegion,
+#[derive(Debug, Default, qemu_api_macros::offsets)]
+pub struct PL011Registers {
#[doc(alias = "fr")]
pub flags: registers::Flags,
#[doc(alias = "lcr")]
@@ -102,8 +99,17 @@ pub struct PL011State {
pub read_pos: u32,
pub read_count: u32,
pub read_trigger: u32,
+}
+
+#[repr(C)]
+#[derive(Debug, 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,
/// QEMU interrupts
///
/// ```text
@@ -161,61 +167,8 @@ fn vmsd() -> Option<&'static VMStateDescription> {
const RESET: Option<fn(&mut Self)> = Some(Self::reset);
}
-impl PL011State {
- /// Initializes a pre-allocated, unitialized instance of `PL011State`.
- ///
- /// # Safety
- ///
- /// `self` must point to a correctly sized and aligned location for the
- /// `PL011State` type. It must not be called more than once on the same
- /// location/instance. All its fields are expected to hold unitialized
- /// values with the sole exception of `parent_obj`.
- unsafe fn init(&mut self) {
- const CLK_NAME: &CStr = c_str!("clk");
-
- // SAFETY:
- //
- // self and self.iomem are guaranteed to be valid at this point since callers
- // must make sure the `self` reference is valid.
- unsafe {
- memory_region_init_io(
- addr_of_mut!(self.iomem),
- addr_of_mut!(*self).cast::<Object>(),
- &PL011_OPS,
- addr_of_mut!(*self).cast::<c_void>(),
- Self::TYPE_NAME.as_ptr(),
- 0x1000,
- );
- }
-
- // SAFETY:
- //
- // self.clock is not initialized at this point; but since `NonNull<_>` is Copy,
- // we can overwrite the undefined value without side effects. This is
- // safe since all PL011State instances are created by QOM code which
- // calls this function to initialize the fields; therefore no code is
- // able to access an invalid self.clock value.
- unsafe {
- let dev: &mut DeviceState = self.upcast_mut();
- self.clock = NonNull::new(qdev_init_clock_in(
- dev,
- CLK_NAME.as_ptr(),
- None, /* pl011_clock_update */
- addr_of_mut!(*self).cast::<c_void>(),
- ClockEvent::ClockUpdate.0,
- ))
- .unwrap();
- }
- }
-
- fn post_init(&self) {
- self.init_mmio(&self.iomem);
- for irq in self.interrupts.iter() {
- self.init_irq(irq);
- }
- }
-
- fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
+impl PL011Registers {
+ pub(self) fn read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
use RegisterOffset::*;
std::ops::ControlFlow::Break(match offset {
@@ -257,7 +210,12 @@ fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
})
}
- fn regs_write(&mut self, offset: RegisterOffset, value: u32) -> bool {
+ pub(self) fn write(
+ &mut self,
+ offset: RegisterOffset,
+ value: u32,
+ char_backend: *mut CharBackend,
+ ) -> bool {
// eprintln!("write offset {offset} value {value}");
use RegisterOffset::*;
match offset {
@@ -267,10 +225,10 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) -> bool {
// XXX this blocks entire thread. Rewrite to use
// qemu_chr_fe_write and background I/O callbacks
- // SAFETY: self.char_backend is a valid CharBackend instance after it's been
+ // SAFETY: char_backend is a valid CharBackend instance after it's been
// initialized in realize().
unsafe {
- qemu_chr_fe_write_all(addr_of_mut!(self.char_backend), &ch, 1);
+ qemu_chr_fe_write_all(char_backend, &ch, 1);
}
// interrupts always checked
let _ = self.loopback_tx(value);
@@ -305,7 +263,7 @@ fn regs_write(&mut self, offset: RegisterOffset, value: u32) -> bool {
// initialized in realize().
unsafe {
qemu_chr_fe_ioctl(
- addr_of_mut!(self.char_backend),
+ char_backend,
CHR_IOCTL_SERIAL_SET_BREAK as i32,
addr_of_mut!(break_enable).cast::<c_void>(),
);
@@ -424,23 +382,6 @@ fn set_read_trigger(&mut self) {
self.read_trigger = 1;
}
- pub fn realize(&mut self) {
- // SAFETY: self.char_backend has the correct size and alignment for a
- // CharBackend object, and its callbacks are of the correct types.
- unsafe {
- qemu_chr_fe_set_handlers(
- addr_of_mut!(self.char_backend),
- Some(pl011_can_receive),
- Some(pl011_receive),
- Some(pl011_event),
- None,
- addr_of_mut!(*self).cast::<c_void>(),
- core::ptr::null_mut(),
- true,
- );
- }
- }
-
pub fn reset(&mut self) {
self.line_control.reset();
self.receive_status_error_clear.reset();
@@ -473,26 +414,6 @@ pub fn reset_tx_fifo(&mut self) {
self.flags.set_transmit_fifo_empty(true);
}
- pub fn can_receive(&self) -> bool {
- // trace_pl011_can_receive(s->lcr, s->read_count, r);
- self.read_count < self.fifo_depth()
- }
-
- pub fn receive(&mut self, ch: u32) {
- if !self.loopback_enabled() && self.put_fifo(ch) {
- self.update();
- }
- }
-
- pub fn event(&mut self, event: QEMUChrEvent) {
- if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
- let update = self.put_fifo(registers::Data::BREAK.into());
- if update {
- self.update();
- }
- }
- }
-
#[inline]
pub fn fifo_enabled(&self) -> bool {
self.line_control.fifos_enabled() == registers::Mode::FIFO
@@ -531,14 +452,7 @@ pub fn put_fifo(&mut self, value: u32) -> bool {
false
}
- pub fn update(&self) {
- let flags = self.int_level & self.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<(), ()> {
+ pub fn post_load(&mut self) -> Result<(), ()> {
/* Sanity-check input state */
if self.read_pos >= self.read_fifo.len() || self.read_count > self.read_fifo.len() {
return Err(());
@@ -558,8 +472,66 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
Ok(())
}
+}
+
+impl PL011State {
+ /// Initializes a pre-allocated, unitialized instance of `PL011State`.
+ ///
+ /// # Safety
+ ///
+ /// `self` must point to a correctly sized and aligned location for the
+ /// `PL011State` type. It must not be called more than once on the same
+ /// location/instance. All its fields are expected to hold unitialized
+ /// values with the sole exception of `parent_obj`.
+ unsafe fn init(&mut self) {
+ const CLK_NAME: &CStr = c_str!("clk");
+
+ // SAFETY:
+ //
+ // self and self.iomem are guaranteed to be valid at this point since callers
+ // must make sure the `self` reference is valid.
+ unsafe {
+ memory_region_init_io(
+ addr_of_mut!(self.iomem),
+ addr_of_mut!(*self).cast::<Object>(),
+ &PL011_OPS,
+ addr_of_mut!(*self).cast::<c_void>(),
+ Self::TYPE_NAME.as_ptr(),
+ 0x1000,
+ );
+ }
+
+ self.regs = Default::default();
+
+ // SAFETY:
+ //
+ // self.clock is not initialized at this point; but since `NonNull<_>` is Copy,
+ // we can overwrite the undefined value without side effects. This is
+ // safe since all PL011State instances are created by QOM code which
+ // calls this function to initialize the fields; therefore no code is
+ // able to access an invalid self.clock value.
+ unsafe {
+ let dev: &mut DeviceState = self.upcast_mut();
+ self.clock = NonNull::new(qdev_init_clock_in(
+ dev,
+ CLK_NAME.as_ptr(),
+ None, /* pl011_clock_update */
+ addr_of_mut!(*self).cast::<c_void>(),
+ ClockEvent::ClockUpdate.0,
+ ))
+ .unwrap();
+ }
+ }
+
+ fn post_init(&self) {
+ self.init_mmio(&self.iomem);
+ for irq in self.interrupts.iter() {
+ self.init_irq(irq);
+ }
+ }
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;
@@ -569,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 self.regs_read(field) {
+ Ok(field) => match regs.read(field) {
ControlFlow::Break(value) => ControlFlow::Break(value.into()),
ControlFlow::Continue(value) => {
self.update();
@@ -580,14 +552,71 @@ 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;
if let Ok(field) = RegisterOffset::try_from(offset) {
- if self.regs_write(field, value as u32) {
+ if regs.write(field, value as u32, &mut self.char_backend) {
self.update();
}
} else {
eprintln!("write bad offset {offset} value {value}");
}
}
+
+ pub fn can_receive(&self) -> bool {
+ // trace_pl011_can_receive(s->lcr, s->read_count, r);
+ let regs = &self.regs;
+ regs.read_count < regs.fifo_depth()
+ }
+
+ pub fn receive(&mut self, ch: u32) {
+ let regs = &mut self.regs;
+ if !regs.loopback_enabled() && regs.put_fifo(ch) {
+ self.update();
+ }
+ }
+
+ pub fn event(&mut self, event: QEMUChrEvent) {
+ let regs = &mut self.regs;
+ if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !regs.loopback_enabled() {
+ let update = regs.put_fifo(registers::Data::BREAK.into());
+ if update {
+ self.update()
+ }
+ }
+ }
+
+ pub fn realize(&mut self) {
+ // SAFETY: self.char_backend has the correct size and alignment for a
+ // CharBackend object, and its callbacks are of the correct types.
+ unsafe {
+ qemu_chr_fe_set_handlers(
+ addr_of_mut!(self.char_backend),
+ Some(pl011_can_receive),
+ Some(pl011_receive),
+ Some(pl011_event),
+ None,
+ addr_of_mut!(*self).cast::<c_void>(),
+ core::ptr::null_mut(),
+ true,
+ );
+ }
+ }
+
+ pub fn reset(&mut self) {
+ self.regs.reset();
+ }
+
+ pub fn update(&self) {
+ let regs = &self.regs;
+ 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()
+ }
}
/// Which bits in the interrupt status matter for each outbound IRQ line ?
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 2fd805fd12d..e25645ceb0d 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -6,11 +6,11 @@
use std::os::raw::{c_int, c_void};
use qemu_api::{
- bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of, vmstate_subsections,
- vmstate_unused, zeroable::Zeroable,
+ bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct,
+ vmstate_subsections, vmstate_unused, zeroable::Zeroable,
};
-use crate::device::PL011State;
+use crate::device::{PL011Registers, PL011State};
extern "C" fn pl011_clock_needed(opaque: *mut c_void) -> bool {
unsafe {
@@ -45,6 +45,30 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
}
}
+static VMSTATE_PL011_REGS: VMStateDescription = VMStateDescription {
+ name: c_str!("pl011").as_ptr(),
+ version_id: 2,
+ minimum_version_id: 2,
+ fields: vmstate_fields! {
+ vmstate_of!(PL011Registers, flags),
+ vmstate_of!(PL011Registers, line_control),
+ vmstate_of!(PL011Registers, receive_status_error_clear),
+ vmstate_of!(PL011Registers, control),
+ vmstate_of!(PL011Registers, dmacr),
+ vmstate_of!(PL011Registers, int_enabled),
+ vmstate_of!(PL011Registers, int_level),
+ vmstate_of!(PL011Registers, read_fifo),
+ vmstate_of!(PL011Registers, ilpr),
+ vmstate_of!(PL011Registers, ibrd),
+ vmstate_of!(PL011Registers, fbrd),
+ vmstate_of!(PL011Registers, ifl),
+ vmstate_of!(PL011Registers, read_pos),
+ vmstate_of!(PL011Registers, read_count),
+ vmstate_of!(PL011Registers, read_trigger),
+ },
+ ..Zeroable::ZERO
+};
+
pub static VMSTATE_PL011: VMStateDescription = VMStateDescription {
name: c_str!("pl011").as_ptr(),
version_id: 2,
@@ -52,21 +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_of!(PL011State, flags),
- vmstate_of!(PL011State, line_control),
- vmstate_of!(PL011State, receive_status_error_clear),
- vmstate_of!(PL011State, control),
- vmstate_of!(PL011State, dmacr),
- vmstate_of!(PL011State, int_enabled),
- vmstate_of!(PL011State, int_level),
- vmstate_of!(PL011State, read_fifo),
- vmstate_of!(PL011State, ilpr),
- vmstate_of!(PL011State, ibrd),
- vmstate_of!(PL011State, fbrd),
- vmstate_of!(PL011State, ifl),
- vmstate_of!(PL011State, read_pos),
- vmstate_of!(PL011State, read_count),
- vmstate_of!(PL011State, read_trigger),
+ vmstate_struct!(PL011State, regs, &VMSTATE_PL011_REGS, PL011Registers),
},
subsections: vmstate_subsections! {
VMSTATE_PL011_CLOCK
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (5 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 06/10] rust: pl011: extract PL011Registers Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-23 5:47 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 08/10] rust: pl011: remove duplicate definitions Paolo Bonzini
` (2 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 08/10] rust: pl011: remove duplicate definitions
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (6 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-23 6:12 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks Paolo Bonzini
2025-01-17 9:26 ` [PATCH 10/10] rust: qdev: make reset take a shared reference Paolo Bonzini
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Unify the "Interrupt" enum and the "INT_*" constants with a struct
that contains the bits. The "int_level" and "int_enabled" fields
could use a crate such as "bitflags".
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 36 ++++++++++++-------------
rust/hw/char/pl011/src/lib.rs | 46 +++++++++++---------------------
2 files changed, 33 insertions(+), 49 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 1d3da59e481..6ecbfb9ac84 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -183,7 +183,7 @@ pub(self) fn read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
self.flags.set_receive_fifo_empty(true);
}
if self.read_count + 1 == self.read_trigger {
- self.int_level &= !registers::INT_RX;
+ self.int_level &= !Interrupt::RX.0;
}
// Update error bits.
self.receive_status_error_clear.set_from_data(c);
@@ -232,7 +232,7 @@ pub(self) fn write(
}
// interrupts always checked
let _ = self.loopback_tx(value);
- self.int_level |= registers::INT_TX;
+ self.int_level |= Interrupt::TX.0;
return true;
}
RSR => {
@@ -356,19 +356,19 @@ fn loopback_mdmctrl(&mut self) -> bool {
// Change interrupts based on updated FR
let mut il = self.int_level;
- il &= !Interrupt::MS;
+ il &= !Interrupt::MS.0;
if self.flags.data_set_ready() {
- il |= Interrupt::DSR as u32;
+ il |= Interrupt::DSR.0;
}
if self.flags.data_carrier_detect() {
- il |= Interrupt::DCD as u32;
+ il |= Interrupt::DCD.0;
}
if self.flags.clear_to_send() {
- il |= Interrupt::CTS as u32;
+ il |= Interrupt::CTS.0;
}
if self.flags.ring_indicator() {
- il |= Interrupt::RI as u32;
+ il |= Interrupt::RI.0;
}
self.int_level = il;
true
@@ -446,7 +446,7 @@ pub fn put_fifo(&mut self, value: u32) -> bool {
}
if self.read_count == self.read_trigger {
- self.int_level |= registers::INT_RX;
+ self.int_level |= Interrupt::RX.0;
return true;
}
false
@@ -622,16 +622,16 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
/// Which bits in the interrupt status matter for each outbound IRQ line ?
const IRQMASK: [u32; 6] = [
/* combined IRQ */
- Interrupt::E
- | Interrupt::MS
- | Interrupt::RT as u32
- | Interrupt::TX as u32
- | Interrupt::RX as u32,
- Interrupt::RX as u32,
- Interrupt::TX as u32,
- Interrupt::RT as u32,
- Interrupt::MS,
- Interrupt::E,
+ Interrupt::E.0
+ | Interrupt::MS.0
+ | Interrupt::RT.0
+ | Interrupt::TX.0
+ | Interrupt::RX.0,
+ Interrupt::RX.0,
+ Interrupt::TX.0,
+ Interrupt::RT.0,
+ Interrupt::MS.0,
+ Interrupt::E.0,
];
/// # Safety
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 2baacba2306..300c732ae1d 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -100,7 +100,6 @@ enum RegisterOffset {
//Reserved = 0x04C,
}
-#[allow(dead_code)]
mod registers {
//! Device registers exposed as typed structs which are backed by arbitrary
//! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
@@ -521,38 +520,23 @@ fn default() -> Self {
}
/// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
- pub const INT_OE: u32 = 1 << 10;
- pub const INT_BE: u32 = 1 << 9;
- pub const INT_PE: u32 = 1 << 8;
- pub const INT_FE: u32 = 1 << 7;
- pub const INT_RT: u32 = 1 << 6;
- pub const INT_TX: u32 = 1 << 5;
- pub const INT_RX: u32 = 1 << 4;
- pub const INT_DSR: u32 = 1 << 3;
- pub const INT_DCD: u32 = 1 << 2;
- pub const INT_CTS: u32 = 1 << 1;
- pub const INT_RI: u32 = 1 << 0;
- pub const INT_E: u32 = INT_OE | INT_BE | INT_PE | INT_FE;
- pub const INT_MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS;
-
- #[repr(u32)]
- pub enum Interrupt {
- OE = 1 << 10,
- BE = 1 << 9,
- PE = 1 << 8,
- FE = 1 << 7,
- RT = 1 << 6,
- TX = 1 << 5,
- RX = 1 << 4,
- DSR = 1 << 3,
- DCD = 1 << 2,
- CTS = 1 << 1,
- RI = 1 << 0,
- }
+ pub struct Interrupt(pub u32);
impl Interrupt {
- pub const E: u32 = INT_OE | INT_BE | INT_PE | INT_FE;
- pub const MS: u32 = INT_RI | INT_DSR | INT_DCD | INT_CTS;
+ pub const OE: Self = Self(1 << 10);
+ pub const BE: Self = Self(1 << 9);
+ pub const PE: Self = Self(1 << 8);
+ pub const FE: Self = Self(1 << 7);
+ pub const RT: Self = Self(1 << 6);
+ pub const TX: Self = Self(1 << 5);
+ pub const RX: Self = Self(1 << 4);
+ pub const DSR: Self = Self(1 << 3);
+ pub const DCD: Self = Self(1 << 2);
+ pub const CTS: Self = Self(1 << 1);
+ pub const RI: Self = Self(1 << 0);
+
+ pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
+ pub const MS: Self = Self(Self::RI.0 | Self::DSR.0 | Self::DCD.0 | Self::CTS.0);
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (7 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 08/10] rust: pl011: remove duplicate definitions Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-23 6:18 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 10/10] rust: qdev: make reset take a shared reference Paolo Bonzini
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
read() can now return a simple u64.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 16 +++++++++-------
rust/hw/char/pl011/src/memory_ops.rs | 23 ++++-------------------
2 files changed, 13 insertions(+), 26 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 6ecbfb9ac84..af0f451deb2 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -171,7 +171,7 @@ impl PL011Registers {
pub(self) fn read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
use RegisterOffset::*;
- std::ops::ControlFlow::Break(match offset {
+ ControlFlow::Break(match offset {
DR => {
self.flags.set_receive_fifo_full(false);
let c = self.read_fifo[self.read_pos];
@@ -530,22 +530,24 @@ fn post_init(&self) {
}
}
- #[allow(clippy::needless_pass_by_ref_mut)]
- pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
+ pub fn read(&mut self, offset: hwaddr, _size: u32) -> u64 {
match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
let device_id = self.get_class().device_id;
- ControlFlow::Break(u64::from(device_id[(offset - 0xfe0) >> 2]))
+ u64::from(device_id[(offset - 0xfe0) >> 2])
}
Err(_) => {
// qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
- ControlFlow::Break(0)
+ 0
}
Ok(field) => match self.regs.borrow_mut().read(field) {
- ControlFlow::Break(value) => ControlFlow::Break(value.into()),
+ ControlFlow::Break(value) => value.into(),
ControlFlow::Continue(value) => {
self.update();
- ControlFlow::Continue(value.into())
+ unsafe {
+ qemu_chr_fe_accept_input(&mut self.char_backend);
+ }
+ value.into()
},
}
}
diff --git a/rust/hw/char/pl011/src/memory_ops.rs b/rust/hw/char/pl011/src/memory_ops.rs
index 8f66c8d492c..95b4df794e4 100644
--- a/rust/hw/char/pl011/src/memory_ops.rs
+++ b/rust/hw/char/pl011/src/memory_ops.rs
@@ -26,26 +26,11 @@
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);
- match val {
- std::ops::ControlFlow::Break(val) => val,
- std::ops::ControlFlow::Continue(val) => {
- // SAFETY: self.char_backend is a valid CharBackend instance after it's been
- // initialized in realize().
- let cb_ptr = unsafe { core::ptr::addr_of_mut!(state.as_mut().char_backend) };
- unsafe {
- qemu_chr_fe_accept_input(cb_ptr);
- }
-
- val
- }
- }
+ unsafe { state.as_mut() }.read(addr, size)
}
unsafe extern "C" fn pl011_write(opaque: *mut c_void, addr: hwaddr, data: u64, _size: c_uint) {
- unsafe {
- assert!(!opaque.is_null());
- let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
- state.as_mut().write(addr, data)
- }
+ assert!(!opaque.is_null());
+ let mut state = unsafe { NonNull::new_unchecked(opaque.cast::<PL011State>()) };
+ unsafe { state.as_mut() }.write(addr, data);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 10/10] rust: qdev: make reset take a shared reference
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
` (8 preceding siblings ...)
2025-01-17 9:26 ` [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks Paolo Bonzini
@ 2025-01-17 9:26 ` Paolo Bonzini
2025-01-23 6:19 ` Zhao Liu
9 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-17 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Because register reset is within a borrow_mut() call, reset
does not need anymore a mut reference to the PL011State.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 4 ++--
rust/qemu-api/src/qdev.rs | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index af0f451deb2..019c1617807 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -164,7 +164,7 @@ fn vmsd() -> Option<&'static VMStateDescription> {
Some(&device_class::VMSTATE_PL011)
}
const REALIZE: Option<fn(&mut Self)> = Some(Self::realize);
- const RESET: Option<fn(&mut Self)> = Some(Self::reset);
+ const RESET: Option<fn(&Self)> = Some(Self::reset);
}
impl PL011Registers {
@@ -604,7 +604,7 @@ pub fn realize(&mut self) {
}
}
- pub fn reset(&mut self) {
+ pub fn reset(&self) {
self.regs.borrow_mut().reset();
}
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 686054e737a..4658409bebb 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -30,7 +30,7 @@ pub trait DeviceImpl {
///
/// Rust does not yet support the three-phase reset protocol; this is
/// usually okay for leaf classes.
- const RESET: Option<fn(&mut Self)> = None;
+ const RESET: Option<fn(&Self)> = None;
/// An array providing the properties that the user can set on the
/// device. Not a `const` because referencing statics in constants
--
2.47.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 01/10] rust: pl011: remove unnecessary "extern crate"
2025-01-17 9:26 ` [PATCH 01/10] rust: pl011: remove unnecessary "extern crate" Paolo Bonzini
@ 2025-01-22 13:37 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 13:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:48AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:48 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 01/10] rust: pl011: remove unnecessary "extern crate"
> X-Mailer: git-send-email 2.47.1
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/lib.rs | 4 ----
> 1 file changed, 4 deletions(-)
Yes, it's unnecessary from Rust 2018.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device
2025-01-17 9:26 ` [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device Paolo Bonzini
@ 2025-01-22 13:39 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 13:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:49AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:49 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from
> outside pl011::device
> X-Mailer: git-send-email 2.47.1
>
> The only public interfaces for pl011 are TYPE_PL011 and pl011_create.
> Remove pub from everything else.
>
> Note: the "allow(dead_code)" is removed later.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 2 +-
> rust/hw/char/pl011/src/device_class.rs | 2 +-
> rust/hw/char/pl011/src/lib.rs | 13 ++++++++-----
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
2025-01-17 9:26 ` [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset Paolo Bonzini
@ 2025-01-22 14:34 ` Zhao Liu
2025-01-22 15:00 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 14:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:50AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:50 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
> X-Mailer: git-send-email 2.47.1
>
> As an added bonus, this also makes the new function return u32 instead
> of u64, thus factoring some casts into a single place.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 114 +++++++++++++++++--------------
> 1 file changed, 63 insertions(+), 51 deletions(-)
[snip]
> - pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
> + fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
> use RegisterOffset::*;
Can we move this "use" to the start of the file?
IMO, placing it in the local scope appears unnecessary and somewhat
fragmented.
> - let value = match RegisterOffset::try_from(offset) {
> - Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
> - let device_id = self.get_class().device_id;
> - u32::from(device_id[(offset - 0xfe0) >> 2])
> - }
> - Err(_) => {
> - // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
> - 0
> - }
> - Ok(DR) => {
> + std::ops::ControlFlow::Break(match offset {
std::ops can be omitted now.
> + DR => {
> self.flags.set_receive_fifo_full(false);
> let c = self.read_fifo[self.read_pos];
> if self.read_count > 0 {
[snip]
> - pub fn write(&mut self, offset: hwaddr, value: u64) {
> + fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
> // eprintln!("write offset {offset} value {value}");
> use RegisterOffset::*;
> - let value: u32 = value as u32;
> - match RegisterOffset::try_from(offset) {
> - Err(_bad_offset) => {
> - eprintln!("write bad offset {offset} value {value}");
> - }
> - Ok(DR) => {
> + match offset {
> + DR => {
> // ??? Check if transmitter is enabled.
> let ch: u8 = value as u8;
> // XXX this blocks entire thread. Rewrite to use
> @@ -290,22 +277,22 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
> self.int_level |= registers::INT_TX;
> self.update();
> }
> - Ok(RSR) => {
> - self.receive_status_error_clear.reset();
> + RSR => {
> + self.receive_status_error_clear = 0.into();
Emm, why do we use 0.into() instead of reset() here? It looks they're
same.
[snip]
> @@ -562,6 +549,31 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
>
> Ok(())
> }
> +
> + pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
Maybe pub(crate)? But both are fine for me :-)
> + match RegisterOffset::try_from(offset) {
> + Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
> + let device_id = self.get_class().device_id;
> + ControlFlow::Break(u64::from(device_id[(offset - 0xfe0) >> 2]))
> + }
> + Err(_) => {
> + // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
> + ControlFlow::Break(0)
> + }
> + Ok(field) => match self.regs_read(field) {
> + ControlFlow::Break(value) => ControlFlow::Break(value.into()),
> + ControlFlow::Continue(value) => ControlFlow::Continue(value.into()),
> + }
> + }
> + }
> +
Look good to me,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function
2025-01-17 9:26 ` [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function Paolo Bonzini
@ 2025-01-22 14:59 ` Zhao Liu
2025-01-22 15:04 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 14:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:51AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:51 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/10] rust: pl011: extract CharBackend receive logic into
> a separate function
> X-Mailer: git-send-email 2.47.1
>
> Prepare for moving all references to the registers and the FIFO into a
> separate struct.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
[snip]
> - pub fn put_fifo(&mut self, value: c_uint) {
> + pub fn put_fifo(&mut self, value: u32) {
> let depth = self.fifo_depth();
> assert!(depth > 0);
> let slot = (self.read_pos + self.read_count) & (depth - 1);
> @@ -615,12 +621,9 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
> unsafe {
> debug_assert!(!opaque.is_null());
> let mut state = NonNull::new_unchecked(opaque.cast::<PL011State>());
> - if state.as_ref().loopback_enabled() {
> - return;
> - }
> if size > 0 {
> debug_assert!(!buf.is_null());
> - state.as_mut().put_fifo(c_uint::from(buf.read_volatile()))
An extra question...here I'm not sure, do we really need read_volatile?
> + state.as_mut().receive(u32::from(buf.read_volatile()));
> }
> }
> }
Patch is fine for me,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
2025-01-22 14:34 ` Zhao Liu
@ 2025-01-22 15:00 ` Paolo Bonzini
2025-01-22 17:00 ` Zhao Liu
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-22 15:00 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 1/22/25 15:34, Zhao Liu wrote:
> On Fri, Jan 17, 2025 at 10:26:50AM +0100, Paolo Bonzini wrote:
>> Date: Fri, 17 Jan 2025 10:26:50 +0100
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Subject: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
>> X-Mailer: git-send-email 2.47.1
>>
>> As an added bonus, this also makes the new function return u32 instead
>> of u64, thus factoring some casts into a single place.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> rust/hw/char/pl011/src/device.rs | 114 +++++++++++++++++--------------
>> 1 file changed, 63 insertions(+), 51 deletions(-)
>
> [snip]
>
>> - pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
>> + fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
>> use RegisterOffset::*;
>
> Can we move this "use" to the start of the file?
I don't think it's a good idea to make the register names visible
globally... "use Enum::*" before a match statement is relatively
common. For example: https://doc.rust-lang.org/src/std/io/error.rs.html#436
>> + std::ops::ControlFlow::Break(match offset {
>
> std::ops can be omitted now.
Done, add added a patch to get rid of ControlFlow completely.
>> - Ok(RSR) => {
>> - self.receive_status_error_clear.reset();
>> + RSR => {
>> + self.receive_status_error_clear = 0.into();
>
> Emm, why do we use 0.into() instead of reset() here? It looks they're
> same.
Fixed.
>> + pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
>
> Maybe pub(crate)? But both are fine for me :-)
The struct is not public outside the crate, so it doesn't make a
difference, does it?
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Thanks, I'll post a quick v2 anyway once you've finished reviewing.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function
2025-01-22 14:59 ` Zhao Liu
@ 2025-01-22 15:04 ` Paolo Bonzini
0 siblings, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-22 15:04 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
On 1/22/25 15:59, Zhao Liu wrote:
>> if size > 0 {
>> debug_assert!(!buf.is_null());
>> - state.as_mut().put_fifo(c_uint::from(buf.read_volatile()))
>
> An extra question...here I'm not sure, do we really need read_volatile?
No, the buffer is not guest visible. It will certainly go away together
with chardev bindings.
Paolo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops
2025-01-22 16:50 ` Zhao Liu
@ 2025-01-22 16:49 ` Paolo Bonzini
0 siblings, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-22 16:49 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 505 bytes --]
Il mer 22 gen 2025, 17:31 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK &&
> !self.loopback_enabled() {
> > - self.put_fifo(registers::Data::BREAK.into());
> > + let update = self.put_fifo(registers::Data::BREAK.into());
>
> We can omit this `update` variable.
>
Indeed, but sometimes I left it because of long lines or to clarify the
meaning of the "bool" (or both).
Paolo
Reviewed-by: Zhao Liu <zhao.liu@intel.com>
[-- Attachment #2: Type: text/html, Size: 1280 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops
2025-01-17 9:26 ` [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops Paolo Bonzini
@ 2025-01-22 16:50 ` Zhao Liu
2025-01-22 16:49 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 16:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:52AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:52 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 05/10] rust: pl011: pull interrupt updates out of
> read/write ops
> X-Mailer: git-send-email 2.47.1
>
> qemu_irqs are not part of the vmstate, therefore they will remain in
> PL011State. Update them if needed after regs_read()/regs_write().
>
> Apply #[must_use] to functions that return whether the interrupt state
> could have changed, so that it's harder to forget the call to update().
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 68 ++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 30 deletions(-)
>
[snip]
>
> pub fn event(&mut self, event: QEMUChrEvent) {
> if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
> - self.put_fifo(registers::Data::BREAK.into());
> + let update = self.put_fifo(registers::Data::BREAK.into());
We can omit this `update` variable.
> + if update {
> + self.update();
> + }
> }
> }
Nice refactoring!
Reviewed-by: Zhao Liu <zhao.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
2025-01-22 15:00 ` Paolo Bonzini
@ 2025-01-22 17:00 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-22 17:00 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> > > - pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
> > > + fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
> > > use RegisterOffset::*;
> >
> > Can we move this "use" to the start of the file?
>
> I don't think it's a good idea to make the register names visible
> globally... "use Enum::*" before a match statement is relatively common.
> For example: https://doc.rust-lang.org/src/std/io/error.rs.html#436
Thanks!
> > > + pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, u64> {
> >
> > Maybe pub(crate)? But both are fine for me :-)
>
> The struct is not public outside the crate, so it doesn't make a difference,
> does it?
You're right.
> > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
> Thanks, I'll post a quick v2 anyway once you've finished reviewing.
>
The remaining critical ones, I'll go through them all tomorrow.
Regerds,
Zhao
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] rust: pl011: extract PL011Registers
2025-01-17 9:26 ` [PATCH 06/10] rust: pl011: extract PL011Registers Paolo Bonzini
@ 2025-01-23 3:44 ` Zhao Liu
2025-01-23 8:07 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 3:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> --- a/rust/hw/char/pl011/src/device_class.rs
> +++ b/rust/hw/char/pl011/src/device_class.rs
> @@ -6,11 +6,11 @@
> use std::os::raw::{c_int, c_void};
>
> use qemu_api::{
> - bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of, vmstate_subsections,
> - vmstate_unused, zeroable::Zeroable,
> + bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of, vmstate_struct,
Oops, I forgot to clean up this bindings::*.
> + vmstate_subsections, vmstate_unused, zeroable::Zeroable,
> };
>
Very good example!
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
2025-01-17 9:26 ` [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell Paolo Bonzini
@ 2025-01-23 5:47 ` Zhao Liu
2025-01-23 8:05 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 5:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] rust: pl011: remove duplicate definitions
2025-01-17 9:26 ` [PATCH 08/10] rust: pl011: remove duplicate definitions Paolo Bonzini
@ 2025-01-23 6:12 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 6:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:55AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 08/10] rust: pl011: remove duplicate definitions
> X-Mailer: git-send-email 2.47.1
>
> Unify the "Interrupt" enum and the "INT_*" constants with a struct
> that contains the bits. The "int_level" and "int_enabled" fields
> could use a crate such as "bitflags".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 36 ++++++++++++-------------
> rust/hw/char/pl011/src/lib.rs | 46 +++++++++++---------------------
> 2 files changed, 33 insertions(+), 49 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks
2025-01-17 9:26 ` [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks Paolo Bonzini
@ 2025-01-23 6:18 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 6:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:56AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:56 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 09/10] rust: pl011: pull device-specific code out of
> MemoryRegionOps callbacks
> X-Mailer: git-send-email 2.47.1
>
> read() can now return a simple u64.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 16 +++++++++-------
> rust/hw/char/pl011/src/memory_ops.rs | 23 ++++-------------------
> 2 files changed, 13 insertions(+), 26 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 10/10] rust: qdev: make reset take a shared reference
2025-01-17 9:26 ` [PATCH 10/10] rust: qdev: make reset take a shared reference Paolo Bonzini
@ 2025-01-23 6:19 ` Zhao Liu
0 siblings, 0 replies; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 6:19 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, Jan 17, 2025 at 10:26:57AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/10] rust: qdev: make reset take a shared reference
> X-Mailer: git-send-email 2.47.1
>
> Because register reset is within a borrow_mut() call, reset
> does not need anymore a mut reference to the PL011State.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 4 ++--
> rust/qemu-api/src/qdev.rs | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
2025-01-23 5:47 ` Zhao Liu
@ 2025-01-23 8:05 ` Paolo Bonzini
2025-01-23 9:24 ` Zhao Liu
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-23 8:05 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 4535 bytes --]
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
[-- Attachment #2: Type: text/html, Size: 7730 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] rust: pl011: extract PL011Registers
2025-01-23 3:44 ` Zhao Liu
@ 2025-01-23 8:07 ` Paolo Bonzini
0 siblings, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-23 8:07 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 822 bytes --]
Il gio 23 gen 2025, 04:25 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > --- a/rust/hw/char/pl011/src/device_class.rs
> > +++ b/rust/hw/char/pl011/src/device_class.rs
> > @@ -6,11 +6,11 @@
> > use std::os::raw::{c_int, c_void};
> >
> > use qemu_api::{
> > - bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of,
> vmstate_subsections,
> > - vmstate_unused, zeroable::Zeroable,
> > + bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_of,
> vmstate_struct,
>
> Oops, I forgot to clean up this bindings::*.
>
No big deal (it's a secondary file and will probably be merged into
device.rs as soon as VMState bindings are more complete).
Paolo
> + vmstate_subsections, vmstate_unused, zeroable::Zeroable,
> > };
> >
>
> Very good example!
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>
>
[-- Attachment #2: Type: text/html, Size: 1852 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
2025-01-23 8:05 ` Paolo Bonzini
@ 2025-01-23 9:24 ` Zhao Liu
2025-01-23 11:39 ` Paolo Bonzini
0 siblings, 1 reply; 29+ messages in thread
From: Zhao Liu @ 2025-01-23 9:24 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
> > > #[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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell
2025-01-23 9:24 ` Zhao Liu
@ 2025-01-23 11:39 ` Paolo Bonzini
0 siblings, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2025-01-23 11:39 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]
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
>
>
>
[-- Attachment #2: Type: text/html, Size: 3891 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2025-01-23 11:39 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-17 9:26 [PATCH 00/10] rust: pl011: correctly use interior mutability Paolo Bonzini
2025-01-17 9:26 ` [PATCH 01/10] rust: pl011: remove unnecessary "extern crate" Paolo Bonzini
2025-01-22 13:37 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 02/10] rust: pl011: hide unnecessarily "pub" items from outside pl011::device Paolo Bonzini
2025-01-22 13:39 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset Paolo Bonzini
2025-01-22 14:34 ` Zhao Liu
2025-01-22 15:00 ` Paolo Bonzini
2025-01-22 17:00 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 04/10] rust: pl011: extract CharBackend receive logic into a separate function Paolo Bonzini
2025-01-22 14:59 ` Zhao Liu
2025-01-22 15:04 ` Paolo Bonzini
2025-01-17 9:26 ` [PATCH 05/10] rust: pl011: pull interrupt updates out of read/write ops Paolo Bonzini
2025-01-22 16:50 ` Zhao Liu
2025-01-22 16:49 ` Paolo Bonzini
2025-01-17 9:26 ` [PATCH 06/10] rust: pl011: extract PL011Registers Paolo Bonzini
2025-01-23 3:44 ` Zhao Liu
2025-01-23 8:07 ` Paolo Bonzini
2025-01-17 9:26 ` [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell Paolo Bonzini
2025-01-23 5:47 ` Zhao Liu
2025-01-23 8:05 ` Paolo Bonzini
2025-01-23 9:24 ` Zhao Liu
2025-01-23 11:39 ` Paolo Bonzini
2025-01-17 9:26 ` [PATCH 08/10] rust: pl011: remove duplicate definitions Paolo Bonzini
2025-01-23 6:12 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 09/10] rust: pl011: pull device-specific code out of MemoryRegionOps callbacks Paolo Bonzini
2025-01-23 6:18 ` Zhao Liu
2025-01-17 9:26 ` [PATCH 10/10] rust: qdev: make reset take a shared reference Paolo Bonzini
2025-01-23 6:19 ` Zhao Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).