[PATCH 0/5] rust: pl011 cleanups + chardev bindings

[PATCH 0/5] rust: pl011 cleanups + chardev bindings

[Paolo Bonzini]

With this series, the only remaining use of unsafe is for vmstate's post
load callback, which is small and self contained.  All functionality
used by pl011 and HPET devices are wrapped in Rust APIs, so they look
like what a "real" from-scratch Rust device would be.

Patch 2 is best reviewed with "git diff -b --color-moved" or similar.

Paolo


Paolo Bonzini (5):
  rust: chardev: provide basic bindings to character devices
  rust: pl011: move register definitions out of lib.rs
  rust: pl011: clean up visibilities
  rust: pl011: switch to safe chardev operation
  rust: pl011: pass around registers::Data

 rust/hw/char/pl011/src/device.rs    | 146 +++-----
 rust/hw/char/pl011/src/lib.rs       | 509 +---------------------------
 rust/hw/char/pl011/src/registers.rs | 507 +++++++++++++++++++++++++++
 rust/qemu-api/meson.build           |  17 +-
 rust/qemu-api/src/chardev.rs        | 237 ++++++++++++-
 rust/qemu-api/src/zeroable.rs       |   1 +
 6 files changed, 800 insertions(+), 617 deletions(-)
 create mode 100644 rust/hw/char/pl011/src/registers.rs

-- 
2.48.1

[PATCH 1/5] rust: chardev: provide basic bindings to character devices

[Paolo Bonzini]

Most of the character device API is pretty simple, with "0 or -errno"
or "number of bytes or -errno" as the convention for return codes.
Add safe wrappers for the API to the CharBackend bindgen-generated
struct.

The API is not complete, but it covers the parts that are used
by the PL011 device, plus qemu_chr_fe_write which is needed to
implement the standard library Write trait.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/meson.build     |  17 ++-
 rust/qemu-api/src/chardev.rs  | 237 +++++++++++++++++++++++++++++++++-
 rust/qemu-api/src/zeroable.rs |   1 +
 3 files changed, 250 insertions(+), 5 deletions(-)

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 6e52c4bad74..a3f226ccc2a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -54,7 +54,19 @@ qemu_api = declare_dependency(link_with: _qemu_api_rs)
 rust_qemu_api_objs = static_library(
     'rust_qemu_api_objs',
     objects: [libqom.extract_all_objects(recursive: false),
-              libhwcore.extract_all_objects(recursive: false)])
+              libhwcore.extract_all_objects(recursive: false),
+              libchardev.extract_all_objects(recursive: false),
+              libcrypto.extract_all_objects(recursive: false),
+              libauthz.extract_all_objects(recursive: false),
+              libio.extract_all_objects(recursive: false)])
+rust_qemu_api_deps = declare_dependency(
+    dependencies: [
+      qom_ss.dependencies(),
+      chardev_ss.dependencies(),
+      crypto_ss.dependencies(),
+      authz_ss.dependencies(),
+      io_ss.dependencies()],
+    link_whole: [rust_qemu_api_objs, libqemuutil])
 
 test('rust-qemu-api-integration',
     executable(
@@ -63,8 +75,7 @@ test('rust-qemu-api-integration',
         override_options: ['rust_std=2021', 'build.rust_std=2021'],
         rust_args: ['--test'],
         install: false,
-        dependencies: [qemu_api, qemu_api_macros],
-        link_whole: [rust_qemu_api_objs, libqemuutil]),
+        dependencies: [qemu_api, qemu_api_macros, rust_qemu_api_deps]),
     args: [
         '--test', '--test-threads', '1',
         '--format', 'pretty',
diff --git a/rust/qemu-api/src/chardev.rs b/rust/qemu-api/src/chardev.rs
index a35b9217e90..3281844fd4e 100644
--- a/rust/qemu-api/src/chardev.rs
+++ b/rust/qemu-api/src/chardev.rs
@@ -3,10 +3,23 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 //! Bindings for character devices
+//!
+//! Character devices in QEMU can run under the big QEMU lock or in a separate
+//! `GMainContext`. Here we only support the former, because the bindings
+//! enforce that the BQL is taken whenever the functions in [`CharBackend`] are
+//! called.
 
-use std::ffi::CStr;
+use std::{
+    ffi::CStr,
+    fmt::{self, Debug},
+    io::{self, ErrorKind, Write},
+    marker::PhantomPinned,
+    os::raw::{c_int, c_void},
+    ptr::addr_of_mut,
+    slice,
+};
 
-use crate::{bindings, cell::Opaque, prelude::*};
+use crate::{bindings, callbacks::FnCall, cell::{BqlRefMut, Opaque}, prelude::*};
 
 /// A safe wrapper around [`bindings::Chardev`].
 #[repr(transparent)]
@@ -14,6 +27,226 @@
 pub struct Chardev(Opaque<bindings::Chardev>);
 
 pub type ChardevClass = bindings::ChardevClass;
+pub type Event = bindings::QEMUChrEvent;
+
+/// A safe wrapper around [`bindings::CharBackend`], denoting the character
+/// back-end that is used for example by a device.  Compared to the
+/// underlying C struct it adds BQL protection, and is marked as pinned
+/// because the QOM object ([`bindings::Chardev`]) contains a pointer to
+/// the `CharBackend`.
+pub struct CharBackend {
+    inner: BqlRefCell<bindings::CharBackend>,
+    _pin: PhantomPinned,
+}
+
+impl Write for BqlRefMut<'_, bindings::CharBackend> {
+    fn flush(&mut self) -> io::Result<()> {
+        Ok(())
+    }
+
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        let chr: &mut bindings::CharBackend = self;
+
+        let len = buf.len().try_into().unwrap();
+        let r = unsafe { bindings::qemu_chr_fe_write(addr_of_mut!(*chr), buf.as_ptr(), len) };
+        errno::into_io_result(r).map(|cnt| cnt as usize)
+    }
+
+    fn write_all(&mut self, buf: &[u8]) -> io::Result<()> {
+        let chr: &mut bindings::CharBackend = self;
+
+        let len = buf.len().try_into().unwrap();
+        let r = unsafe { bindings::qemu_chr_fe_write_all(addr_of_mut!(*chr), buf.as_ptr(), len) };
+        errno::into_io_result(r).and_then(|cnt| {
+            if cnt as usize == buf.len() {
+                Ok(())
+            } else {
+                Err(ErrorKind::WriteZero.into())
+            }
+        })
+    }
+}
+
+impl Debug for CharBackend {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // SAFETY: accessed just to print the values
+        let chr = self.inner.as_ptr();
+        Debug::fmt(unsafe { &*chr }, f)
+    }
+}
+
+// FIXME: use something like PinnedDrop from the pinned_init crate
+impl Drop for CharBackend {
+    fn drop(&mut self) {
+        self.disable_handlers();
+    }
+}
+
+impl CharBackend {
+    /// Enable the front-end's character device handlers, if there is an
+    /// associated `Chardev`.
+    pub fn enable_handlers<
+        'chardev,
+        'owner: 'chardev,
+        T,
+        CanReceiveFn: for<'a> FnCall<(&'a T,), u32>,
+        ReceiveFn: for<'a, 'b> FnCall<(&'a T, &'b [u8])>,
+        EventFn: for<'a> FnCall<(&'a T, Event)>,
+    >(
+        // When "self" is dropped, the handlers are automatically disabled.
+        // However, this is not necessarily true if the owner is dropped.
+        // So require the owner to outlive the character device.
+        &'chardev self,
+        owner: &'owner T,
+        _can_receive: CanReceiveFn,
+        _receive: ReceiveFn,
+        _event: EventFn,
+    ) {
+        unsafe extern "C" fn rust_can_receive_cb<T, F: for<'a> FnCall<(&'a T,), u32>>(
+            opaque: *mut c_void,
+        ) -> c_int {
+            // SAFETY: the values are safe according to the contract of
+            // enable_handlers() and qemu_chr_fe_set_handlers()
+            let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+            let r = F::call((owner,));
+            r.try_into().unwrap()
+        }
+
+        unsafe extern "C" fn rust_receive_cb<T, F: for<'a, 'b> FnCall<(&'a T, &'b [u8])>>(
+            opaque: *mut c_void,
+            buf: *const u8,
+            size: c_int,
+        ) {
+            // SAFETY: the values are safe according to the contract of
+            // enable_handlers() and qemu_chr_fe_set_handlers()
+            let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+            let buf = unsafe { slice::from_raw_parts(buf, size.try_into().unwrap()) };
+            F::call((owner, buf))
+        }
+
+        unsafe extern "C" fn rust_event_cb<T, F: for<'a> FnCall<(&'a T, Event)>>(
+            opaque: *mut c_void,
+            event: Event,
+        ) {
+            // SAFETY: the values are safe according to the contract of
+            // enable_handlers() and qemu_chr_fe_set_handlers()
+            let owner: &T = unsafe { &*(opaque.cast::<T>()) };
+            F::call((owner, event))
+        }
+
+        let _: () = CanReceiveFn::ASSERT_IS_SOME;
+        let receive_cb: Option<unsafe extern "C" fn(*mut c_void, *const u8, c_int)> =
+            if ReceiveFn::is_some() {
+                Some(rust_receive_cb::<T, ReceiveFn>)
+            } else {
+                None
+            };
+        let event_cb: Option<unsafe extern "C" fn(*mut c_void, Event)> = if EventFn::is_some() {
+            Some(rust_event_cb::<T, EventFn>)
+        } else {
+            None
+        };
+
+        let mut chr = self.inner.borrow_mut();
+        // SAFETY: the borrow promises that the BQL is taken
+        unsafe {
+            bindings::qemu_chr_fe_set_handlers(
+                addr_of_mut!(*chr),
+                Some(rust_can_receive_cb::<T, CanReceiveFn>),
+                receive_cb,
+                event_cb,
+                None,
+                (owner as *const T as *mut T).cast::<c_void>(),
+                core::ptr::null_mut(),
+                true,
+            );
+        }
+    }
+
+    /// Disable the front-end's character device handlers.
+    pub fn disable_handlers(&self) {
+        let mut chr = self.inner.borrow_mut();
+        // SAFETY: the borrow promises that the BQL is taken
+        unsafe {
+            bindings::qemu_chr_fe_set_handlers(
+                addr_of_mut!(*chr),
+                None,
+                None,
+                None,
+                None,
+                core::ptr::null_mut(),
+                core::ptr::null_mut(),
+                true,
+            );
+        }
+    }
+
+    /// Notify that the frontend is ready to receive data.
+    pub fn accept_input(&self) {
+        let mut chr = self.inner.borrow_mut();
+        // SAFETY: the borrow promises that the BQL is taken
+        unsafe { bindings::qemu_chr_fe_accept_input(addr_of_mut!(*chr)) }
+    }
+
+    /// Temporarily borrow the character device, allowing it to be used
+    /// as an implementor of `Write`.  Note that it is not valid to drop
+    /// the big QEMU lock while the character device is borrowed, as
+    /// that might cause C code to write to the character device.
+    pub fn borrow_mut(&self) -> impl Write + '_ {
+        self.inner.borrow_mut()
+    }
+
+    /// Send a continuous stream of zero bits on the line if `enabled` is
+    /// true, or a short stream if `enabled` is false.
+    pub fn send_break(&self, long: bool) -> io::Result<()> {
+        let mut chr = self.inner.borrow_mut();
+        let mut duration: c_int = long.into();
+        // SAFETY: the borrow promises that the BQL is taken
+        let r = unsafe {
+            bindings::qemu_chr_fe_ioctl(
+                addr_of_mut!(*chr),
+                bindings::CHR_IOCTL_SERIAL_SET_BREAK as i32,
+                addr_of_mut!(duration).cast::<c_void>(),
+            )
+        };
+
+        errno::into_io_result(r).map(|_| ())
+    }
+
+    /// Write data to a character backend from the front end.  This function
+    /// will send data from the front end to the back end.  Unlike
+    /// `write`, this function will block if the back end cannot
+    /// consume all of the data attempted to be written.
+    ///
+    /// Returns the number of bytes consumed (0 if no associated Chardev) or an
+    /// error.
+    pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
+        let len = buf.len().try_into().unwrap();
+        // SAFETY: qemu_chr_fe_write is thread-safe
+        let r = unsafe { bindings::qemu_chr_fe_write(self.inner.as_ptr(), buf.as_ptr(), len) };
+        errno::into_io_result(r).map(|cnt| cnt as usize)
+    }
+
+    /// Write data to a character backend from the front end.  This function
+    /// will send data from the front end to the back end.  Unlike
+    /// `write`, this function will block if the back end cannot
+    /// consume all of the data attempted to be written.
+    ///
+    /// Returns the number of bytes consumed (0 if no associated Chardev) or an
+    /// error.
+    pub fn write_all(&self, buf: &[u8]) -> io::Result<()> {
+        let len = buf.len().try_into().unwrap();
+        // SAFETY: qemu_chr_fe_write_all is thread-safe
+        let r = unsafe { bindings::qemu_chr_fe_write_all(self.inner.as_ptr(), buf.as_ptr(), len) };
+        errno::into_io_result(r).and_then(|cnt| {
+            if cnt as usize == buf.len() {
+                Ok(())
+            } else {
+                Err(ErrorKind::WriteZero.into())
+            }
+        })
+    }
+}
 
 unsafe impl ObjectType for Chardev {
     type Class = ChardevClass;
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 47b6977828d..a3415a2ebcc 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -106,3 +106,4 @@ fn default() -> Self {
 impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
 impl_zeroable!(crate::bindings::MemoryRegionOps);
 impl_zeroable!(crate::bindings::MemTxAttrs);
+impl_zeroable!(crate::bindings::CharBackend);
-- 
2.48.1

[PATCH 2/5] rust: pl011: move register definitions out of lib.rs

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs    |   7 +-
 rust/hw/char/pl011/src/lib.rs       | 509 +---------------------------
 rust/hw/char/pl011/src/registers.rs | 507 +++++++++++++++++++++++++++
 3 files changed, 513 insertions(+), 510 deletions(-)
 create mode 100644 rust/hw/char/pl011/src/registers.rs

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index d0857b470c9..01540654cc9 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -26,10 +26,13 @@
 
 use crate::{
     device_class,
-    registers::{self, Interrupt},
-    RegisterOffset,
+    registers::{self, Interrupt, RegisterOffset},
 };
 
+// TODO: You must disable the UART before any of the control registers are
+// reprogrammed. When the UART is disabled in the middle of transmission or
+// reception, it completes the current character before stopping
+
 /// Integer Baud Rate Divider, `UARTIBRD`
 const IBRD_MASK: u32 = 0xffff;
 
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 1bf46c65af2..45c13ba899e 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -18,516 +18,9 @@
 
 mod device;
 mod device_class;
+mod registers;
 
 pub use device::pl011_create;
 
 pub const TYPE_PL011: &::std::ffi::CStr = c_str!("pl011");
 pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c_str!("pl011_luminary");
-
-/// Offset of each register from the base memory address of the device.
-///
-/// # Source
-/// ARM DDI 0183G, Table 3-1 p.3-3
-#[doc(alias = "offset")]
-#[allow(non_camel_case_types)]
-#[repr(u64)]
-#[derive(Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
-enum RegisterOffset {
-    /// Data Register
-    ///
-    /// A write to this register initiates the actual data transmission
-    #[doc(alias = "UARTDR")]
-    DR = 0x000,
-    /// Receive Status Register or Error Clear Register
-    #[doc(alias = "UARTRSR")]
-    #[doc(alias = "UARTECR")]
-    RSR = 0x004,
-    /// Flag Register
-    ///
-    /// A read of this register shows if transmission is complete
-    #[doc(alias = "UARTFR")]
-    FR = 0x018,
-    /// Fractional Baud Rate Register
-    ///
-    /// responsible for baud rate speed
-    #[doc(alias = "UARTFBRD")]
-    FBRD = 0x028,
-    /// `IrDA` Low-Power Counter Register
-    #[doc(alias = "UARTILPR")]
-    ILPR = 0x020,
-    /// Integer Baud Rate Register
-    ///
-    /// Responsible for baud rate speed
-    #[doc(alias = "UARTIBRD")]
-    IBRD = 0x024,
-    /// line control register (data frame format)
-    #[doc(alias = "UARTLCR_H")]
-    LCR_H = 0x02C,
-    /// Toggle UART, transmission or reception
-    #[doc(alias = "UARTCR")]
-    CR = 0x030,
-    /// Interrupt FIFO Level Select Register
-    #[doc(alias = "UARTIFLS")]
-    FLS = 0x034,
-    /// Interrupt Mask Set/Clear Register
-    #[doc(alias = "UARTIMSC")]
-    IMSC = 0x038,
-    /// Raw Interrupt Status Register
-    #[doc(alias = "UARTRIS")]
-    RIS = 0x03C,
-    /// Masked Interrupt Status Register
-    #[doc(alias = "UARTMIS")]
-    MIS = 0x040,
-    /// Interrupt Clear Register
-    #[doc(alias = "UARTICR")]
-    ICR = 0x044,
-    /// DMA control Register
-    #[doc(alias = "UARTDMACR")]
-    DMACR = 0x048,
-    ///// Reserved, offsets `0x04C` to `0x07C`.
-    //Reserved = 0x04C,
-}
-
-mod registers {
-    //! Device registers exposed as typed structs which are backed by arbitrary
-    //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
-    use bilge::prelude::*;
-    use qemu_api::impl_vmstate_bitsized;
-
-    /// Receive Status Register / Data Register common error bits
-    ///
-    /// 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)]
-    pub struct Errors {
-        pub framing_error: bool,
-        pub parity_error: bool,
-        pub break_error: bool,
-        pub overrun_error: bool,
-        _reserved_unpredictable: u4,
-    }
-
-    // TODO: FIFO Mode has different semantics
-    /// Data Register, `UARTDR`
-    ///
-    /// The `UARTDR` register is the data register.
-    ///
-    /// For words to be transmitted:
-    ///
-    /// - if the FIFOs are enabled, data written to this location is pushed onto
-    ///   the transmit
-    /// FIFO
-    /// - if the FIFOs are not enabled, data is stored in the transmitter
-    ///   holding register (the
-    /// bottom word of the transmit FIFO).
-    ///
-    /// The write operation initiates transmission from the UART. The data is
-    /// prefixed with a start bit, appended with the appropriate parity bit
-    /// (if parity is enabled), and a stop bit. The resultant word is then
-    /// transmitted.
-    ///
-    /// For received words:
-    ///
-    /// - if the FIFOs are enabled, the data byte and the 4-bit status (break,
-    ///   frame, parity,
-    /// and overrun) is pushed onto the 12-bit wide receive FIFO
-    /// - if the FIFOs are not enabled, the data byte and status are stored in
-    ///   the receiving
-    /// holding register (the bottom word of the receive FIFO).
-    ///
-    /// The received data byte is read by performing reads from the `UARTDR`
-    /// register along with the corresponding status information. The status
-    /// information can also be read by a read of the `UARTRSR/UARTECR`
-    /// register.
-    ///
-    /// # Note
-    ///
-    /// You must disable the UART before any of the control registers are
-    /// reprogrammed. When the UART is disabled in the middle of
-    /// transmission or reception, it completes the current character before
-    /// stopping.
-    ///
-    /// # Source
-    /// ARM DDI 0183G 3.3.1 Data Register, UARTDR
-    #[bitsize(32)]
-    #[derive(Clone, Copy, Default, DebugBits, FromBits)]
-    #[doc(alias = "UARTDR")]
-    pub struct Data {
-        pub data: u8,
-        pub errors: Errors,
-        _reserved: u16,
-    }
-    impl_vmstate_bitsized!(Data);
-
-    impl Data {
-        // bilge is not very const-friendly, unfortunately
-        pub const BREAK: Self = Self { value: 1 << 10 };
-    }
-
-    // TODO: FIFO Mode has different semantics
-    /// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
-    ///
-    /// The UARTRSR/UARTECR register is the receive status register/error clear
-    /// register. Receive status can also be read from the `UARTRSR`
-    /// register. If the status is read from this register, then the status
-    /// information for break, framing and parity corresponds to the
-    /// data character read from the [Data register](Data), `UARTDR` prior to
-    /// reading the UARTRSR register. The status information for overrun is
-    /// set immediately when an overrun condition occurs.
-    ///
-    ///
-    /// # Note
-    /// The received data character must be read first from the [Data
-    /// Register](Data), `UARTDR` before reading the error status associated
-    /// with that data character from the `UARTRSR` register. This read
-    /// sequence cannot be reversed, because the `UARTRSR` register is
-    /// updated only when a read occurs from the `UARTDR` register. However,
-    /// the status information can also be obtained by reading the `UARTDR`
-    /// register
-    ///
-    /// # Source
-    /// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
-    /// UARTRSR/UARTECR
-    #[bitsize(32)]
-    #[derive(Clone, Copy, DebugBits, FromBits)]
-    pub struct ReceiveStatusErrorClear {
-        pub errors: Errors,
-        _reserved_unpredictable: u24,
-    }
-    impl_vmstate_bitsized!(ReceiveStatusErrorClear);
-
-    impl ReceiveStatusErrorClear {
-        pub fn set_from_data(&mut self, data: Data) {
-            self.set_errors(data.errors());
-        }
-
-        pub fn reset(&mut self) {
-            // All the bits are cleared to 0 on reset.
-            *self = Self::default();
-        }
-    }
-
-    impl Default for ReceiveStatusErrorClear {
-        fn default() -> Self {
-            0.into()
-        }
-    }
-
-    #[bitsize(32)]
-    #[derive(Clone, Copy, DebugBits, FromBits)]
-    /// Flag Register, `UARTFR`
-    #[doc(alias = "UARTFR")]
-    pub struct Flags {
-        /// CTS Clear to send. This bit is the complement of the UART clear to
-        /// send, `nUARTCTS`, modem status input. That is, the bit is 1
-        /// when `nUARTCTS` is LOW.
-        pub clear_to_send: bool,
-        /// DSR Data set ready. This bit is the complement of the UART data set
-        /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when
-        /// `nUARTDSR` is LOW.
-        pub data_set_ready: bool,
-        /// DCD Data carrier detect. This bit is the complement of the UART data
-        /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is
-        /// 1 when `nUARTDCD` is LOW.
-        pub data_carrier_detect: bool,
-        /// BUSY UART busy. If this bit is set to 1, the UART is busy
-        /// transmitting data. This bit remains set until the complete
-        /// byte, including all the stop bits, has been sent from the
-        /// shift register. This bit is set as soon as the transmit FIFO
-        /// becomes non-empty, regardless of whether the UART is enabled
-        /// or not.
-        pub busy: bool,
-        /// RXFE Receive FIFO empty. The meaning of this bit depends on the
-        /// state of the FEN bit in the UARTLCR_H register. If the FIFO
-        /// is disabled, this bit is set when the receive holding
-        /// register is empty. If the FIFO is enabled, the RXFE bit is
-        /// set when the receive FIFO is empty.
-        pub receive_fifo_empty: bool,
-        /// TXFF Transmit FIFO full. The meaning of this bit depends on the
-        /// state of the FEN bit in the UARTLCR_H register. If the FIFO
-        /// is disabled, this bit is set when the transmit holding
-        /// register is full. If the FIFO is enabled, the TXFF bit is
-        /// set when the transmit FIFO is full.
-        pub transmit_fifo_full: bool,
-        /// RXFF Receive FIFO full. The meaning of this bit depends on the state
-        /// of the FEN bit in the UARTLCR_H register. If the FIFO is
-        /// disabled, this bit is set when the receive holding register
-        /// is full. If the FIFO is enabled, the RXFF bit is set when
-        /// the receive FIFO is full.
-        pub receive_fifo_full: bool,
-        /// Transmit FIFO empty. The meaning of this bit depends on the state of
-        /// the FEN bit in the [Line Control register](LineControl),
-        /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the
-        /// transmit holding register is empty. If the FIFO is enabled,
-        /// the TXFE bit is set when the transmit FIFO is empty. This
-        /// bit does not indicate if there is data in the transmit shift
-        /// register.
-        pub transmit_fifo_empty: bool,
-        /// `RI`, is `true` when `nUARTRI` is `LOW`.
-        pub ring_indicator: bool,
-        _reserved_zero_no_modify: u23,
-    }
-    impl_vmstate_bitsized!(Flags);
-
-    impl Flags {
-        pub fn reset(&mut self) {
-            *self = Self::default();
-        }
-    }
-
-    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
-        }
-    }
-
-    #[bitsize(32)]
-    #[derive(Clone, Copy, DebugBits, FromBits)]
-    /// Line Control Register, `UARTLCR_H`
-    #[doc(alias = "UARTLCR_H")]
-    pub struct LineControl {
-        /// BRK Send break.
-        ///
-        /// If this bit is set to `1`, a low-level is continually output on the
-        /// `UARTTXD` output, after completing transmission of the
-        /// current character. For the proper execution of the break command,
-        /// the software must set this bit for at least two complete
-        /// frames. For normal use, this bit must be cleared to `0`.
-        pub send_break: bool,
-        /// 1 PEN Parity enable:
-        ///
-        /// - 0 = parity is disabled and no parity bit added to the data frame
-        /// - 1 = parity checking and generation is enabled.
-        ///
-        /// See Table 3-11 on page 3-14 for the parity truth table.
-        pub parity_enabled: bool,
-        /// EPS Even parity select. Controls the type of parity the UART uses
-        /// during transmission and reception:
-        /// - 0 = odd parity. The UART generates or checks for an odd number of
-        ///   1s in the data and parity bits.
-        /// - 1 = even parity. The UART generates or checks for an even number
-        ///   of 1s in the data and parity bits.
-        /// This bit has no effect when the `PEN` bit disables parity checking
-        /// and generation. See Table 3-11 on page 3-14 for the parity
-        /// truth table.
-        pub parity: Parity,
-        /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits
-        /// are transmitted at the end of the frame. The receive
-        /// logic does not check for two stop bits being received.
-        pub two_stops_bits: bool,
-        /// FEN Enable FIFOs:
-        /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
-        /// 1-byte-deep holding registers 1 = transmit and receive FIFO
-        /// buffers are enabled (FIFO mode).
-        pub fifos_enabled: Mode,
-        /// WLEN Word length. These bits indicate the number of data bits
-        /// transmitted or received in a frame as follows: b11 = 8 bits
-        /// b10 = 7 bits
-        /// b01 = 6 bits
-        /// b00 = 5 bits.
-        pub word_length: WordLength,
-        /// 7 SPS Stick parity select.
-        /// 0 = stick parity is disabled
-        /// 1 = either:
-        /// • if the EPS bit is 0 then the parity bit is transmitted and checked
-        /// as a 1 • if the EPS bit is 1 then the parity bit is
-        /// transmitted and checked as a 0. This bit has no effect when
-        /// the PEN bit disables parity checking and generation. See Table 3-11
-        /// on page 3-14 for the parity truth table.
-        pub sticky_parity: bool,
-        /// 31:8 - Reserved, do not modify, read as zero.
-        _reserved_zero_no_modify: u24,
-    }
-    impl_vmstate_bitsized!(LineControl);
-
-    impl LineControl {
-        pub fn reset(&mut self) {
-            // All the bits are cleared to 0 when reset.
-            *self = 0.into();
-        }
-    }
-
-    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).
-    pub enum Parity {
-        /// - 0 = odd parity. The UART generates or checks for an odd number of
-        ///   1s in the data and parity bits.
-        Odd = 0,
-        /// - 1 = even parity. The UART generates or checks for an even number
-        ///   of 1s in the data and parity bits.
-        Even = 1,
-    }
-
-    #[bitsize(1)]
-    #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
-    /// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
-    /// register](LineControl).
-    pub enum Mode {
-        /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
-        /// 1-byte-deep holding registers
-        Character = 0,
-        /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode).
-        FIFO = 1,
-    }
-
-    #[bitsize(2)]
-    #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
-    /// `WLEN` Word length, field of [Line Control register](LineControl).
-    ///
-    /// These bits indicate the number of data bits transmitted or received in a
-    /// frame as follows:
-    pub enum WordLength {
-        /// b11 = 8 bits
-        _8Bits = 0b11,
-        /// b10 = 7 bits
-        _7Bits = 0b10,
-        /// b01 = 6 bits
-        _6Bits = 0b01,
-        /// b00 = 5 bits.
-        _5Bits = 0b00,
-    }
-
-    /// Control Register, `UARTCR`
-    ///
-    /// The `UARTCR` register is the control register. All the bits are cleared
-    /// to `0` on reset except for bits `9` and `8` that are set to `1`.
-    ///
-    /// # Source
-    /// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
-    #[bitsize(32)]
-    #[doc(alias = "UARTCR")]
-    #[derive(Clone, Copy, DebugBits, FromBits)]
-    pub struct Control {
-        /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled
-        /// in the middle of transmission or reception, it completes the current
-        /// character before stopping. 1 = the UART is enabled. Data
-        /// transmission and reception occurs for either UART signals or SIR
-        /// signals depending on the setting of the SIREN bit.
-        pub enable_uart: bool,
-        /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT`
-        /// remains LOW (no light pulse generated), and signal transitions on
-        /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is
-        /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH,
-        /// in the marking state. Signal transitions on UARTRXD or modem status
-        /// inputs have no effect. This bit has no effect if the UARTEN bit
-        /// disables the UART.
-        pub enable_sir: bool,
-        /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding
-        /// mode. If this bit is cleared to 0, low-level bits are transmitted as
-        /// an active high pulse with a width of 3/ 16th of the bit period. If
-        /// this bit is set to 1, low-level bits are transmitted with a pulse
-        /// width that is 3 times the period of the IrLPBaud16 input signal,
-        /// regardless of the selected bit rate. Setting this bit uses less
-        /// power, but might reduce transmission distances.
-        pub sir_lowpower_irda_mode: u1,
-        /// Reserved, do not modify, read as zero.
-        _reserved_zero_no_modify: u4,
-        /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is
-        /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR
-        /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed
-        /// through to the SIRIN path. The SIRTEST bit in the test register must
-        /// be set to 1 to override the normal half-duplex SIR operation. This
-        /// must be the requirement for accessing the test registers during
-        /// normal operation, and SIRTEST must be cleared to 0 when loopback
-        /// testing is finished. This feature reduces the amount of external
-        /// coupling required during system test. If this bit is set to 1, and
-        /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the
-        /// UARTRXD path. In either SIR mode or UART mode, when this bit is set,
-        /// the modem outputs are also fed through to the modem inputs. This bit
-        /// is cleared to 0 on reset, to disable loopback.
-        pub enable_loopback: bool,
-        /// `TXE` Transmit enable. If this bit is set to 1, the transmit section
-        /// of the UART is enabled. Data transmission occurs for either UART
-        /// signals, or SIR signals depending on the setting of the SIREN bit.
-        /// When the UART is disabled in the middle of transmission, it
-        /// completes the current character before stopping.
-        pub enable_transmit: bool,
-        /// `RXE` Receive enable. If this bit is set to 1, the receive section
-        /// of the UART is enabled. Data reception occurs for either UART
-        /// signals or SIR signals depending on the setting of the SIREN bit.
-        /// When the UART is disabled in the middle of reception, it completes
-        /// the current character before stopping.
-        pub enable_receive: bool,
-        /// `DTR` Data transmit ready. This bit is the complement of the UART
-        /// data transmit ready, `nUARTDTR`, modem status output. That is, when
-        /// the bit is programmed to a 1 then `nUARTDTR` is LOW.
-        pub data_transmit_ready: bool,
-        /// `RTS` Request to send. This bit is the complement of the UART
-        /// request to send, `nUARTRTS`, modem status output. That is, when the
-        /// bit is programmed to a 1 then `nUARTRTS` is LOW.
-        pub request_to_send: bool,
-        /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`)
-        /// modem status output. That is, when the bit is programmed to a 1 the
-        /// output is 0. For DTE this can be used as Data Carrier Detect (DCD).
-        pub out_1: bool,
-        /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`)
-        /// modem status output. That is, when the bit is programmed to a 1, the
-        /// output is 0. For DTE this can be used as Ring Indicator (RI).
-        pub out_2: bool,
-        /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1,
-        /// RTS hardware flow control is enabled. Data is only requested when
-        /// there is space in the receive FIFO for it to be received.
-        pub rts_hardware_flow_control_enable: bool,
-        /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1,
-        /// CTS hardware flow control is enabled. Data is only transmitted when
-        /// the `nUARTCTS` signal is asserted.
-        pub cts_hardware_flow_control_enable: bool,
-        /// 31:16 - Reserved, do not modify, read as zero.
-        _reserved_zero_no_modify2: u16,
-    }
-    impl_vmstate_bitsized!(Control);
-
-    impl Control {
-        pub fn reset(&mut self) {
-            *self = 0.into();
-            self.set_enable_receive(true);
-            self.set_enable_transmit(true);
-        }
-    }
-
-    impl Default for Control {
-        fn default() -> Self {
-            let mut ret: Self = 0.into();
-            ret.reset();
-            ret
-        }
-    }
-
-    /// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
-    pub struct Interrupt(pub u32);
-
-    impl Interrupt {
-        pub const OE: Self = Self(1 << 10);
-        pub const BE: Self = Self(1 << 9);
-        pub const PE: Self = Self(1 << 8);
-        pub const FE: Self = Self(1 << 7);
-        pub const RT: Self = Self(1 << 6);
-        pub const TX: Self = Self(1 << 5);
-        pub const RX: Self = Self(1 << 4);
-        pub const DSR: Self = Self(1 << 3);
-        pub const DCD: Self = Self(1 << 2);
-        pub const CTS: Self = Self(1 << 1);
-        pub const RI: Self = Self(1 << 0);
-
-        pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
-        pub const MS: Self = Self(Self::RI.0 | Self::DSR.0 | Self::DCD.0 | Self::CTS.0);
-    }
-}
-
-// TODO: You must disable the UART before any of the control registers are
-// reprogrammed. When the UART is disabled in the middle of transmission or
-// reception, it completes the current character before stopping
diff --git a/rust/hw/char/pl011/src/registers.rs b/rust/hw/char/pl011/src/registers.rs
new file mode 100644
index 00000000000..ebd8297ce69
--- /dev/null
+++ b/rust/hw/char/pl011/src/registers.rs
@@ -0,0 +1,507 @@
+// Copyright 2024, Linaro Limited
+// Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Device registers exposed as typed structs which are backed by arbitrary
+//! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
+
+use bilge::prelude::*;
+
+use qemu_api::impl_vmstate_bitsized;
+
+/// Offset of each register from the base memory address of the device.
+///
+/// # Source
+/// ARM DDI 0183G, Table 3-1 p.3-3
+#[doc(alias = "offset")]
+#[allow(non_camel_case_types)]
+#[repr(u64)]
+#[derive(Debug, Eq, PartialEq, qemu_api_macros::TryInto)]
+pub enum RegisterOffset {
+    /// Data Register
+    ///
+    /// A write to this register initiates the actual data transmission
+    #[doc(alias = "UARTDR")]
+    DR = 0x000,
+    /// Receive Status Register or Error Clear Register
+    #[doc(alias = "UARTRSR")]
+    #[doc(alias = "UARTECR")]
+    RSR = 0x004,
+    /// Flag Register
+    ///
+    /// A read of this register shows if transmission is complete
+    #[doc(alias = "UARTFR")]
+    FR = 0x018,
+    /// Fractional Baud Rate Register
+    ///
+    /// responsible for baud rate speed
+    #[doc(alias = "UARTFBRD")]
+    FBRD = 0x028,
+    /// `IrDA` Low-Power Counter Register
+    #[doc(alias = "UARTILPR")]
+    ILPR = 0x020,
+    /// Integer Baud Rate Register
+    ///
+    /// Responsible for baud rate speed
+    #[doc(alias = "UARTIBRD")]
+    IBRD = 0x024,
+    /// line control register (data frame format)
+    #[doc(alias = "UARTLCR_H")]
+    LCR_H = 0x02C,
+    /// Toggle UART, transmission or reception
+    #[doc(alias = "UARTCR")]
+    CR = 0x030,
+    /// Interrupt FIFO Level Select Register
+    #[doc(alias = "UARTIFLS")]
+    FLS = 0x034,
+    /// Interrupt Mask Set/Clear Register
+    #[doc(alias = "UARTIMSC")]
+    IMSC = 0x038,
+    /// Raw Interrupt Status Register
+    #[doc(alias = "UARTRIS")]
+    RIS = 0x03C,
+    /// Masked Interrupt Status Register
+    #[doc(alias = "UARTMIS")]
+    MIS = 0x040,
+    /// Interrupt Clear Register
+    #[doc(alias = "UARTICR")]
+    ICR = 0x044,
+    /// DMA control Register
+    #[doc(alias = "UARTDMACR")]
+    DMACR = 0x048,
+    ///// Reserved, offsets `0x04C` to `0x07C`.
+    //Reserved = 0x04C,
+}
+
+/// Receive Status Register / Data Register common error bits
+///
+/// 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)]
+pub struct Errors {
+    pub framing_error: bool,
+    pub parity_error: bool,
+    pub break_error: bool,
+    pub overrun_error: bool,
+    _reserved_unpredictable: u4,
+}
+
+// TODO: FIFO Mode has different semantics
+/// Data Register, `UARTDR`
+///
+/// The `UARTDR` register is the data register.
+///
+/// For words to be transmitted:
+///
+/// - if the FIFOs are enabled, data written to this location is pushed onto
+///   the transmit
+/// FIFO
+/// - if the FIFOs are not enabled, data is stored in the transmitter
+///   holding register (the
+/// bottom word of the transmit FIFO).
+///
+/// The write operation initiates transmission from the UART. The data is
+/// prefixed with a start bit, appended with the appropriate parity bit
+/// (if parity is enabled), and a stop bit. The resultant word is then
+/// transmitted.
+///
+/// For received words:
+///
+/// - if the FIFOs are enabled, the data byte and the 4-bit status (break,
+///   frame, parity,
+/// and overrun) is pushed onto the 12-bit wide receive FIFO
+/// - if the FIFOs are not enabled, the data byte and status are stored in
+///   the receiving
+/// holding register (the bottom word of the receive FIFO).
+///
+/// The received data byte is read by performing reads from the `UARTDR`
+/// register along with the corresponding status information. The status
+/// information can also be read by a read of the `UARTRSR/UARTECR`
+/// register.
+///
+/// # Note
+///
+/// You must disable the UART before any of the control registers are
+/// reprogrammed. When the UART is disabled in the middle of
+/// transmission or reception, it completes the current character before
+/// stopping.
+///
+/// # Source
+/// ARM DDI 0183G 3.3.1 Data Register, UARTDR
+#[bitsize(32)]
+#[derive(Clone, Copy, Default, DebugBits, FromBits)]
+#[doc(alias = "UARTDR")]
+pub struct Data {
+    pub data: u8,
+    pub errors: Errors,
+    _reserved: u16,
+}
+impl_vmstate_bitsized!(Data);
+
+impl Data {
+    // bilge is not very const-friendly, unfortunately
+    pub const BREAK: Self = Self { value: 1 << 10 };
+}
+
+// TODO: FIFO Mode has different semantics
+/// Receive Status Register / Error Clear Register, `UARTRSR/UARTECR`
+///
+/// The UARTRSR/UARTECR register is the receive status register/error clear
+/// register. Receive status can also be read from the `UARTRSR`
+/// register. If the status is read from this register, then the status
+/// information for break, framing and parity corresponds to the
+/// data character read from the [Data register](Data), `UARTDR` prior to
+/// reading the UARTRSR register. The status information for overrun is
+/// set immediately when an overrun condition occurs.
+///
+///
+/// # Note
+/// The received data character must be read first from the [Data
+/// Register](Data), `UARTDR` before reading the error status associated
+/// with that data character from the `UARTRSR` register. This read
+/// sequence cannot be reversed, because the `UARTRSR` register is
+/// updated only when a read occurs from the `UARTDR` register. However,
+/// the status information can also be obtained by reading the `UARTDR`
+/// register
+///
+/// # Source
+/// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
+/// UARTRSR/UARTECR
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+pub struct ReceiveStatusErrorClear {
+    pub errors: Errors,
+    _reserved_unpredictable: u24,
+}
+impl_vmstate_bitsized!(ReceiveStatusErrorClear);
+
+impl ReceiveStatusErrorClear {
+    pub fn set_from_data(&mut self, data: Data) {
+        self.set_errors(data.errors());
+    }
+
+    pub fn reset(&mut self) {
+        // All the bits are cleared to 0 on reset.
+        *self = Self::default();
+    }
+}
+
+impl Default for ReceiveStatusErrorClear {
+    fn default() -> Self {
+        0.into()
+    }
+}
+
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+/// Flag Register, `UARTFR`
+#[doc(alias = "UARTFR")]
+pub struct Flags {
+    /// CTS Clear to send. This bit is the complement of the UART clear to
+    /// send, `nUARTCTS`, modem status input. That is, the bit is 1
+    /// when `nUARTCTS` is LOW.
+    pub clear_to_send: bool,
+    /// DSR Data set ready. This bit is the complement of the UART data set
+    /// ready, `nUARTDSR`, modem status input. That is, the bit is 1 when
+    /// `nUARTDSR` is LOW.
+    pub data_set_ready: bool,
+    /// DCD Data carrier detect. This bit is the complement of the UART data
+    /// carrier detect, `nUARTDCD`, modem status input. That is, the bit is
+    /// 1 when `nUARTDCD` is LOW.
+    pub data_carrier_detect: bool,
+    /// BUSY UART busy. If this bit is set to 1, the UART is busy
+    /// transmitting data. This bit remains set until the complete
+    /// byte, including all the stop bits, has been sent from the
+    /// shift register. This bit is set as soon as the transmit FIFO
+    /// becomes non-empty, regardless of whether the UART is enabled
+    /// or not.
+    pub busy: bool,
+    /// RXFE Receive FIFO empty. The meaning of this bit depends on the
+    /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+    /// is disabled, this bit is set when the receive holding
+    /// register is empty. If the FIFO is enabled, the RXFE bit is
+    /// set when the receive FIFO is empty.
+    pub receive_fifo_empty: bool,
+    /// TXFF Transmit FIFO full. The meaning of this bit depends on the
+    /// state of the FEN bit in the UARTLCR_H register. If the FIFO
+    /// is disabled, this bit is set when the transmit holding
+    /// register is full. If the FIFO is enabled, the TXFF bit is
+    /// set when the transmit FIFO is full.
+    pub transmit_fifo_full: bool,
+    /// RXFF Receive FIFO full. The meaning of this bit depends on the state
+    /// of the FEN bit in the UARTLCR_H register. If the FIFO is
+    /// disabled, this bit is set when the receive holding register
+    /// is full. If the FIFO is enabled, the RXFF bit is set when
+    /// the receive FIFO is full.
+    pub receive_fifo_full: bool,
+    /// Transmit FIFO empty. The meaning of this bit depends on the state of
+    /// the FEN bit in the [Line Control register](LineControl),
+    /// `UARTLCR_H`. If the FIFO is disabled, this bit is set when the
+    /// transmit holding register is empty. If the FIFO is enabled,
+    /// the TXFE bit is set when the transmit FIFO is empty. This
+    /// bit does not indicate if there is data in the transmit shift
+    /// register.
+    pub transmit_fifo_empty: bool,
+    /// `RI`, is `true` when `nUARTRI` is `LOW`.
+    pub ring_indicator: bool,
+    _reserved_zero_no_modify: u23,
+}
+impl_vmstate_bitsized!(Flags);
+
+impl Flags {
+    pub fn reset(&mut self) {
+        *self = Self::default();
+    }
+}
+
+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
+    }
+}
+
+#[bitsize(32)]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+/// Line Control Register, `UARTLCR_H`
+#[doc(alias = "UARTLCR_H")]
+pub struct LineControl {
+    /// BRK Send break.
+    ///
+    /// If this bit is set to `1`, a low-level is continually output on the
+    /// `UARTTXD` output, after completing transmission of the
+    /// current character. For the proper execution of the break command,
+    /// the software must set this bit for at least two complete
+    /// frames. For normal use, this bit must be cleared to `0`.
+    pub send_break: bool,
+    /// 1 PEN Parity enable:
+    ///
+    /// - 0 = parity is disabled and no parity bit added to the data frame
+    /// - 1 = parity checking and generation is enabled.
+    ///
+    /// See Table 3-11 on page 3-14 for the parity truth table.
+    pub parity_enabled: bool,
+    /// EPS Even parity select. Controls the type of parity the UART uses
+    /// during transmission and reception:
+    /// - 0 = odd parity. The UART generates or checks for an odd number of
+    ///   1s in the data and parity bits.
+    /// - 1 = even parity. The UART generates or checks for an even number
+    ///   of 1s in the data and parity bits.
+    /// This bit has no effect when the `PEN` bit disables parity checking
+    /// and generation. See Table 3-11 on page 3-14 for the parity
+    /// truth table.
+    pub parity: Parity,
+    /// 3 STP2 Two stop bits select. If this bit is set to 1, two stop bits
+    /// are transmitted at the end of the frame. The receive
+    /// logic does not check for two stop bits being received.
+    pub two_stops_bits: bool,
+    /// FEN Enable FIFOs:
+    /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+    /// 1-byte-deep holding registers 1 = transmit and receive FIFO
+    /// buffers are enabled (FIFO mode).
+    pub fifos_enabled: Mode,
+    /// WLEN Word length. These bits indicate the number of data bits
+    /// transmitted or received in a frame as follows: b11 = 8 bits
+    /// b10 = 7 bits
+    /// b01 = 6 bits
+    /// b00 = 5 bits.
+    pub word_length: WordLength,
+    /// 7 SPS Stick parity select.
+    /// 0 = stick parity is disabled
+    /// 1 = either:
+    /// • if the EPS bit is 0 then the parity bit is transmitted and checked
+    /// as a 1 • if the EPS bit is 1 then the parity bit is
+    /// transmitted and checked as a 0. This bit has no effect when
+    /// the PEN bit disables parity checking and generation. See Table 3-11
+    /// on page 3-14 for the parity truth table.
+    pub sticky_parity: bool,
+    /// 31:8 - Reserved, do not modify, read as zero.
+    _reserved_zero_no_modify: u24,
+}
+impl_vmstate_bitsized!(LineControl);
+
+impl LineControl {
+    pub fn reset(&mut self) {
+        // All the bits are cleared to 0 when reset.
+        *self = 0.into();
+    }
+}
+
+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).
+pub enum Parity {
+    /// - 0 = odd parity. The UART generates or checks for an odd number of
+    ///   1s in the data and parity bits.
+    Odd = 0,
+    /// - 1 = even parity. The UART generates or checks for an even number
+    ///   of 1s in the data and parity bits.
+    Even = 1,
+}
+
+#[bitsize(1)]
+#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+/// `FEN` "Enable FIFOs" or Device mode, field of [Line Control
+/// register](LineControl).
+pub enum Mode {
+    /// 0 = FIFOs are disabled (character mode) that is, the FIFOs become
+    /// 1-byte-deep holding registers
+    Character = 0,
+    /// 1 = transmit and receive FIFO buffers are enabled (FIFO mode).
+    FIFO = 1,
+}
+
+#[bitsize(2)]
+#[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
+/// `WLEN` Word length, field of [Line Control register](LineControl).
+///
+/// These bits indicate the number of data bits transmitted or received in a
+/// frame as follows:
+pub enum WordLength {
+    /// b11 = 8 bits
+    _8Bits = 0b11,
+    /// b10 = 7 bits
+    _7Bits = 0b10,
+    /// b01 = 6 bits
+    _6Bits = 0b01,
+    /// b00 = 5 bits.
+    _5Bits = 0b00,
+}
+
+/// Control Register, `UARTCR`
+///
+/// The `UARTCR` register is the control register. All the bits are cleared
+/// to `0` on reset except for bits `9` and `8` that are set to `1`.
+///
+/// # Source
+/// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
+#[bitsize(32)]
+#[doc(alias = "UARTCR")]
+#[derive(Clone, Copy, DebugBits, FromBits)]
+pub struct Control {
+    /// `UARTEN` UART enable: 0 = UART is disabled. If the UART is disabled
+    /// in the middle of transmission or reception, it completes the current
+    /// character before stopping. 1 = the UART is enabled. Data
+    /// transmission and reception occurs for either UART signals or SIR
+    /// signals depending on the setting of the SIREN bit.
+    pub enable_uart: bool,
+    /// `SIREN` `SIR` enable: 0 = IrDA SIR ENDEC is disabled. `nSIROUT`
+    /// remains LOW (no light pulse generated), and signal transitions on
+    /// SIRIN have no effect. 1 = IrDA SIR ENDEC is enabled. Data is
+    /// transmitted and received on nSIROUT and SIRIN. UARTTXD remains HIGH,
+    /// in the marking state. Signal transitions on UARTRXD or modem status
+    /// inputs have no effect. This bit has no effect if the UARTEN bit
+    /// disables the UART.
+    pub enable_sir: bool,
+    /// `SIRLP` SIR low-power IrDA mode. This bit selects the IrDA encoding
+    /// mode. If this bit is cleared to 0, low-level bits are transmitted as
+    /// an active high pulse with a width of 3/ 16th of the bit period. If
+    /// this bit is set to 1, low-level bits are transmitted with a pulse
+    /// width that is 3 times the period of the IrLPBaud16 input signal,
+    /// regardless of the selected bit rate. Setting this bit uses less
+    /// power, but might reduce transmission distances.
+    pub sir_lowpower_irda_mode: u1,
+    /// Reserved, do not modify, read as zero.
+    _reserved_zero_no_modify: u4,
+    /// `LBE` Loopback enable. If this bit is set to 1 and the SIREN bit is
+    /// set to 1 and the SIRTEST bit in the Test Control register, UARTTCR
+    /// on page 4-5 is set to 1, then the nSIROUT path is inverted, and fed
+    /// through to the SIRIN path. The SIRTEST bit in the test register must
+    /// be set to 1 to override the normal half-duplex SIR operation. This
+    /// must be the requirement for accessing the test registers during
+    /// normal operation, and SIRTEST must be cleared to 0 when loopback
+    /// testing is finished. This feature reduces the amount of external
+    /// coupling required during system test. If this bit is set to 1, and
+    /// the SIRTEST bit is set to 0, the UARTTXD path is fed through to the
+    /// UARTRXD path. In either SIR mode or UART mode, when this bit is set,
+    /// the modem outputs are also fed through to the modem inputs. This bit
+    /// is cleared to 0 on reset, to disable loopback.
+    pub enable_loopback: bool,
+    /// `TXE` Transmit enable. If this bit is set to 1, the transmit section
+    /// of the UART is enabled. Data transmission occurs for either UART
+    /// signals, or SIR signals depending on the setting of the SIREN bit.
+    /// When the UART is disabled in the middle of transmission, it
+    /// completes the current character before stopping.
+    pub enable_transmit: bool,
+    /// `RXE` Receive enable. If this bit is set to 1, the receive section
+    /// of the UART is enabled. Data reception occurs for either UART
+    /// signals or SIR signals depending on the setting of the SIREN bit.
+    /// When the UART is disabled in the middle of reception, it completes
+    /// the current character before stopping.
+    pub enable_receive: bool,
+    /// `DTR` Data transmit ready. This bit is the complement of the UART
+    /// data transmit ready, `nUARTDTR`, modem status output. That is, when
+    /// the bit is programmed to a 1 then `nUARTDTR` is LOW.
+    pub data_transmit_ready: bool,
+    /// `RTS` Request to send. This bit is the complement of the UART
+    /// request to send, `nUARTRTS`, modem status output. That is, when the
+    /// bit is programmed to a 1 then `nUARTRTS` is LOW.
+    pub request_to_send: bool,
+    /// `Out1` This bit is the complement of the UART Out1 (`nUARTOut1`)
+    /// modem status output. That is, when the bit is programmed to a 1 the
+    /// output is 0. For DTE this can be used as Data Carrier Detect (DCD).
+    pub out_1: bool,
+    /// `Out2` This bit is the complement of the UART Out2 (`nUARTOut2`)
+    /// modem status output. That is, when the bit is programmed to a 1, the
+    /// output is 0. For DTE this can be used as Ring Indicator (RI).
+    pub out_2: bool,
+    /// `RTSEn` RTS hardware flow control enable. If this bit is set to 1,
+    /// RTS hardware flow control is enabled. Data is only requested when
+    /// there is space in the receive FIFO for it to be received.
+    pub rts_hardware_flow_control_enable: bool,
+    /// `CTSEn` CTS hardware flow control enable. If this bit is set to 1,
+    /// CTS hardware flow control is enabled. Data is only transmitted when
+    /// the `nUARTCTS` signal is asserted.
+    pub cts_hardware_flow_control_enable: bool,
+    /// 31:16 - Reserved, do not modify, read as zero.
+    _reserved_zero_no_modify2: u16,
+}
+impl_vmstate_bitsized!(Control);
+
+impl Control {
+    pub fn reset(&mut self) {
+        *self = 0.into();
+        self.set_enable_receive(true);
+        self.set_enable_transmit(true);
+    }
+}
+
+impl Default for Control {
+    fn default() -> Self {
+        let mut ret: Self = 0.into();
+        ret.reset();
+        ret
+    }
+}
+
+/// Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC
+pub struct Interrupt(pub u32);
+
+impl Interrupt {
+    pub const OE: Self = Self(1 << 10);
+    pub const BE: Self = Self(1 << 9);
+    pub const PE: Self = Self(1 << 8);
+    pub const FE: Self = Self(1 << 7);
+    pub const RT: Self = Self(1 << 6);
+    pub const TX: Self = Self(1 << 5);
+    pub const RX: Self = Self(1 << 4);
+    pub const DSR: Self = Self(1 << 3);
+    pub const DCD: Self = Self(1 << 2);
+    pub const CTS: Self = Self(1 << 1);
+    pub const RI: Self = Self(1 << 0);
+
+    pub const E: Self = Self(Self::OE.0 | Self::BE.0 | Self::PE.0 | Self::FE.0);
+    pub const MS: Self = Self(Self::RI.0 | Self::DSR.0 | Self::DCD.0 | Self::CTS.0);
+}
-- 
2.48.1

Re: [PATCH 2/5] rust: pl011: move register definitions out of lib.rs

[Peter Maydell]

On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs    |   7 +-
>  rust/hw/char/pl011/src/lib.rs       | 509 +---------------------------
>  rust/hw/char/pl011/src/registers.rs | 507 +++++++++++++++++++++++++++
>  3 files changed, 513 insertions(+), 510 deletions(-)
>  create mode 100644 rust/hw/char/pl011/src/registers.rs

Looking at this patch I'm sorely tempted to suggest significantly
trimming down the commentary in these comments: it contains
rather more text cut-n-pasted from the PL011 TRM than I'm
entirely comfortable with, and much of it is detail that
is irrelevant to QEMU. I don't think we should be trying to
make it unnecessary for somebody working on the QEMU device
models to ever look at the hardware reference manuals.

thanks
-- PMM

Re: [PATCH 2/5] rust: pl011: move register definitions out of lib.rs

[Paolo Bonzini]

On 2/27/25 18:28, Peter Maydell wrote:
> On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   rust/hw/char/pl011/src/device.rs    |   7 +-
>>   rust/hw/char/pl011/src/lib.rs       | 509 +---------------------------
>>   rust/hw/char/pl011/src/registers.rs | 507 +++++++++++++++++++++++++++
>>   3 files changed, 513 insertions(+), 510 deletions(-)
>>   create mode 100644 rust/hw/char/pl011/src/registers.rs
> 
> Looking at this patch I'm sorely tempted to suggest significantly
> trimming down the commentary in these comments: it contains
> rather more text cut-n-pasted from the PL011 TRM than I'm
> entirely comfortable with, and much of it is detail that
> is irrelevant to QEMU.

Yeah, that was a point that was made on the call last week, too.  I kind 
of agree, but it wasn't a decision I really wanted to take or suggest.

Also, some of the stuff does not belong in the structs but could be 
added to lib.rs, too, with more fair-use justification than in 
registers.rs.  So perhaps delay removing it until more aspects of the 
FIFO are modeled correctly, so that one does not have to reinvent the 
wording from scratch.

Paolo

Re: [PATCH 2/5] rust: pl011: move register definitions out of lib.rs

[Paolo Bonzini]

On 2/27/25 18:28, Peter Maydell wrote:
> On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   rust/hw/char/pl011/src/device.rs    |   7 +-
>>   rust/hw/char/pl011/src/lib.rs       | 509 +---------------------------
>>   rust/hw/char/pl011/src/registers.rs | 507 +++++++++++++++++++++++++++
>>   3 files changed, 513 insertions(+), 510 deletions(-)
>>   create mode 100644 rust/hw/char/pl011/src/registers.rs
> 
> Looking at this patch I'm sorely tempted to suggest significantly
> trimming down the commentary in these comments: it contains
> rather more text cut-n-pasted from the PL011 TRM than I'm
> entirely comfortable with, and much of it is detail that
> is irrelevant to QEMU.

Yeah, that was a point that was made on the call last week, too.  I 
think I agree, but it wasn't a decision I really wanted to take or 
suggest myself...

That said, some of the stuff does not belong in the structs but could be 
added to lib.rs, too, with more fair-use justification than in 
registers.rs.  So perhaps we could delay removing it until more aspects 
of the FIFO are modeled correctly, so that one does not have to reinvent 
the wording from scratch.

Paolo

[PATCH 3/5] rust: pl011: clean up visibilities of callbacks

[Paolo Bonzini]

Do not make callbacks unnecessarily "pub", they are only used
through function pointers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 01540654cc9..4cdbbf4b73d 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -537,7 +537,7 @@ fn post_init(&self) {
         }
     }
 
-    pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
+    fn read(&self, offset: hwaddr, _size: u32) -> u64 {
         match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
                 let device_id = self.get_class().device_id;
@@ -560,7 +560,7 @@ pub fn read(&self, offset: hwaddr, _size: u32) -> u64 {
         }
     }
 
-    pub fn write(&self, offset: hwaddr, value: u64, _size: u32) {
+    fn write(&self, offset: hwaddr, value: u64, _size: u32) {
         let mut update_irq = false;
         if let Ok(field) = RegisterOffset::try_from(offset) {
             // qemu_chr_fe_write_all() calls into the can_receive
@@ -621,7 +621,7 @@ pub fn event(&self, event: QEMUChrEvent) {
         }
     }
 
-    pub fn realize(&self) {
+    fn realize(&self) {
         // SAFETY: self.char_backend has the correct size and alignment for a
         // CharBackend object, and its callbacks are of the correct types.
         unsafe {
@@ -638,11 +638,11 @@ pub fn realize(&self) {
         }
     }
 
-    pub fn reset_hold(&self, _type: ResetType) {
+    fn reset_hold(&self, _type: ResetType) {
         self.regs.borrow_mut().reset();
     }
 
-    pub fn update(&self) {
+    fn update(&self) {
         let regs = self.regs.borrow();
         let flags = regs.int_level & regs.int_enabled;
         for (irq, i) in self.interrupts.iter().zip(IRQMASK) {
-- 
2.48.1

[PATCH 4/5] rust: pl011: switch to safe chardev operation

[Paolo Bonzini]

Switch bindings::CharBackend with chardev::CharBackend.  This removes
occurrences of "unsafe" due to FFI and switches the wrappers for receive,
can_receive and event callbacks to the common ones implemented by
chardev::CharBackend.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 116 +++++++------------------------
 1 file changed, 25 insertions(+), 91 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 4cdbbf4b73d..2283bae71eb 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -4,16 +4,11 @@
 
 use std::{
     ffi::CStr,
-    os::raw::{c_int, c_void},
-    ptr::{addr_of, addr_of_mut, NonNull},
+    ptr::addr_of_mut,
 };
 
 use qemu_api::{
-    bindings::{
-        qemu_chr_fe_accept_input, qemu_chr_fe_ioctl, qemu_chr_fe_set_handlers,
-        qemu_chr_fe_write_all, CharBackend, QEMUChrEvent, CHR_IOCTL_SERIAL_SET_BREAK,
-    },
-    chardev::Chardev,
+    chardev::{CharBackend, Chardev, Event},
     impl_vmstate_forward,
     irq::{IRQState, InterruptSource},
     memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
@@ -235,7 +230,7 @@ pub(self) fn write(
         &mut self,
         offset: RegisterOffset,
         value: u32,
-        char_backend: *mut CharBackend,
+        char_backend: &CharBackend,
     ) -> bool {
         // eprintln!("write offset {offset} value {value}");
         use RegisterOffset::*;
@@ -269,17 +264,9 @@ pub(self) fn write(
                     self.reset_tx_fifo();
                 }
                 let update = (self.line_control.send_break() != new_val.send_break()) && {
-                    let mut break_enable: c_int = new_val.send_break().into();
-                    // SAFETY: self.char_backend is a valid CharBackend instance after it's been
-                    // initialized in realize().
-                    unsafe {
-                        qemu_chr_fe_ioctl(
-                            char_backend,
-                            CHR_IOCTL_SERIAL_SET_BREAK as i32,
-                            addr_of_mut!(break_enable).cast::<c_void>(),
-                        );
-                    }
-                    self.loopback_break(break_enable > 0)
+                    let break_enable = new_val.send_break();
+                    let _ = char_backend.send_break(break_enable);
+                    self.loopback_break(break_enable)
                 };
                 self.line_control = new_val;
                 self.set_read_trigger();
@@ -551,9 +538,7 @@ fn read(&self, offset: hwaddr, _size: u32) -> u64 {
                 let (update_irq, result) = self.regs.borrow_mut().read(field);
                 if update_irq {
                     self.update();
-                    unsafe {
-                        qemu_chr_fe_accept_input(addr_of!(self.char_backend) as *mut _);
-                    }
+                    self.char_backend.accept_input();
                 }
                 result.into()
             }
@@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
             // callback, so handle writes before entering PL011Registers.
             if field == RegisterOffset::DR {
                 // ??? Check if transmitter is enabled.
-                let ch: u8 = value as u8;
-                // SAFETY: char_backend is a valid CharBackend instance after it's been
-                // initialized in realize().
+                let ch: [u8; 1] = [value as u8];
                 // XXX this blocks entire thread. Rewrite to use
                 // qemu_chr_fe_write and background I/O callbacks
-                unsafe {
-                    qemu_chr_fe_write_all(addr_of!(self.char_backend) as *mut _, &ch, 1);
-                }
+                let _ = self.char_backend.write_all(&ch);
             }
 
-            update_irq = self.regs.borrow_mut().write(
-                field,
-                value as u32,
-                addr_of!(self.char_backend) as *mut _,
-            );
+            update_irq = self
+                .regs
+                .borrow_mut()
+                .write(field, value as u32, &self.char_backend);
         } else {
             eprintln!("write bad offset {offset} value {value}");
         }
@@ -590,15 +570,18 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
         }
     }
 
-    pub fn can_receive(&self) -> bool {
-        // trace_pl011_can_receive(s->lcr, s->read_count, r);
+    fn can_receive(&self) -> u32 {
         let regs = self.regs.borrow();
-        regs.read_count < regs.fifo_depth()
+        // trace_pl011_can_receive(s->lcr, s->read_count, r);
+        u32::from(regs.read_count < regs.fifo_depth())
     }
 
-    pub fn receive(&self, ch: u32) {
+    fn receive(&self, buf: &[u8]) {
+        if buf.is_empty() {
+            return;
+        }
         let mut regs = self.regs.borrow_mut();
-        let update_irq = !regs.loopback_enabled() && regs.put_fifo(ch);
+        let update_irq = !regs.loopback_enabled() && regs.put_fifo(buf[0].into());
         // Release the BqlRefCell before calling self.update()
         drop(regs);
 
@@ -607,10 +590,10 @@ pub fn receive(&self, ch: u32) {
         }
     }
 
-    pub fn event(&self, event: QEMUChrEvent) {
+    fn event(&self, event: Event) {
         let mut update_irq = false;
         let mut regs = self.regs.borrow_mut();
-        if event == QEMUChrEvent::CHR_EVENT_BREAK && !regs.loopback_enabled() {
+        if event == Event::CHR_EVENT_BREAK && !regs.loopback_enabled() {
             update_irq = regs.put_fifo(registers::Data::BREAK.into());
         }
         // Release the BqlRefCell before calling self.update()
@@ -622,20 +605,8 @@ pub fn event(&self, event: QEMUChrEvent) {
     }
 
     fn realize(&self) {
-        // SAFETY: self.char_backend has the correct size and alignment for a
-        // CharBackend object, and its callbacks are of the correct types.
-        unsafe {
-            qemu_chr_fe_set_handlers(
-                addr_of!(self.char_backend) as *mut CharBackend,
-                Some(pl011_can_receive),
-                Some(pl011_receive),
-                Some(pl011_event),
-                None,
-                addr_of!(*self).cast::<c_void>() as *mut c_void,
-                core::ptr::null_mut(),
-                true,
-            );
-        }
+        self.char_backend
+            .enable_handlers(self, Self::can_receive, Self::receive, Self::event);
     }
 
     fn reset_hold(&self, _type: ResetType) {
@@ -666,43 +637,6 @@ pub fn post_load(&self, _version_id: u32) -> Result<(), ()> {
     Interrupt::E.0,
 ];
 
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_can_receive(opaque: *mut c_void) -> c_int {
-    let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
-    unsafe { state.as_ref().can_receive().into() }
-}
-
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-///
-/// The buffer and size arguments must also be valid.
-pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const u8, size: c_int) {
-    let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
-    unsafe {
-        if size > 0 {
-            debug_assert!(!buf.is_null());
-            state.as_ref().receive(u32::from(buf.read_volatile()));
-        }
-    }
-}
-
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_event(opaque: *mut c_void, event: QEMUChrEvent) {
-    let state = NonNull::new(opaque).unwrap().cast::<PL011State>();
-    unsafe { state.as_ref().event(event) }
-}
-
 /// # Safety
 ///
 /// We expect the FFI user of this function to pass a valid pointer for `chr`
-- 
2.48.1

Re: [PATCH 4/5] rust: pl011: switch to safe chardev operation

[Peter Maydell]

On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Switch bindings::CharBackend with chardev::CharBackend.  This removes
> occurrences of "unsafe" due to FFI and switches the wrappers for receive,
> can_receive and event callbacks to the common ones implemented by
> chardev::CharBackend.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


> @@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {

> -            update_irq = self.regs.borrow_mut().write(
> -                field,
> -                value as u32,
> -                addr_of!(self.char_backend) as *mut _,
> -            );
> +            update_irq = self
> +                .regs
> +                .borrow_mut()
> +                .write(field, value as u32, &self.char_backend);
>          } else {
>              eprintln!("write bad offset {offset} value {value}");
>          }

Entirely unrelated to this patch, but seeing this go past
reminded me that I had a question I didn't get round to
asking in the community call the other day. In this
PL011State::write function, we delegate the job of
updating the register state to PL011Registers::write,
which returns a bool to tell us whether to call update().

I guess the underlying design idea here is "the register
object updates itself and tells the device object what
kinds of updates to the outside world it needs to do" ?
But then, why is the irq output something that PL011State
needs to handle itself whereas the chardev backend is
something we can pass into PL011Registers ?

In the C version, we just call pl011_update() where we
need to; we could validly call it unconditionally for any
write, we're just being (possibly prematurely) efficient
by avoiding a call when we happen to know that the register
write didn't touch any of the state that pl011_update()
cares about. So it feels a bit odd to me that in the Rust
version this "we happen to know that sometimes it would be
unnecessary to call the update function" has been kind of
promoted to being part of an interface between the two
different types PL011Registers and PL011State.

Thinking about other devices, presumably for more complex
devices we might need to pass more than just a single 'bool'
back from PL011Registers::write. What other kinds of thing
might we need to do in the FooState function, and (since
the pl011 code is presumably going to be used as a template
for those other devices) is it worth having something that
expresses that better than just a raw 'bool' return ?

thanks
-- PMM

Re: [PATCH 4/5] rust: pl011: switch to safe chardev operation

[Paolo Bonzini]

On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 27 Feb 2025 at 16:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Switch bindings::CharBackend with chardev::CharBackend.  This removes
> > occurrences of "unsafe" due to FFI and switches the wrappers for receive,
> > can_receive and event callbacks to the common ones implemented by
> > chardev::CharBackend.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> > @@ -567,21 +552,16 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
>
> > -            update_irq = self.regs.borrow_mut().write(
> > -                field,
> > -                value as u32,
> > -                addr_of!(self.char_backend) as *mut _,
> > -            );
> > +            update_irq = self
> > +                .regs
> > +                .borrow_mut()
> > +                .write(field, value as u32, &self.char_backend);
> >          } else {
> >              eprintln!("write bad offset {offset} value {value}");
> >          }
>
> Entirely unrelated to this patch, but seeing this go past
> reminded me that I had a question I didn't get round to
> asking in the community call the other day. In this
> PL011State::write function, we delegate the job of
> updating the register state to PL011Registers::write,
> which returns a bool to tell us whether to call update().
>
> I guess the underlying design idea here is "the register
> object updates itself and tells the device object what
> kinds of updates to the outside world it needs to do" ?
> But then, why is the irq output something that PL011State
> needs to handle itself whereas the chardev backend is
> something we can pass into PL011Registers ?

Just because the IRQ update is needed in many places and the chardev
backend only in one place.

> In the C version, we just call pl011_update() where we
> need to; we could validly call it unconditionally for any
> write, we're just being (possibly prematurely) efficient
> by avoiding a call when we happen to know that the register
> write didn't touch any of the state that pl011_update()
> cares about. So it feels a bit odd to me that in the Rust
> version this "we happen to know that sometimes it would be
> unnecessary to call the update function" has been kind of
> promoted to being part of an interface between the two
> different types PL011Registers and PL011State.

Yeah, if I was writing from scratch I would probably call update()
unconditionally. If it turns out to be inefficient you could cache the
current value of

        let flags = regs.int_level & regs.int_enabled;

in PL011State as a BqlCell.

> Thinking about other devices, presumably for more complex
> devices we might need to pass more than just a single 'bool'
> back from PL011Registers::write. What other kinds of thing
> might we need to do in the FooState function, and (since
> the pl011 code is presumably going to be used as a template
> for those other devices) is it worth having something that
> expresses that better than just a raw 'bool' return ?

Ideally nothing, especially considering that more modern devices have
edge-triggered interrupts like MSIs, instead of level-triggered
interrupts that need qemu_set_irq() calls. But if you have something a
lot more complex than a bool I would pass down the PL011State and do
something like pl011.schedule_update_irq() which updates a BqlCell<>.
The device could then use a bottom half or process them after
"drop(regs)".

HPET has another approach, which is to store a backpointer from
HPETTimer to the HPETState, so that it can do

    self.get_state().irqs[route].pulse();

without passing down anything. The reason for this is that it has
multiple timers on the same routine, and it assigns the timers to
separate HPETTimers. I would not use it for PL011 because all accesses
to the PL011Registers go through the PL011State.

A while ago I checked how OpenVMM does it, and basically it does not
have the PL011State/PL011Registers separation at all: the devices are
automatically wrapped with a Mutex and memory accesses take a &mut.
That removes some of the complexity, but also a lot of flexibility.

Unfortunately, before being able to reason on how to make peace with
the limitations of safe Rust, it was necessary to spend a lot of time
writing API abstractions, otherwise you don't actually experience the
limitations. But that means that the number of lines of code in
qemu_api is not representative of my experience using it. :( I am
sorry this isn't a great answer yet; certainly some aspects of the
PL011 or HPET devices could be treated as a blueprint for future
devices, but which and how to document that is something where I would
like to consult with an actual Rust maven.

Paolo

Re: [PATCH 4/5] rust: pl011: switch to safe chardev operation

[Peter Maydell]

On Thu, 27 Feb 2025 at 18:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Feb 27, 2025 at 6:25 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > Thinking about other devices, presumably for more complex
> > devices we might need to pass more than just a single 'bool'
> > back from PL011Registers::write. What other kinds of thing
> > might we need to do in the FooState function, and (since
> > the pl011 code is presumably going to be used as a template
> > for those other devices) is it worth having something that
> > expresses that better than just a raw 'bool' return ?
>
> Ideally nothing, especially considering that more modern devices have
> edge-triggered interrupts like MSIs, instead of level-triggered
> interrupts that need qemu_set_irq() calls. But if you have something a
> lot more complex than a bool I would pass down the PL011State and do
> something like pl011.schedule_update_irq() which updates a BqlCell<>.
> The device could then use a bottom half or process them after
> "drop(regs)".
>
> HPET has another approach, which is to store a backpointer from
> HPETTimer to the HPETState, so that it can do
>
>     self.get_state().irqs[route].pulse();
>
> without passing down anything. The reason for this is that it has
> multiple timers on the same routine, and it assigns the timers to
> separate HPETTimers. I would not use it for PL011 because all accesses
> to the PL011Registers go through the PL011State.

I think the idea I vaguely have in the back of my mind is that
maybe it's a nice idea to have a coding structure that enforces
"you update your own internal state, and only then do things that
might involve calling out to the outside world, and if there's
something that you need to do with the result of that callout,
there's an easy mechanism for 'this is what I will want to do
next after that' continuations".

The fact that our C device implementations don't do that is
kind of the underlying cause of all the "recursive entry back
into the device via DMA" problems that we squashed with the
big hammer of "just forbid it entirely". It's also in a way
the problem underlying the race condition segfault here:
https://lore.kernel.org/qemu-devel/CAFEAcA9odnPo2LPip295Uztri7JfoVnQbkJ=Wn+k8dQneB_ynQ@mail.gmail.com/T/#u
(memory_region_snapshot_and_clear_dirty() drops the BKL, no
callers expect that, segfaults in the calling code in the
framebuffer device models if something else gets in and
e.g. resizes the framebuffer in the middle of a display update).

So I was sort of wondering if the pl011 structure was aiming
at providing that kind of separation of "internal state" and
"external interactions", such that the compiler would complain
if you tried to do an externally-interacting thing while your
internal state was not consistent.

thanks
-- PMM

[PATCH 5/5] rust: pl011: pass around registers::Data

[Paolo Bonzini]

The values stored in the Fifo are instances of the bitfield-struct
registers::Data.  Convert as soon as possible the value written
into DR, and always refer to the bitfield struct; it's generally
cleaner other than PL011State::receive having to do a double
conversion u8=>u32=>registers::Data.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 2283bae71eb..a679f6295f5 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -237,7 +237,7 @@ pub(self) fn write(
         match offset {
             DR => {
                 // interrupts always checked
-                let _ = self.loopback_tx(value);
+                let _ = self.loopback_tx(value.into());
                 self.int_level |= Interrupt::TX.0;
                 return true;
             }
@@ -304,7 +304,7 @@ pub(self) fn write(
 
     #[inline]
     #[must_use]
-    fn loopback_tx(&mut self, value: u32) -> bool {
+    fn loopback_tx(&mut self, value: registers::Data) -> bool {
         // Caveat:
         //
         // In real hardware, TX loopback happens at the serial-bit level
@@ -373,7 +373,7 @@ fn loopback_mdmctrl(&mut self) -> bool {
     }
 
     fn loopback_break(&mut self, enable: bool) -> bool {
-        enable && self.loopback_tx(registers::Data::BREAK.into())
+        enable && self.loopback_tx(registers::Data::BREAK)
     }
 
     fn set_read_trigger(&mut self) {
@@ -432,11 +432,11 @@ pub fn fifo_depth(&self) -> u32 {
     }
 
     #[must_use]
-    pub fn put_fifo(&mut self, value: u32) -> bool {
+    pub fn put_fifo(&mut self, value: registers::Data) -> bool {
         let depth = self.fifo_depth();
         assert!(depth > 0);
         let slot = (self.read_pos + self.read_count) & (depth - 1);
-        self.read_fifo[slot] = registers::Data::from(value);
+        self.read_fifo[slot] = value;
         self.read_count += 1;
         self.flags.set_receive_fifo_empty(false);
         if self.read_count == depth {
@@ -581,7 +581,8 @@ fn receive(&self, buf: &[u8]) {
             return;
         }
         let mut regs = self.regs.borrow_mut();
-        let update_irq = !regs.loopback_enabled() && regs.put_fifo(buf[0].into());
+        let c: u32 = buf[0].into();
+        let update_irq = !regs.loopback_enabled() && regs.put_fifo(c.into());
         // Release the BqlRefCell before calling self.update()
         drop(regs);
 
@@ -594,7 +595,7 @@ fn event(&self, event: Event) {
         let mut update_irq = false;
         let mut regs = self.regs.borrow_mut();
         if event == Event::CHR_EVENT_BREAK && !regs.loopback_enabled() {
-            update_irq = regs.put_fifo(registers::Data::BREAK.into());
+            update_irq = regs.put_fifo(registers::Data::BREAK);
         }
         // Release the BqlRefCell before calling self.update()
         drop(regs);
-- 
2.48.1