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

["Alex Bennée" <alex.bennee@linaro.org>]

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