Re: [PATCH v2] rust/pl011: Fix DeviceID reads

[Peter Maydell]

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