diff mbox series

[v1] rust/pl011: Fix DeviceID reads

Message ID 20241116181549.3430225-1-manos.pitsidianakis@linaro.org (mailing list archive)
State New
Headers show
Series [v1] rust/pl011: Fix DeviceID reads | expand

Commit Message

Manos Pitsidianakis Nov. 16, 2024, 6:15 p.m. UTC 
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(-)


base-commit: 43f2def68476697deb0d119cbae51b20019c6c86

Comments

Paolo Bonzini Nov. 16, 2024, 8:24 p.m. UTC | #1 
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
Peter Maydell Nov. 16, 2024, 9:21 p.m. UTC | #2 
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
Manos Pitsidianakis Nov. 16, 2024, 9:49 p.m. UTC | #3 
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.
diff mbox series

Patch

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,
+        }
     }
 }