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.1On 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>
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
> > > - 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
© 2016 - 2026 Red Hat, Inc.