rust/hw/char/pl011/src/device.rs | 93 ++++++++++++++++++++++++-------- rust/hw/char/pl011/src/lib.rs | 22 +++++++- 2 files changed, 93 insertions(+), 22 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.
While at it, print errors when guest attempts to write to other RO
registers as well.
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Notes:
Changes from v1:
- Print error when guest attempts to write to RO registers (Peter
Maydell)
Version 1:
<20241116181549.3430225-1-manos.pitsidianakis@linaro.org/raw>
Interdiff against v1:
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 98dd97dbfd..fc6f3f394d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -277,13 +277,15 @@ 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) => {
+ Err(_) => {
eprintln!("write bad offset {offset} value {value}");
}
+ Ok(
+ dev_id @ (PeriphID0 | PeriphID1 | PeriphID2 | PeriphID3 | PCellID0 | PCellID1
+ | PCellID2 | PCellID3),
+ ) => {
+ eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
+ }
Ok(DR) => {
// ??? Check if transmitter is enabled.
let ch: u8 = value as u8;
@@ -303,7 +305,7 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.receive_status_error_clear = 0.into();
}
Ok(FR) => {
- // flag writes are ignored
+ eprintln!("write bad offset {offset} at RO register UARTFR value {value}");
}
Ok(ILPR) => {
self.ilpr = value;
@@ -353,8 +355,12 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.int_enabled = value;
self.update();
}
- Ok(RIS) => {}
- Ok(MIS) => {}
+ Ok(RIS) => {
+ eprintln!("write bad offset {offset} at RO register UARTRIS value {value}");
+ }
+ Ok(MIS) => {
+ eprintln!("write bad offset {offset} at RO register UARTMIS value {value}");
+ }
Ok(ICR) => {
self.int_level &= !value;
self.update();
rust/hw/char/pl011/src/device.rs | 93 ++++++++++++++++++++++++--------
rust/hw/char/pl011/src/lib.rs | 22 +++++++-
2 files changed, 93 insertions(+), 22 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2a85960b81..fc6f3f394d 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,9 +277,15 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
use RegisterOffset::*;
let value: u32 = value as u32;
match RegisterOffset::try_from(offset) {
- Err(_bad_offset) => {
+ Err(_) => {
eprintln!("write bad offset {offset} value {value}");
}
+ Ok(
+ dev_id @ (PeriphID0 | PeriphID1 | PeriphID2 | PeriphID3 | PCellID0 | PCellID1
+ | PCellID2 | PCellID3),
+ ) => {
+ eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
+ }
Ok(DR) => {
// ??? Check if transmitter is enabled.
let ch: u8 = value as u8;
@@ -258,7 +305,7 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.receive_status_error_clear = 0.into();
}
Ok(FR) => {
- // flag writes are ignored
+ eprintln!("write bad offset {offset} at RO register UARTFR value {value}");
}
Ok(ILPR) => {
self.ilpr = value;
@@ -308,8 +355,12 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.int_enabled = value;
self.update();
}
- Ok(RIS) => {}
- Ok(MIS) => {}
+ Ok(RIS) => {
+ eprintln!("write bad offset {offset} at RO register UARTRIS value {value}");
+ }
+ Ok(MIS) => {
+ eprintln!("write bad offset {offset} at RO register UARTMIS value {value}");
+ }
Ok(ICR) => {
self.int_level &= !value;
self.update();
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, 16 Nov 2024 at 22:18, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> 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.
>
> While at it, print errors when guest attempts to write to other RO
> registers as well.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> rust/hw/char/pl011/src/device.rs | 93 ++++++++++++++++++++++++--------
> rust/hw/char/pl011/src/lib.rs | 22 +++++++-
> 2 files changed, 93 insertions(+), 22 deletions(-)
>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 2a85960b81..fc6f3f394d 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
> + }
This seems extremely verbose and rather obscures the fact that these
registers are a set of adjacent simple ID registers, compared to
the previous code which defined them as an array of values.
Isn't there some way to write this that doesn't need so much code to do it?
All the PLxxx devices and various others have this set of standard
ID registers, because it's part of a standardized scheme, so we're
going to end up with similar code in multiple device models. I would
hope that Rust offers us ways to avoid having boilerplate code
where you need to write dozens of lines to express this.
thanks
-- PMM
On Sun, Nov 17, 2024, 12:14 PM Peter Maydell <peter.maydell@linaro.org>
wrote:
> On Sat, 16 Nov 2024 at 22:18, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > 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.
> >
> > While at it, print errors when guest attempts to write to other RO
> > registers as well.
> >
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>
>
>
> > rust/hw/char/pl011/src/device.rs | 93 ++++++++++++++++++++++++--------
> > rust/hw/char/pl011/src/lib.rs | 22 +++++++-
> > 2 files changed, 93 insertions(+), 22 deletions(-)
> >
> > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/
> device.rs
> > index 2a85960b81..fc6f3f394d 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
> > + }
>
> This seems extremely verbose and rather obscures the fact that these
> registers are a set of adjacent simple ID registers, compared to
> the previous code which defined them as an array of values.
>
> Isn't there some way to write this that doesn't need so much code to do it?
> All the PLxxx devices and various others have this set of standard
> ID registers, because it's part of a standardized scheme, so we're
> going to end up with similar code in multiple device models. I would
> hope that Rust offers us ways to avoid having boilerplate code
> where you need to write dozens of lines to express this.
>
> thanks
> -- PMM
>
One could abstract them with declarative macros to reduce the line count.
WDYT about that approach?
>
Il dom 17 nov 2024, 11:21 Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> ha scritto:
> This seems extremely verbose and rather obscures the fact that these
>> registers are a set of adjacent simple ID registers, compared to
>> the previous code which defined them as an array of values.
>
>
> One could abstract them with declarative macros to reduce the line count.
> WDYT about that approach?
>
No, this is just overcomplicating things. Like Peter said, just use arrays.
Copying from the V1 review for reference:
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]
}
Paolo
>
On Sun, Nov 17, 2024 at 1:16 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il dom 17 nov 2024, 11:21 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> ha scritto:
>>>
>>> This seems extremely verbose and rather obscures the fact that these
>>> registers are a set of adjacent simple ID registers, compared to
>>> the previous code which defined them as an array of values.
>>
>>
>> One could abstract them with declarative macros to reduce the line count. WDYT about that approach?
>
>
> No, this is just overcomplicating things. Like Peter said, just use arrays. Copying from the V1 review for reference:
>
> 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]
> }
>
> Paolo
Nope, it's not overcomplicating things. `idx` is usize where we have
just 4 registers.
I will submit a new version
Il sab 16 nov 2024, 23:18 Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> ha scritto:
> - 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
> + }
> }
>
In your reply to V1 you wrote:
> 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.
I don't see what C has to do with it. Of the three extant options for
DeviceId, you can write them in both C and Rust and they would look pretty
much the same.
I explained the real reason: it's much harder to verify/review the
correctness of independent functions instead of two arrays of four
elements, because consecutive elements are four-five lines apart. There is
also a lot more repetition in writing four match expressions instead of one.
Given Peter's remark on rejecting writes, personally I see no reason to
switch away from Index; but I understand that you felt it was an important
change, so I am trying very hard to find a middle ground that is more
readable than both the old code and your proposal here, and combines the
advantages of both. Please try to listen.
match RegisterOffset::try_from(offset) {
> - Err(_bad_offset) => {
> + Err(_) => {
> eprintln!("write bad offset {offset} value {value}");
> }
> + Ok(
> + dev_id @ (PeriphID0 | PeriphID1 | PeriphID2 | PeriphID3 |
> PCellID0 | PCellID1
> + | PCellID2 | PCellID3),
> + ) => {
> + eprintln!("write bad offset {offset} at RO register
> {dev_id:?} value {value}");
> + }
> Ok(FR) => {
> - // flag writes are ignored
> + eprintln!("write bad offset {offset} at RO register
> UARTFR value {value}");
> }
> - Ok(RIS) => {}
> - Ok(MIS) => {}
> + Ok(RIS) => {
> + eprintln!("write bad offset {offset} at RO register
> UARTRIS value {value}");
> + }
> + Ok(MIS) => {
> + eprintln!("write bad offset {offset} at RO register
> UARTMIS value {value}");
> + }
>
Please use a single "dev_id @ (...)" pattern instead of duplicating code.
Paolo
Ok(ICR) => {
> self.int_level &= !value;
> self.update();
> 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 Sun, Nov 17, 2024 at 9:40 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il sab 16 nov 2024, 23:18 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> ha scritto:
>>
>> - 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
>> + }
>> }
>
>
> In your reply to V1 you wrote:
>
> > 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.
>
> I don't see what C has to do with it. Of the three extant options for DeviceId, you can write them in both C and Rust and they would look pretty much the same.
>
> I explained the real reason: it's much harder to verify/review the correctness of independent functions instead of two arrays of four elements, because consecutive elements are four-five lines apart. There is also a lot more repetition in writing four match expressions instead of one.
>
> Given Peter's remark on rejecting writes, personally I see no reason to switch away from Index; but I understand that you felt it was an important change, so I am trying very hard to find a middle ground that is more readable than both the old code and your proposal here, and combines the advantages of both. Please try to listen.
I very much prefer it this way; the only reason it was Index before
was because the DEVICE_ID arrays were lifted verbatim from C code. I
disagree that these are reasonable requests for code change, sorry.
>
>> match RegisterOffset::try_from(offset) {
>> - Err(_bad_offset) => {
>> + Err(_) => {
>> eprintln!("write bad offset {offset} value {value}");
>> }
>> + Ok(
>> + dev_id @ (PeriphID0 | PeriphID1 | PeriphID2 | PeriphID3 | PCellID0 | PCellID1
>> + | PCellID2 | PCellID3),
>> + ) => {
>> + eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
>> + }
>> Ok(FR) => {
>> - // flag writes are ignored
>> + eprintln!("write bad offset {offset} at RO register UARTFR value {value}");
>> }
>> - Ok(RIS) => {}
>> - Ok(MIS) => {}
>> + Ok(RIS) => {
>> + eprintln!("write bad offset {offset} at RO register UARTRIS value {value}");
>> + }
>> + Ok(MIS) => {
>> + eprintln!("write bad offset {offset} at RO register UARTMIS value {value}");
>> + }
>
>
> Please use a single "dev_id @ (...)" pattern instead of duplicating code.
>
> Paolo
>
>> Ok(ICR) => {
>> self.int_level &= !value;
>> self.update();
>> 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
>> --
>> γαῖα πυρί μιχθήτω
>>
>>
© 2016 - 2024 Red Hat, Inc.