rust/hw/char/pl011/src/device.rs | 78 ++++++++++++++++++++++++-------- rust/hw/char/pl011/src/lib.rs | 22 ++++++++- 2 files changed, 79 insertions(+), 21 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 v2:
- Group invalid write case matches (Paolo)
- Reduce register getter line count to aid review (Peter Maydell)
v1 -> v2:
- Print error when guest attempts to write to RO registers (Peter
Maydell)
Version 1:
<20241116181549.3430225-1-manos.pitsidianakis@linaro.org>
Version 2:
<20241116221757.3501603-1-manos.pitsidianakis@linaro.org>
Interdiff against v2:
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index fc6f3f394d..f1d959ca28 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -40,54 +40,50 @@ enum DeviceId {
Luminary,
}
+macro_rules! pcell_reg_getter {
+ ($($(#[$attrs:meta])* fn $getter_fn:ident -> $value:literal),*$(,)?) => {
+ $($(#[$attrs])* const fn $getter_fn(self) -> u64 { $value })*
+ };
+}
+
+macro_rules! periph_reg_getter {
+ ($($(#[$attrs:meta])* fn $getter_fn:ident -> { Arm => $arm:literal, Luminary => $lum:literal$(,)?}),*$(,)?) => {
+ $(
+ $(#[$attrs])*
+ const fn $getter_fn(self) -> u64 {
+ (match self {
+ Self::Arm => $arm,
+ Self::Luminary => $lum,
+ }) as u64
+ }
+ )*
+ };
+}
+
impl DeviceId {
/// 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
+ periph_reg_getter! {
+ /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
+ fn uart_periph_id1 -> { Arm => 0x10, Luminary => 0x00 },
+ /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
+ fn uart_periph_id2 -> { Arm => 0x14, Luminary => 0x18 },
+ /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
+ fn uart_periph_id3 -> { Arm => 0x0, Luminary => 0x1 }
}
- /// 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
+ pcell_reg_getter! {
+ /// Value of `UARTPCellID0` register.
+ fn uart_pcell_id0 -> 0x0d,
+ /// Value of `UARTPCellID1` register.
+ fn uart_pcell_id1 -> 0xf0,
+ /// Value of `UARTPCellID2` register.
+ fn uart_pcell_id2 -> 0x05,
+ /// Value of `UARTPCellID3` register.
+ fn uart_pcell_id3 -> 0xb1,
}
}
@@ -282,7 +278,7 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
}
Ok(
dev_id @ (PeriphID0 | PeriphID1 | PeriphID2 | PeriphID3 | PCellID0 | PCellID1
- | PCellID2 | PCellID3),
+ | PCellID2 | PCellID3 | FR | RIS | MIS),
) => {
eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
}
@@ -304,9 +300,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
Ok(RSR) => {
self.receive_status_error_clear = 0.into();
}
- Ok(FR) => {
- eprintln!("write bad offset {offset} at RO register UARTFR value {value}");
- }
Ok(ILPR) => {
self.ilpr = value;
}
@@ -355,12 +348,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.int_enabled = value;
self.update();
}
- 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 | 78 ++++++++++++++++++++++++--------
rust/hw/char/pl011/src/lib.rs | 22 ++++++++-
2 files changed, 79 insertions(+), 21 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2a85960b81..f1d959ca28 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,51 @@ enum DeviceId {
Luminary,
}
-impl std::ops::Index<hwaddr> for DeviceId {
- type Output = c_uchar;
+macro_rules! pcell_reg_getter {
+ ($($(#[$attrs:meta])* fn $getter_fn:ident -> $value:literal),*$(,)?) => {
+ $($(#[$attrs])* const fn $getter_fn(self) -> u64 { $value })*
+ };
+}
- 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],
- }
- }
+macro_rules! periph_reg_getter {
+ ($($(#[$attrs:meta])* fn $getter_fn:ident -> { Arm => $arm:literal, Luminary => $lum:literal$(,)?}),*$(,)?) => {
+ $(
+ $(#[$attrs])*
+ const fn $getter_fn(self) -> u64 {
+ (match self {
+ Self::Arm => $arm,
+ Self::Luminary => $lum,
+ }) as u64
+ }
+ )*
+ };
}
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
+ }
+
+ periph_reg_getter! {
+ /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
+ fn uart_periph_id1 -> { Arm => 0x10, Luminary => 0x00 },
+ /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
+ fn uart_periph_id2 -> { Arm => 0x14, Luminary => 0x18 },
+ /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
+ fn uart_periph_id3 -> { Arm => 0x0, Luminary => 0x1 }
+ }
+
+ pcell_reg_getter! {
+ /// Value of `UARTPCellID0` register.
+ fn uart_pcell_id0 -> 0x0d,
+ /// Value of `UARTPCellID1` register.
+ fn uart_pcell_id1 -> 0xf0,
+ /// Value of `UARTPCellID2` register.
+ fn uart_pcell_id2 -> 0x05,
+ /// Value of `UARTPCellID3` register.
+ fn uart_pcell_id3 -> 0xb1,
+ }
}
#[repr(C)]
@@ -182,9 +214,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 +273,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 | FR | RIS | MIS),
+ ) => {
+ 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;
@@ -257,9 +300,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
Ok(RSR) => {
self.receive_status_error_clear = 0.into();
}
- Ok(FR) => {
- // flag writes are ignored
- }
Ok(ILPR) => {
self.ilpr = value;
}
@@ -308,8 +348,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
self.int_enabled = value;
self.update();
}
- Ok(RIS) => {}
- Ok(MIS) => {}
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
--
γαῖα πυρί μιχθήτω
Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
> 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 v2:
>
> - Group invalid write case matches (Paolo)
> - Reduce register getter line count to aid review (Peter Maydell)
>
<snip>
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 2a85960b81..f1d959ca28 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,51 @@ enum DeviceId {
> Luminary,
> }
>
> -impl std::ops::Index<hwaddr> for DeviceId {
> - type Output = c_uchar;
> +macro_rules! pcell_reg_getter {
> + ($($(#[$attrs:meta])* fn $getter_fn:ident -> $value:literal),*$(,)?) => {
> + $($(#[$attrs])* const fn $getter_fn(self) -> u64 { $value })*
> + };
> +}
>
> - 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],
> - }
> - }
> +macro_rules! periph_reg_getter {
> + ($($(#[$attrs:meta])* fn $getter_fn:ident -> { Arm => $arm:literal, Luminary => $lum:literal$(,)?}),*$(,)?) => {
> + $(
> + $(#[$attrs])*
> + const fn $getter_fn(self) -> u64 {
> + (match self {
> + Self::Arm => $arm,
> + Self::Luminary => $lum,
> + }) as u64
> + }
> + )*
> + };
> }
>
> 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
> + }
> +
> + periph_reg_getter! {
> + /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
> + fn uart_periph_id1 -> { Arm => 0x10, Luminary => 0x00 },
> + /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
> + fn uart_periph_id2 -> { Arm => 0x14, Luminary => 0x18 },
> + /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
> + fn uart_periph_id3 -> { Arm => 0x0, Luminary => 0x1 }
> + }
> +
> + pcell_reg_getter! {
> + /// Value of `UARTPCellID0` register.
> + fn uart_pcell_id0 -> 0x0d,
> + /// Value of `UARTPCellID1` register.
> + fn uart_pcell_id1 -> 0xf0,
> + /// Value of `UARTPCellID2` register.
> + fn uart_pcell_id2 -> 0x05,
> + /// Value of `UARTPCellID3` register.
> + fn uart_pcell_id3 -> 0xb1,
> + }
I share the concern that this is quite a verbose way of handling a
fairly simple set of read-only constants. Is the end result really
folded away to a simple const lookup?
Perhaps this comes down to unfamiliarity with the way macros are working
here but in general macros should be eliding boilerplate to allow us to
concisely represent the relevant data and functionality. Here it adds an
additional indirection when reading the code just to see what is going
on.
> }
>
> #[repr(C)]
> @@ -182,9 +214,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 +273,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 | FR | RIS | MIS),
> + ) => {
This is a nice improvement in conciseness over the separate legs removed bellow.
> + eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
> + }
Is a binding for qemu_log and friends on the todo list?
> Ok(DR) => {
> // ??? Check if transmitter is enabled.
> let ch: u8 = value as u8;
> @@ -257,9 +300,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
> Ok(RSR) => {
> self.receive_status_error_clear = 0.into();
> }
> - Ok(FR) => {
> - // flag writes are ignored
> - }
> Ok(ILPR) => {
> self.ilpr = value;
> }
> @@ -308,8 +348,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
> self.int_enabled = value;
> self.update();
> }
> - Ok(RIS) => {}
> - Ok(MIS) => {}
> 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,
Why do we have specific doc aliases rather than just naming the
registers with the full name?
> ///// 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
On Mon, 18 Nov 2024 13:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
>
>> 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 v2:
>>
>> - Group invalid write case matches (Paolo)
>> - Reduce register getter line count to aid review (Peter Maydell)
>>
><snip>
>> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
>> index 2a85960b81..f1d959ca28 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,51 @@ enum DeviceId {
>> Luminary,
>> }
>>
>> -impl std::ops::Index<hwaddr> for DeviceId {
>> - type Output = c_uchar;
>> +macro_rules! pcell_reg_getter {
>> + ($($(#[$attrs:meta])* fn $getter_fn:ident -> $value:literal),*$(,)?) => {
>> + $($(#[$attrs])* const fn $getter_fn(self) -> u64 { $value })*
>> + };
>> +}
>>
>> - 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],
>> - }
>> - }
>> +macro_rules! periph_reg_getter {
>> + ($($(#[$attrs:meta])* fn $getter_fn:ident -> { Arm => $arm:literal, Luminary => $lum:literal$(,)?}),*$(,)?) => {
>> + $(
>> + $(#[$attrs])*
>> + const fn $getter_fn(self) -> u64 {
>> + (match self {
>> + Self::Arm => $arm,
>> + Self::Luminary => $lum,
>> + }) as u64
>> + }
>> + )*
>> + };
>> }
>>
>> 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
>> + }
>> +
>> + periph_reg_getter! {
>> + /// Value of `UARTPeriphID1` register, which contains the `Designer0` and `PartNumber1` values.
>> + fn uart_periph_id1 -> { Arm => 0x10, Luminary => 0x00 },
>> + /// Value of `UARTPeriphID2` register, which contains the `Revision` and `Designer1` values.
>> + fn uart_periph_id2 -> { Arm => 0x14, Luminary => 0x18 },
>> + /// Value of `UARTPeriphID3` register, which contains the `Configuration` value.
>> + fn uart_periph_id3 -> { Arm => 0x0, Luminary => 0x1 }
>> + }
>> +
>> + pcell_reg_getter! {
>> + /// Value of `UARTPCellID0` register.
>> + fn uart_pcell_id0 -> 0x0d,
>> + /// Value of `UARTPCellID1` register.
>> + fn uart_pcell_id1 -> 0xf0,
>> + /// Value of `UARTPCellID2` register.
>> + fn uart_pcell_id2 -> 0x05,
>> + /// Value of `UARTPCellID3` register.
>> + fn uart_pcell_id3 -> 0xb1,
>> + }
>
>I share the concern that this is quite a verbose way of handling a
>fairly simple set of read-only constants. Is the end result really
>folded away to a simple const lookup?
Yep, const fns are evaluated at compile time. Alternatively we just
could remove the DeviceID struct and inline everything in the read()
method.
>
>Perhaps this comes down to unfamiliarity with the way macros are working
>here but in general macros should be eliding boilerplate to allow us to
>concisely represent the relevant data and functionality. Here it adds an
>additional indirection when reading the code just to see what is going
>on.
Well in the previous patch versions the concern was that it was
"verbose". But even if the registers are available as memory mapped in
the device it's the wrong abstraction to use here; this is a higher
level language than C and using indices is a "when you have a hammer
everything looks like a nail" situation.
We have different programming paradigms in Rust, allowing as to have
documented code (via rustdoc), so either we take advantage of Rust or we
don't.
>
>> }
>>
>> #[repr(C)]
>> @@ -182,9 +214,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 +273,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 | FR | RIS | MIS),
>> + ) => {
>
>This is a nice improvement in conciseness over the separate legs removed bellow.
>
>> + eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
>> + }
>
>Is a binding for qemu_log and friends on the todo list?
I had sent patches on a previous patch series that only some patches of
them were picked up for merging, I plan to send a new revision soon
>
>> Ok(DR) => {
>> // ??? Check if transmitter is enabled.
>> let ch: u8 = value as u8;
>> @@ -257,9 +300,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
>> Ok(RSR) => {
>> self.receive_status_error_clear = 0.into();
>> }
>> - Ok(FR) => {
>> - // flag writes are ignored
>> - }
>> Ok(ILPR) => {
>> self.ilpr = value;
>> }
>> @@ -308,8 +348,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
>> self.int_enabled = value;
>> self.update();
>> }
>> - Ok(RIS) => {}
>> - Ok(MIS) => {}
>> 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,
>
>Why do we have specific doc aliases rather than just naming the
>registers with the full name?
Because prefix enum variants with the same prefix is an anti-pattern in
idiomatic Rust.
>
>> ///// 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
>
>--
>Alex Bennée
>Virtualisation Tech Lead @ Linaro
On Mon, Nov 18, 2024 at 3:46 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> Yep, const fns are evaluated at compile time
This is not correct; "const fn"s *can* be evaluated at compile time.
In this case they won't since "self" is only available at run time.
The match statements persist in the final compiled code.
> >Perhaps this comes down to unfamiliarity with the way macros are working
> >here but in general macros should be eliding boilerplate to allow us to
> >concisely represent the relevant data and functionality. Here it adds an
> >additional indirection when reading the code just to see what is going
> >on.
>
> Well in the previous patch versions the concern was that it was
> "verbose". But even if the registers are available as memory mapped in
> the device it's the wrong abstraction to use here; this is a higher
> level language than C and using indices is a "when you have a hammer
> everything looks like a nail" situation.
>
> We have different programming paradigms in Rust, allowing as to have
> documented code (via rustdoc), so either we take advantage of Rust or we
> don't.
Your usage of macros also is a "when you have a hammer everything
looks like a nail" situation. You're going straight from code that
might be a bit C-like to code that looks like Perl. Rust macros can be
useful to reduce boilerplate, but in this case the macro-based
solution does not make it much easier to see all values at once for
verification purposes, and makes it harder to understand the code's
behavior.
What makes code more "Rusty" would be things like using Result for
error handling, taking advantage of the type system, using traits for
polymorphism, using references to guarantee memory safety. QEMU is
hardly doing any of this, and the rustdoc for qemu_api is also just
starting, so maybe this patch is getting a bit ahead of ourselves?
It's not reasonable to use 50 lines of code just for eight
identification bytes. Even the spec says "The registers can
conceptually be treated as a 32-bit register", which to me suggests
making its contents a u32 const and calling it a day. But if you want
to make it overengineered but sensible, then do this:
const fn getPartNumber(self) -> u12 {0x11}
const fn getDesignerID(self) -> u8 {match ...}
const fn getRevision(self) -> u4 {0x1}
const fn getConfiguration(self) -> u8 {match ...}
and then a getPeriphId(idx: u8) that puts the four together in a u32
and returns ((val32 >> (8 * idx)) & 255) as u8.
For PCellID, the spec itself says the value is 0xB105F00D, so *please*
just hardcode them in the read method. Anything else is a waste of
everybody's time. And since we're in hard freeze, maybe we should
reconsider the one line fix and leave everything for later.
> I had sent patches on a previous patch series that only some patches of
> them were picked up for merging, I plan to send a new revision soon
Cool, thanks.
Paolo
On Mon, Nov 18, 2024 at 12:41 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> This is a nice improvement in conciseness over the separate legs removed bellow.
>
> > + eprintln!("write bad offset {offset} at RO register {dev_id:?} value {value}");
> > + }
>
> Is a binding for qemu_log and friends on the todo list?
Yes, see https://lore.kernel.org/qemu-devel/20241024-rust-round-2-v1-10-051e7a25b978@linaro.org/
and my concerns at
https://lore.kernel.org/qemu-devel/e8e55772-906b-42cb-a744-031e6ae65f16@redhat.com/.
More reviews are welcome; in general, we should define a convention
for Rust functions and constants (is it okay to rename constants in
the Rust bindings if their C name is ugly? should functions keep or
lose the "qemu_*" prefixes?).
The corresponding patch to move from "eprintln!" to the logging
functions is at
https://lore.kernel.org/qemu-devel/20241024-rust-round-2-v1-11-051e7a25b978@linaro.org/.
Paolo
On 11/18/24 04:40, Paolo Bonzini wrote: > More reviews are welcome; in general, we should define a convention > for Rust functions and constants (is it okay to rename constants in > the Rust bindings if their C name is ugly? should functions keep or > lose the "qemu_*" prefixes?). If the C name is ugly, feel free to fix that too. :-) r~
© 2016 - 2024 Red Hat, Inc.