Re: [RFC PATCH 5/6] rust: pl011: switch from bilge to bitfield-struct

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9016 bytes --]

Il mer 21 mag 2025, 13:12 Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> ha scritto:

> On Wed, May 21, 2025 at 12:50 PM Alex
> > > -    _reserved_unpredictable: u4,
> > > +    #[bits(4)]
> > > +    _reserved_unpredictable: u8,
> >
> > This does come off as a little janky - effectively casting the u8 to
> > only cover 4 bits. Is this not something we can derive from the type? I
> > see lower down...
>
> Also, I wonder, does bitfield_struct also use 1 bit to represent bool?
>

Yes.

Also, to answer Alex, note that u4 is not a standard Rust type, it comes
from the arbitrary-int crate.

Paolo

>
> >
> > > +}
> > > +
> > > +impl Errors {
> > > +    pub const BREAK: Self = Errors::new().with_break_error(true);
> > >  }
> > >
> > >  /// Data Register, `UARTDR`
> > > @@ -93,19 +101,18 @@ pub struct Errors {
> > >  /// The `UARTDR` register is the data register; write for TX and
> > >  /// read for RX. It is a 12-bit register, where bits 7..0 are the
> > >  /// character and bits 11..8 are error bits.
> > > -#[bitsize(32)]
> > > -#[derive(Clone, Copy, Default, DebugBits, FromBits)]
> > > +#[bitfield(u32)]
> > >  #[doc(alias = "UARTDR")]
> > >  pub struct Data {
> > >      pub data: u8,
> > > +    #[bits(8)]
> > >      pub errors: Errors,
> >
> > We should be able to derive that Errors fits into 8 bits as defined
> above.
> >
> > >      _reserved: u16,
> > >  }
> > > -impl_vmstate_bitsized!(Data);
> > > +impl_vmstate_forward!(Data);
> > >
> > >  impl Data {
> > > -    // bilge is not very const-friendly, unfortunately
> > > -    pub const BREAK: Self = Self { value: 1 << 10 };
> > > +    pub const BREAK: Self = Self::new().with_errors(Errors::BREAK);
> > >  }
> >
> > I guess this flys a little over my head, is the effect only seen in the
> > generated code?
>
> Because these functions are const, they can be evaluated at compile
> time, so this would be replaced with a constant value when compiled.
>
> >
> > >
> > >  /// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
> > > @@ -119,13 +126,14 @@ impl Data {
> > >  /// and UARTECR for writes, but really it's a single error status
> > >  /// register where writing anything to the register clears the error
> > >  /// bits.
> > > -#[bitsize(32)]
> > > -#[derive(Clone, Copy, DebugBits, FromBits)]
> > > +#[bitfield(u32)]
> > >  pub struct ReceiveStatusErrorClear {
> > > +    #[bits(8)]
> > >      pub errors: Errors,
> > > -    _reserved_unpredictable: u24,
> > > +    #[bits(24)]
> > > +    _reserved_unpredictable: u32,
> > >  }
> > > -impl_vmstate_bitsized!(ReceiveStatusErrorClear);
> > > +impl_vmstate_forward!(ReceiveStatusErrorClear);
> > >
> > >  impl ReceiveStatusErrorClear {
> > >      pub fn set_from_data(&mut self, data: Data) {
> > > @@ -138,14 +146,7 @@ pub fn reset(&mut self) {
> > >      }
> > >  }
> > >
> > > -impl Default for ReceiveStatusErrorClear {
> > > -    fn default() -> Self {
> > > -        0.into()
> > > -    }
> > > -}
> > > -
> > > -#[bitsize(32)]
> > > -#[derive(Clone, Copy, DebugBits, FromBits)]
> > > +#[bitfield(u32, default = false)]
> > >  /// Flag Register, `UARTFR`
> > >  ///
> > >  /// This has the usual inbound RS232 modem-control signals, plus flags
> > > @@ -171,9 +172,10 @@ pub struct Flags {
> > >      pub transmit_fifo_empty: bool,
> > >      /// RI: Ring indicator
> > >      pub ring_indicator: bool,
> > > -    _reserved_zero_no_modify: u23,
> > > +    #[bits(23)]
> > > +    _reserved_zero_no_modify: u32,
> > >  }
> > > -impl_vmstate_bitsized!(Flags);
> > > +impl_vmstate_forward!(Flags);
> > >
> > >  impl Flags {
> > >      pub fn reset(&mut self) {
> > > @@ -183,16 +185,14 @@ pub fn reset(&mut self) {
> > >
> > >  impl Default for Flags {
> > >      fn default() -> Self {
> > > -        let mut ret: Self = 0.into();
> > >          // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE
> are 1
> > > -        ret.set_receive_fifo_empty(true);
> > > -        ret.set_transmit_fifo_empty(true);
> > > -        ret
> > > +        Self::from(0)
> > > +            .with_receive_fifo_empty(true)
> > > +            .with_transmit_fifo_empty(true)
> >
> > I guess skipping the mut is the advantage of being able to const eval.
>
> No you can actually have mut in const-eval. What you can't have is
> heap allocations and calling non-const functions, and some other
> things. But in this case it doesn't matter, because this is the
> Default trait and it's not const, because it's a trait method.
>
> >
> > >      }
> > >  }
> > >
> > > -#[bitsize(32)]
> > > -#[derive(Clone, Copy, DebugBits, FromBits)]
> > > +#[bitfield(u32)]
> > >  /// Line Control Register, `UARTLCR_H`
> > >  #[doc(alias = "UARTLCR_H")]
> > >  pub struct LineControl {
> > > @@ -201,48 +201,46 @@ pub struct LineControl {
> > >      /// PEN: Parity enable
> > >      pub parity_enabled: bool,
> > >      /// EPS: Even parity select
> > > +    #[bits(1)]
> > >      pub parity: Parity,
> > >      /// STP2: Two stop bits select
> > >      pub two_stops_bits: bool,
> > >      /// FEN: Enable FIFOs
> > > +    #[bits(1)]
> > >      pub fifos_enabled: Mode,
> > >      /// WLEN: Word length in bits
> > >      /// b11 = 8 bits
> > >      /// b10 = 7 bits
> > >      /// b01 = 6 bits
> > >      /// b00 = 5 bits.
> > > +    #[bits(2)]
> > >      pub word_length: WordLength,
> > >      /// SPS Stick parity select
> > >      pub sticky_parity: bool,
> > >      /// 31:8 - Reserved, do not modify, read as zero.
> > > -    _reserved_zero_no_modify: u24,
> > > +    #[bits(24)]
> > > +    _reserved_zero_no_modify: u32,
> > >  }
> > > -impl_vmstate_bitsized!(LineControl);
> > > +impl_vmstate_forward!(LineControl);
> > >
> > >  impl LineControl {
> > >      pub fn reset(&mut self) {
> > >          // All the bits are cleared to 0 when reset.
> > > -        *self = 0.into();
> > > +        *self = Self::default();
> > >      }
> > >  }
> > >
> > > -impl Default for LineControl {
> > > -    fn default() -> Self {
> > > -        0.into()
> > > -    }
> > > -}
> > > -
> > > -#[bitsize(1)]
> > > -#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
> > >  /// `EPS` "Even parity select", field of [Line Control
> > >  /// register](LineControl).
> > > +#[repr(u8)]
> > > +#[derive(Clone, Copy, Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
> > >  pub enum Parity {
> > >      Odd = 0,
> > >      Even = 1,
> > >  }
> > >
> > > -#[bitsize(1)]
> > > -#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
> > > +#[repr(u8)]
> > > +#[derive(Clone, Copy, Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
> > >  /// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
> > >  /// register](LineControl).
> > >  pub enum Mode {
> > > @@ -253,8 +251,8 @@ pub enum Mode {
> > >      FIFO = 1,
> > >  }
> > >
> > > -#[bitsize(2)]
> > > -#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
> > > +#[repr(u8)]
> > > +#[derive(Clone, Copy, Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
> > >  /// `WLEN` Word length, field of [Line Control register](LineControl).
> > >  ///
> > >  /// These bits indicate the number of data bits transmitted or
> received in a
> > > @@ -275,9 +273,8 @@ pub enum WordLength {
> > >  /// The `UARTCR` register is the control register. It contains various
> > >  /// enable bits, and the bits to write to set the usual outbound RS232
> > >  /// modem control signals. All bits reset to 0 except TXE and RXE.
> > > -#[bitsize(32)]
> > > +#[bitfield(u32, default = false)]
> > >  #[doc(alias = "UARTCR")]
> > > -#[derive(Clone, Copy, DebugBits, FromBits)]
> > >  pub struct Control {
> > >      /// `UARTEN` UART enable: 0 = UART is disabled.
> > >      pub enable_uart: bool,
> > > @@ -285,9 +282,10 @@ pub struct Control {
> > >      /// QEMU does not model this.
> > >      pub enable_sir: bool,
> > >      /// `SIRLP` SIR low-power IrDA mode. QEMU does not model this.
> > > -    pub sir_lowpower_irda_mode: u1,
> > > +    pub sir_lowpower_irda_mode: bool,
> > >      /// Reserved, do not modify, read as zero.
> > > -    _reserved_zero_no_modify: u4,
> > > +    #[bits(4)]
> > > +    _reserved_zero_no_modify: u8,
> > >      /// `LBE` Loopback enable: feed UART output back to the input
> > >      pub enable_loopback: bool,
> > >      /// `TXE` Transmit enable
> > <snip>
> >
> > I guess I'm not seeing a massive difference here. I guess the const eval
> > is nice but there is cognitive dissonance having annotations not match
> > types. It would be nice to have the best of both worlds.
> >
> > For now I don't see a compelling reason to change from a standard crate
> > (which I guess is the reason this is an RFC ;-)
> >
> > --
> > Alex Bennée
> > Virtualisation Tech Lead @ Linaro
>
> --
> Manos Pitsidianakis
> Emulation and Virtualization Engineer at Linaro Ltd
>
>

[-- Attachment #2: Type: text/html, Size: 11846 bytes --]