rust/hw/char/pl011/src/device.rs | 79 +++++++++++++++++++++++++------- rust/hw/char/pl011/src/lib.rs | 22 ++++++++- 2 files changed, 83 insertions(+), 18 deletions(-)
DeviceId, which maps the peripheral and PCell registers of a PL011
device, was not treating each register value as a 32 bit value.
Change DeviceId enum to return register values via constified getter
functions instead of leveraging the std::ops::Index<_> trait.
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
rust/hw/char/pl011/src/device.rs | 79 +++++++++++++++++++++++++-------
rust/hw/char/pl011/src/lib.rs | 22 ++++++++-
2 files changed, 83 insertions(+), 18 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2a85960b81..98dd97dbfd 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -5,7 +5,7 @@
use core::ptr::{addr_of, addr_of_mut, NonNull};
use std::{
ffi::CStr,
- os::raw::{c_int, c_uchar, c_uint, c_void},
+ os::raw::{c_int, c_uint, c_void},
};
use qemu_api::{
@@ -32,6 +32,7 @@
/// QEMU sourced constant.
pub const PL011_FIFO_DEPTH: usize = 16_usize;
+/// State enum that represents the values of the peripheral and PCell registers of a PL011 device.
#[derive(Clone, Copy, Debug)]
enum DeviceId {
#[allow(dead_code)]
@@ -39,20 +40,55 @@ enum DeviceId {
Luminary,
}
-impl std::ops::Index<hwaddr> for DeviceId {
- type Output = c_uchar;
-
- fn index(&self, idx: hwaddr) -> &Self::Output {
- match self {
- Self::Arm => &Self::PL011_ID_ARM[idx as usize],
- Self::Luminary => &Self::PL011_ID_LUMINARY[idx as usize],
- }
- }
-}
-
impl DeviceId {
- const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
- const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
+ /// Value of `UARTPeriphID0` register, which contains the `PartNumber0` value.
+ const fn uart_periph_id0(self) -> u64 {
+ 0x11
+ }
+
+ /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
+ const fn uart_periph_id1(self) -> u64 {
+ (match self {
+ Self::Arm => 0x10,
+ Self::Luminary => 0x00,
+ }) as u64
+ }
+
+ /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
+ const fn uart_periph_id2(self) -> u64 {
+ (match self {
+ Self::Arm => 0x14,
+ Self::Luminary => 0x18,
+ }) as u64
+ }
+
+ /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
+ const fn uart_periph_id3(self) -> u64 {
+ (match self {
+ Self::Arm => 0x0,
+ Self::Luminary => 0x1,
+ }) as u64
+ }
+
+ /// Value of `UARTPCellID0` register.
+ const fn uart_pcell_id0(self) -> u64 {
+ 0x0d
+ }
+
+ /// Value of `UARTPCellID1` register.
+ const fn uart_pcell_id1(self) -> u64 {
+ 0xf0
+ }
+
+ /// Value of `UARTPCellID2` register.
+ const fn uart_pcell_id2(self) -> u64 {
+ 0x05
+ }
+
+ /// Value of `UARTPCellID3` register.
+ const fn uart_pcell_id3(self) -> u64 {
+ 0xb1
+ }
}
#[repr(C)]
@@ -182,9 +218,14 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
use RegisterOffset::*;
std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
- Err(v) if (0x3f8..0x400).contains(&v) => {
- u64::from(self.device_id[(offset - 0xfe0) >> 2])
- }
+ Ok(PeriphID0) => self.device_id.uart_periph_id0(),
+ Ok(PeriphID1) => self.device_id.uart_periph_id1(),
+ Ok(PeriphID2) => self.device_id.uart_periph_id2(),
+ Ok(PeriphID3) => self.device_id.uart_periph_id3(),
+ Ok(PCellID0) => self.device_id.uart_pcell_id0(),
+ Ok(PCellID1) => self.device_id.uart_pcell_id1(),
+ Ok(PCellID2) => self.device_id.uart_pcell_id2(),
+ Ok(PCellID3) => self.device_id.uart_pcell_id3(),
Err(_) => {
// qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
0
@@ -236,6 +277,10 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
use RegisterOffset::*;
let value: u32 = value as u32;
match RegisterOffset::try_from(offset) {
+ Ok(PeriphID0) | Ok(PeriphID1) | Ok(PeriphID2) | Ok(PeriphID3) | Ok(PCellID0)
+ | Ok(PCellID1) | Ok(PCellID2) | Ok(PCellID3) => {
+ // Ignore writes to read-only registers.
+ }
Err(_bad_offset) => {
eprintln!("write bad offset {offset} value {value}");
}
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index cd0a49acb9..1f305aa13f 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -111,6 +111,22 @@ pub enum RegisterOffset {
/// DMA control Register
#[doc(alias = "UARTDMACR")]
DMACR = 0x048,
+ #[doc(alias = "UARTPeriphID0")]
+ PeriphID0 = 0xFE0,
+ #[doc(alias = "UARTPeriphID1")]
+ PeriphID1 = 0xFE4,
+ #[doc(alias = "UARTPeriphID2")]
+ PeriphID2 = 0xFE8,
+ #[doc(alias = "UARTPeriphID3")]
+ PeriphID3 = 0xFEC,
+ #[doc(alias = "UARTPCellID0")]
+ PCellID0 = 0xFF0,
+ #[doc(alias = "UARTPCellID1")]
+ PCellID1 = 0xFF4,
+ #[doc(alias = "UARTPCellID2")]
+ PCellID2 = 0xFF8,
+ #[doc(alias = "UARTPCellID3")]
+ PCellID3 = 0xFFC,
///// Reserved, offsets `0x04C` to `0x07C`.
//Reserved = 0x04C,
}
@@ -137,7 +153,11 @@ const fn _assert_exhaustive(val: RegisterOffset) {
}
}
}
- case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR }
+ case! {
+ DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR,
+ PeriphID0, PeriphID1, PeriphID2, PeriphID3,
+ PCellID0, PCellID1, PCellID2, PCellID3,
+ }
}
}
base-commit: 43f2def68476697deb0d119cbae51b20019c6c86
--
γαῖα πυρί μιχθήτω
On 11/16/24 19:15, Manos Pitsidianakis wrote:
> impl DeviceId {
> - const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
> - const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
> + /// Value of `UARTPeriphID0` register, which contains the `PartNumber0` value.
> + const fn uart_periph_id0(self) -> u64 {
> + 0x11
> + }
> +
> + /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
> + const fn uart_periph_id1(self) -> u64 {
> + (match self {
> + Self::Arm => 0x10,
> + Self::Luminary => 0x00,
> + }) as u64
> + }
>
> +
> + /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
> + const fn uart_periph_id2(self) -> u64 {
> + (match self {
> + Self::Arm => 0x14,
> + Self::Luminary => 0x18,
> + }) as u64
> + }
> +
> + /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
> + const fn uart_periph_id3(self) -> u64 {
> + (match self {
> + Self::Arm => 0x0,
> + Self::Luminary => 0x1,
> + }) as u64
> + }
If I understand correctly, these are two reasons why you did not go with
the simple adjustment to the Err(v) pattern:
* the separate registers match the datasheet more
* given the bug that you are fixing in the write function, you want to
avoid duplicating "Err(v) if (0xfe0..=0xffc).contains(&v)" between read
and write; instead, you rely on exhaustive enums for error checking.
I wonder if we can keep these improvements while making the
implementation a bit more concise... The eight const getter functions
are quite verbose, and having the device type match inside each function
is repetitive and hard to verify. Maybe something like
const UART_PCELL_ID: [u8; 4] = [0x0d, 0xf0, 0x05, 0xb1];
const ARM_UART_PERIPH_ID: [u8; 4] = [0x11, 0x10, 0x14, 0x00];
const LUMINARY_UART_PERIPH_ID: [u8; 4] = [0x11, 0x00, 0x18, 0x01];
/// Value of `UARTPeriphID0` through `UARTPeriphID3` registers
const fn uart_periph_id(&self, idx: usize) -> u8 {
match self {
Self::Arm => ARM_UART_PERIPH_ID,
Self::Luminary => LUMINARY_UART_PERIPH_ID,
}[idx]
}
/// Value of `UARTPCellID0` through `UARTPCellID3` registers
const fn uart_pcell_id(idx: usize) -> u8 {
Self::UART_PCELL_ID[idx]
}
could be the best of both worlds?
> match RegisterOffset::try_from(offset) {
> + Ok(PeriphID0) | Ok(PeriphID1) | Ok(PeriphID2) | Ok(PeriphID3) | Ok(PCellID0)
> + | Ok(PCellID1) | Ok(PCellID2) | Ok(PCellID3) => {
> + // Ignore writes to read-only registers.
> + }
This is indeed an improvement over the patches that Junjie and I had
sent, because the writes would have gone down the eprintln! path.
Thanks,
Paolo
> Err(_bad_offset) => {
> eprintln!("write bad offset {offset} value {value}");
> }
> diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
> index cd0a49acb9..1f305aa13f 100644
> --- a/rust/hw/char/pl011/src/lib.rs
> +++ b/rust/hw/char/pl011/src/lib.rs
> @@ -111,6 +111,22 @@ pub enum RegisterOffset {
> /// DMA control Register
> #[doc(alias = "UARTDMACR")]
> DMACR = 0x048,
> + #[doc(alias = "UARTPeriphID0")]
> + PeriphID0 = 0xFE0,
> + #[doc(alias = "UARTPeriphID1")]
> + PeriphID1 = 0xFE4,
> + #[doc(alias = "UARTPeriphID2")]
> + PeriphID2 = 0xFE8,
> + #[doc(alias = "UARTPeriphID3")]
> + PeriphID3 = 0xFEC,
> + #[doc(alias = "UARTPCellID0")]
> + PCellID0 = 0xFF0,
> + #[doc(alias = "UARTPCellID1")]
> + PCellID1 = 0xFF4,
> + #[doc(alias = "UARTPCellID2")]
> + PCellID2 = 0xFF8,
> + #[doc(alias = "UARTPCellID3")]
> + PCellID3 = 0xFFC,
> ///// Reserved, offsets `0x04C` to `0x07C`.
> //Reserved = 0x04C,
> }
> @@ -137,7 +153,11 @@ const fn _assert_exhaustive(val: RegisterOffset) {
> }
> }
> }
> - case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR }
> + case! {
> + DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR,
> + PeriphID0, PeriphID1, PeriphID2, PeriphID3,
> + PCellID0, PCellID1, PCellID2, PCellID3,
> + }
> }
> }
>
>
> base-commit: 43f2def68476697deb0d119cbae51b20019c6c86
On Sat, Nov 16, 2024 at 10:24 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/16/24 19:15, Manos Pitsidianakis wrote:
> > impl DeviceId {
> > - const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
> > - const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
> > + /// Value of `UARTPeriphID0` register, which contains the `PartNumber0` value.
> > + const fn uart_periph_id0(self) -> u64 {
> > + 0x11
> > + }
> > +
> > + /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
> > + const fn uart_periph_id1(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x10,
> > + Self::Luminary => 0x00,
> > + }) as u64
> > + }
> >
> > +
> > + /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
> > + const fn uart_periph_id2(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x14,
> > + Self::Luminary => 0x18,
> > + }) as u64
> > + }
> > +
> > + /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
> > + const fn uart_periph_id3(self) -> u64 {
> > + (match self {
> > + Self::Arm => 0x0,
> > + Self::Luminary => 0x1,
> > + }) as u64
> > + }
>
> If I understand correctly, these are two reasons why you did not go with
> the simple adjustment to the Err(v) pattern:
>
> * the separate registers match the datasheet more
>
> * given the bug that you are fixing in the write function, you want to
> avoid duplicating "Err(v) if (0xfe0..=0xffc).contains(&v)" between read
> and write; instead, you rely on exhaustive enums for error checking.
>
> I wonder if we can keep these improvements while making the
> implementation a bit more concise... The eight const getter functions
> are quite verbose, and having the device type match inside each function
> is repetitive and hard to verify. Maybe something like
>
> const UART_PCELL_ID: [u8; 4] = [0x0d, 0xf0, 0x05, 0xb1];
> const ARM_UART_PERIPH_ID: [u8; 4] = [0x11, 0x10, 0x14, 0x00];
> const LUMINARY_UART_PERIPH_ID: [u8; 4] = [0x11, 0x00, 0x18, 0x01];
>
> /// Value of `UARTPeriphID0` through `UARTPeriphID3` registers
> const fn uart_periph_id(&self, idx: usize) -> u8 {
> match self {
> Self::Arm => ARM_UART_PERIPH_ID,
> Self::Luminary => LUMINARY_UART_PERIPH_ID,
> }[idx]
> }
>
> /// Value of `UARTPCellID0` through `UARTPCellID3` registers
> const fn uart_pcell_id(idx: usize) -> u8 {
> Self::UART_PCELL_ID[idx]
> }
>
> could be the best of both worlds?
Eh, there's no real reason to do that though. I prefer verbosity and
static checking with symbols rather than indexing; we're not writing C
here.
>
> > match RegisterOffset::try_from(offset) {
> > + Ok(PeriphID0) | Ok(PeriphID1) | Ok(PeriphID2) | Ok(PeriphID3) | Ok(PCellID0)
> > + | Ok(PCellID1) | Ok(PCellID2) | Ok(PCellID3) => {
> > + // Ignore writes to read-only registers.
> > + }
>
> This is indeed an improvement over the patches that Junjie and I had
> sent, because the writes would have gone down the eprintln! path.
I will send a v2 to print them anyway as guest errors like Peter requested.
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
On Sat, 16 Nov 2024 at 20:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/16/24 19:15, Manos Pitsidianakis wrote:
> > match RegisterOffset::try_from(offset) {
> > + Ok(PeriphID0) | Ok(PeriphID1) | Ok(PeriphID2) | Ok(PeriphID3) | Ok(PCellID0)
> > + | Ok(PCellID1) | Ok(PCellID2) | Ok(PCellID3) => {
> > + // Ignore writes to read-only registers.
> > + }
>
> This is indeed an improvement over the patches that Junjie and I had
> sent, because the writes would have gone down the eprintln! path.
In our C implementations of devices we usually let writes
to read-only registers end up in the same default case
as writes to invalid offsets, because they're all
guest errors that we'd like to log. That's what the C pl011
model does.
-- PMM
© 2016 - 2024 Red Hat, Inc.