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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Wed, May 21, 2025 at 12:50 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > The bilge crate, while very nice and espressive, is heavily reliant on
> > traits; because trait functions are never const, bilge and const mix
> > about as well as water and oil.
> >
> > Try using the bitfield-struct crate instead.  It is built to support
> > const very well and the only downside is that more manual annotations
> > are needed (for enums and non-full-byte members).  Otherwise, the use
> > is pretty much the same and in fact device code does not change at all,
> > only register declarations.
> >
> > Recent versions want to use Rust 1.83, so this uses a slightly older
> > version with basically no lost functionality; but anyway, I want to switch
> > to 1.83 for QEMU as well due to improved "const" support in the compiler.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  rust/Cargo.toml                     |   1 +
> >  rust/hw/char/pl011/Cargo.toml       |   3 +-
> >  rust/hw/char/pl011/meson.build      |  11 +--
> >  rust/hw/char/pl011/src/registers.rs | 108 ++++++++++++++--------------
> >  4 files changed, 56 insertions(+), 67 deletions(-)
> >
> > diff --git a/rust/Cargo.toml b/rust/Cargo.toml
> > index 165328b6d01..3345858b5b4 100644
> > --- a/rust/Cargo.toml
> > +++ b/rust/Cargo.toml
> > @@ -97,5 +97,6 @@ used_underscore_binding = "deny"
> >  #wildcard_imports = "deny"   # still have many bindings::* imports
> >
> >  # these may have false positives
> > +enum_variant_names = "allow"
> >  #option_if_let_else = "deny"
> >  cognitive_complexity = "deny"
> > diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml
> > index 003ef9613d4..97e3dd00c35 100644
> > --- a/rust/hw/char/pl011/Cargo.toml
> > +++ b/rust/hw/char/pl011/Cargo.toml
> > @@ -16,8 +16,7 @@ rust-version.workspace = true
> >  crate-type = ["staticlib"]
> >
> >  [dependencies]
> > -bilge = { version = "0.2.0" }
> > -bilge-impl = { version = "0.2.0" }
> > +bitfield-struct = { version = "0.9" }
> >  bits = { path = "../../../bits" }
> >  qemu_api = { path = "../../../qemu-api" }
> >  qemu_api_macros = { path = "../../../qemu-api-macros" }
> > diff --git a/rust/hw/char/pl011/meson.build b/rust/hw/char/pl011/meson.build
> > index f134a6cdc6b..1bae5a03310 100644
> > --- a/rust/hw/char/pl011/meson.build
> > +++ b/rust/hw/char/pl011/meson.build
> > @@ -1,17 +1,10 @@
> > -subproject('bilge-0.2-rs', required: true)
> > -subproject('bilge-impl-0.2-rs', required: true)
> > -
> > -bilge_dep = dependency('bilge-0.2-rs')
> > -bilge_impl_dep = dependency('bilge-impl-0.2-rs')
> > -
> >  _libpl011_rs = static_library(
> >    'pl011',
> >    files('src/lib.rs'),
> >    override_options: ['rust_std=2021', 'build.rust_std=2021'],
> >    rust_abi: 'rust',
> >    dependencies: [
> > -    bilge_dep,
> > -    bilge_impl_dep,
> > +    bitfield_struct_dep,
> >      bits_rs,
> >      qemu_api,
> >      qemu_api_macros,
> > @@ -22,6 +15,6 @@ rust_devices_ss.add(when: 'CONFIG_X_PL011_RUST', if_true: [declare_dependency(
> >    link_whole: [_libpl011_rs],
> >    # Putting proc macro crates in `dependencies` is necessary for Meson to find
> >    # them when compiling the root per-target static rust lib.
> > -  dependencies: [bilge_impl_dep, qemu_api_macros],
> > +  dependencies: [bitfield_struct_dep, qemu_api_macros],
> >    variables: {'crate': 'pl011'},
> >  )])
> > diff --git a/rust/hw/char/pl011/src/registers.rs b/rust/hw/char/pl011/src/registers.rs
> > index 7ececd39f86..f2138c637c5 100644
> > --- a/rust/hw/char/pl011/src/registers.rs
> > +++ b/rust/hw/char/pl011/src/registers.rs
> > @@ -5,12 +5,16 @@
> >  //! Device registers exposed as typed structs which are backed by arbitrary
> >  //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
> >
> > +// rustc prefers "constant-like" enums to use upper case names, but that
> > +// is inconsistent in its own way.
> > +#![allow(non_upper_case_globals)]
> > +
> >  // For more detail see the PL011 Technical Reference Manual DDI0183:
> >  // https://developer.arm.com/documentation/ddi0183/latest/
> >
> > -use bilge::prelude::*;
> > +use bitfield_struct::bitfield;
> >  use bits::bits;
> > -use qemu_api::{impl_vmstate_bitsized, impl_vmstate_forward};
> > +use qemu_api::impl_vmstate_forward;
> >
> >  /// Offset of each register from the base memory address of the device.
> >  #[doc(alias = "offset")]
> > @@ -78,14 +82,18 @@ pub enum RegisterOffset {
> >  /// The `UARTRSR` register is updated only when a read occurs
> >  /// from the `UARTDR` register with the same status information
> >  /// that can also be obtained by reading the `UARTDR` register
> > -#[bitsize(8)]
> > -#[derive(Clone, Copy, Default, DebugBits, FromBits)]
> > +#[bitfield(u8)]
> >  pub struct Errors {
> >      pub framing_error: bool,
> >      pub parity_error: bool,
> >      pub break_error: bool,
> >      pub overrun_error: bool,
> > -    _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?


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