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

[Manos Pitsidianakis]

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
>> --
>> γαῖα πυρί μιχθήτω
>>
>>