[PATCH 0/7] rust: pl011: bug fixes

[PATCH 0/7] rust: pl011: bug fixes

[Paolo Bonzini]

While preparing the comparison at
https://lists.nongnu.org/archive/html/qemu-rust/2024-12/msg00006.html,
I noticed some bugs in the code.  These are the corresponding fixes.

CCing Philippe because he expressed interest in bringing the Rust
version on par with the C version of the pl011 device.

Thanks,

Paolo

Paolo Bonzini (7):
  rust: pl011: fix declaration of LineControl bits
  rust: pl011: match break logic of C version
  rust: pl011: always use reset() method on registers
  rust: pl011: fix break errors and definition of Data struct
  rust: pl011: extend registers to 32 bits
  rust: pl011: fix migration stream
  rust: pl011: simplify handling of the FIFO enabled bit in LCR

 rust/hw/char/pl011/src/device.rs       | 119 +++++++++++-------
 rust/hw/char/pl011/src/device_class.rs |   8 +-
 rust/hw/char/pl011/src/lib.rs          | 161 +++++++++++++------------
 rust/qemu-api/src/vmstate.rs           |  22 ----
 4 files changed, 160 insertions(+), 150 deletions(-)

-- 
2.47.1

[PATCH 1/7] rust: pl011: fix declaration of LineControl bits

[Paolo Bonzini]

The bits in the LineControl struct were backwards. :(

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

diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 4dc0e8f345f..d5089f78854 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -319,32 +319,21 @@ fn default() -> Self {
     /// Line Control Register, `UARTLCR_H`
     #[doc(alias = "UARTLCR_H")]
     pub struct LineControl {
-        /// 15:8 - Reserved, do not modify, read as zero.
-        _reserved_zero_no_modify: u8,
-        /// 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,
-        /// 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,
-        /// 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,
-        /// 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,
+        /// 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
@@ -355,21 +344,32 @@ pub struct LineControl {
         /// and generation. See Table 3-11 on page 3-14 for the parity
         /// truth table.
         pub parity: Parity,
-        /// 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,
-        /// 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,
+        /// 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,
+        /// 15:8 - Reserved, do not modify, read as zero.
+        _reserved_zero_no_modify: u8,
     }
 
     impl LineControl {
-- 
2.47.1

[PATCH 2/7] rust: pl011: match break logic of C version

[Paolo Bonzini]

Check loopback_enabled(), not fifo_enabled(), like the C code.

Also, set_break_error() must not happen until the break is read from
the FIFO.

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

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 41220c99a83..c6a8dbe1af4 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -465,9 +465,8 @@ pub fn can_receive(&self) -> bool {
     }
 
     pub fn event(&mut self, event: QEMUChrEvent) {
-        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.fifo_enabled() {
+        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
             self.put_fifo(DATA_BREAK);
-            self.receive_status_error_clear.set_break_error(true);
         }
     }
 
-- 
2.47.1

[PATCH 3/7] rust: pl011: always use reset() method on registers

[Paolo Bonzini]

For CR, the ugly-ish "0.into()" idiom is already hidden within the
Default trait.  Do not repeat it.

For FR, standardize on reset() being equivalent to "*self = Self::default()"
and let reset_fifo toggle only the bits that are related to FIFOs.  This
commit also reproduces C commit 02b1f7f6192 ("hw/char/pl011: Split RX/TX
path of pl011_reset_fifo()", 2024-09-13).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 23 ++++++++++++++++-------
 rust/hw/char/pl011/src/lib.rs    | 13 +++++--------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index c6a8dbe1af4..eb15e9d5788 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -267,7 +267,7 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
                 self.update();
             }
             Ok(RSR) => {
-                self.receive_status_error_clear = 0.into();
+                self.receive_status_error_clear.reset();
             }
             Ok(FR) => {
                 // flag writes are ignored
@@ -288,7 +288,8 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
                 if bool::from(self.line_control.fifos_enabled())
                     ^ bool::from(new_val.fifos_enabled())
                 {
-                    self.reset_fifo();
+                    self.reset_rx_fifo();
+                    self.reset_tx_fifo();
                 }
                 if self.line_control.send_break() ^ new_val.send_break() {
                     let mut break_enable: c_int = new_val.send_break().into();
@@ -447,16 +448,24 @@ pub fn reset(&mut self) {
         self.read_trigger = 1;
         self.ifl = 0x12;
         self.control.reset();
-        self.flags = 0.into();
-        self.reset_fifo();
+        self.flags.reset();
+        self.reset_rx_fifo();
+        self.reset_tx_fifo();
     }
 
-    pub fn reset_fifo(&mut self) {
+    pub fn reset_rx_fifo(&mut self) {
         self.read_count = 0;
         self.read_pos = 0;
 
-        /* Reset FIFO flags */
-        self.flags.reset();
+        // Reset FIFO flags
+        self.flags.set_receive_fifo_full(false);
+        self.flags.set_receive_fifo_empty(true);
+    }
+
+    pub fn reset_tx_fifo(&mut self) {
+        // Reset FIFO flags
+        self.flags.set_transmit_fifo_full(false);
+        self.flags.set_transmit_fifo_empty(true);
     }
 
     pub fn can_receive(&self) -> bool {
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index d5089f78854..e3eacb0e6b9 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -230,7 +230,7 @@ pub struct ReceiveStatusErrorClear {
     impl ReceiveStatusErrorClear {
         pub fn reset(&mut self) {
             // All the bits are cleared to 0 on reset.
-            *self = 0.into();
+            *self = Self::default();
         }
     }
 
@@ -297,19 +297,16 @@ pub struct Flags {
 
     impl Flags {
         pub fn reset(&mut self) {
-            // After reset TXFF, RXFF, and BUSY are 0, and TXFE and RXFE are 1
-            self.set_receive_fifo_full(false);
-            self.set_transmit_fifo_full(false);
-            self.set_busy(false);
-            self.set_receive_fifo_empty(true);
-            self.set_transmit_fifo_empty(true);
+            *self = Self::default();
         }
     }
 
     impl Default for Flags {
         fn default() -> Self {
             let mut ret: Self = 0.into();
-            ret.reset();
+            // 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
         }
     }
-- 
2.47.1

[PATCH 4/7] rust: pl011: fix break errors and definition of Data struct

[Paolo Bonzini]

The Data struct is wrong, and does not show how bits 8-15 of DR
are the receive status.  Fix it, and use it to fix break
errors ("c >> 8" in the C code does not translate to
"c.to_be_bytes()[3]").

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 15 ++++++------
 rust/hw/char/pl011/src/lib.rs    | 41 ++++++++++++++++++++++----------
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index eb15e9d5788..e88ea24b00e 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -30,8 +30,6 @@
 /// Fractional Baud Rate Divider, `UARTFBRD`
 const FBRD_MASK: u32 = 0x3f;
 
-const DATA_BREAK: u32 = 1 << 10;
-
 /// QEMU sourced constant.
 pub const PL011_FIFO_DEPTH: usize = 16_usize;
 
@@ -68,7 +66,7 @@ pub struct PL011State {
     pub dmacr: u32,
     pub int_enabled: u32,
     pub int_level: u32,
-    pub read_fifo: [u32; PL011_FIFO_DEPTH],
+    pub read_fifo: [registers::Data; PL011_FIFO_DEPTH],
     pub ilpr: u32,
     pub ibrd: u32,
     pub fbrd: u32,
@@ -215,10 +213,11 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
                     self.int_level &= !registers::INT_RX;
                 }
                 // Update error bits.
-                self.receive_status_error_clear = c.to_be_bytes()[3].into();
+                self.receive_status_error_clear.set_from_data(c);
                 self.update();
                 // Must call qemu_chr_fe_accept_input, so return Continue:
-                return std::ops::ControlFlow::Continue(c.into());
+                let c = u32::from(c);
+                return std::ops::ControlFlow::Continue(u64::from(c));
             }
             Ok(RSR) => u8::from(self.receive_status_error_clear).into(),
             Ok(FR) => u16::from(self.flags).into(),
@@ -411,7 +410,7 @@ fn loopback_mdmctrl(&mut self) {
 
     fn loopback_break(&mut self, enable: bool) {
         if enable {
-            self.loopback_tx(DATA_BREAK);
+            self.loopback_tx(registers::Data::BREAK.into());
         }
     }
 
@@ -475,7 +474,7 @@ pub fn can_receive(&self) -> bool {
 
     pub fn event(&mut self, event: QEMUChrEvent) {
         if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
-            self.put_fifo(DATA_BREAK);
+            self.put_fifo(registers::Data::BREAK.into());
         }
     }
 
@@ -502,7 +501,7 @@ pub fn put_fifo(&mut self, value: c_uint) {
         let depth = self.fifo_depth();
         assert!(depth > 0);
         let slot = (self.read_pos + self.read_count) & (depth - 1);
-        self.read_fifo[slot] = value;
+        self.read_fifo[slot] = registers::Data::from(value);
         self.read_count += 1;
         self.flags.set_receive_fifo_empty(false);
         if self.read_count == depth {
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index e3eacb0e6b9..463ae60543b 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -139,6 +139,21 @@ pub mod registers {
     //! unused thus treated as zero when read or written.
     use bilge::prelude::*;
 
+    /// 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`
     ///
@@ -181,16 +196,18 @@ pub mod registers {
     ///
     /// # Source
     /// ARM DDI 0183G 3.3.1 Data Register, UARTDR
-    #[bitsize(16)]
-    #[derive(Clone, Copy, DebugBits, FromBits)]
+    #[bitsize(32)]
+    #[derive(Clone, Copy, Default, DebugBits, FromBits)]
     #[doc(alias = "UARTDR")]
     pub struct Data {
-        _reserved: u4,
         pub data: u8,
-        pub framing_error: bool,
-        pub parity_error: bool,
-        pub break_error: bool,
-        pub overrun_error: bool,
+        pub errors: Errors,
+        _reserved: u16,
+    }
+
+    impl Data {
+        // bilge is not very const-friendly, unfortunately
+        pub const BREAK: Self = Self { value: 1 << 10 };
     }
 
     // TODO: FIFO Mode has different semantics
@@ -220,14 +237,14 @@ pub struct Data {
     #[bitsize(8)]
     #[derive(Clone, Copy, DebugBits, FromBits)]
     pub struct ReceiveStatusErrorClear {
-        pub framing_error: bool,
-        pub parity_error: bool,
-        pub break_error: bool,
-        pub overrun_error: bool,
-        _reserved_unpredictable: u4,
+        pub errors: Errors,
     }
 
     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();
-- 
2.47.1

[PATCH 5/7] rust: pl011: extend registers to 32 bits

[Paolo Bonzini]

The PL011 Technical Reference Manual lists the "real" size of the
registers in table 3-1, and only rounds up to the next byte when
describing the registers; for example, UARTDR is listed as having
width 12/8 (12 bits read, 8 written) and only bits 15:0 are listed
in "Table 3-2 UARTDR Register".

However, in practice these are 32-bit registers, accessible only
through 32-bit MMIO accesses; preserving the fiction that they're
smaller introduces multiple casts (to go from the bilge bitfield
type to e.g u16 to u64) and more importantly it breaks the
migration stream (though only on big-endian machines) because
the Rust vmstate macros are not yet type safe.

So, just make everything 32-bits wide.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 36 ++++++++++++++------------------
 rust/hw/char/pl011/src/lib.rs    | 23 +++++++++-----------
 2 files changed, 26 insertions(+), 33 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index e88ea24b00e..332e0a31a82 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -190,10 +190,10 @@ unsafe fn init(&mut self) {
     pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
         use RegisterOffset::*;
 
-        std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
+        let value = match RegisterOffset::try_from(offset) {
             Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
                 let device_id = self.get_class().device_id;
-                u64::from(device_id[(offset - 0xfe0) >> 2])
+                u32::from(device_id[(offset - 0xfe0) >> 2])
             }
             Err(_) => {
                 // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
@@ -219,27 +219,25 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
                 let c = u32::from(c);
                 return std::ops::ControlFlow::Continue(u64::from(c));
             }
-            Ok(RSR) => u8::from(self.receive_status_error_clear).into(),
-            Ok(FR) => u16::from(self.flags).into(),
-            Ok(FBRD) => self.fbrd.into(),
-            Ok(ILPR) => self.ilpr.into(),
-            Ok(IBRD) => self.ibrd.into(),
-            Ok(LCR_H) => u16::from(self.line_control).into(),
-            Ok(CR) => {
-                // We exercise our self-control.
-                u16::from(self.control).into()
-            }
-            Ok(FLS) => self.ifl.into(),
-            Ok(IMSC) => self.int_enabled.into(),
-            Ok(RIS) => self.int_level.into(),
-            Ok(MIS) => u64::from(self.int_level & self.int_enabled),
+            Ok(RSR) => u32::from(self.receive_status_error_clear),
+            Ok(FR) => u32::from(self.flags),
+            Ok(FBRD) => self.fbrd,
+            Ok(ILPR) => self.ilpr,
+            Ok(IBRD) => self.ibrd,
+            Ok(LCR_H) => u32::from(self.line_control),
+            Ok(CR) => u32::from(self.control),
+            Ok(FLS) => self.ifl,
+            Ok(IMSC) => self.int_enabled,
+            Ok(RIS) => self.int_level,
+            Ok(MIS) => self.int_level & self.int_enabled,
             Ok(ICR) => {
                 // "The UARTICR Register is the interrupt clear register and is write-only"
                 // Source: ARM DDI 0183G 3.3.13 Interrupt Clear Register, UARTICR
                 0
             }
-            Ok(DMACR) => self.dmacr.into(),
-        })
+            Ok(DMACR) => self.dmacr,
+        };
+        std::ops::ControlFlow::Break(value.into())
     }
 
     pub fn write(&mut self, offset: hwaddr, value: u64) {
@@ -281,7 +279,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
                 self.fbrd = value;
             }
             Ok(LCR_H) => {
-                let value = value as u16;
                 let new_val: registers::LineControl = value.into();
                 // Reset the FIFO state on FIFO enable or disable
                 if bool::from(self.line_control.fifos_enabled())
@@ -308,7 +305,6 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
             }
             Ok(CR) => {
                 // ??? Need to implement the enable bit.
-                let value = value as u16;
                 self.control = value.into();
                 self.loopback_mdmctrl();
             }
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 463ae60543b..0747e130cae 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -131,12 +131,6 @@ const fn _assert_exhaustive(val: RegisterOffset) {
 pub mod registers {
     //! Device registers exposed as typed structs which are backed by arbitrary
     //! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
-    //!
-    //! All PL011 registers are essentially 32-bit wide, but are typed here as
-    //! bitmaps with only the necessary width. That is, if a struct bitmap
-    //! in this module is for example 16 bits long, it should be conceived
-    //! as a 32-bit register where the unmentioned higher bits are always
-    //! unused thus treated as zero when read or written.
     use bilge::prelude::*;
 
     /// Receive Status Register / Data Register common error bits
@@ -234,10 +228,11 @@ impl Data {
     /// # Source
     /// ARM DDI 0183G 3.3.2 Receive Status Register/Error Clear Register,
     /// UARTRSR/UARTECR
-    #[bitsize(8)]
+    #[bitsize(32)]
     #[derive(Clone, Copy, DebugBits, FromBits)]
     pub struct ReceiveStatusErrorClear {
         pub errors: Errors,
+        _reserved_unpredictable: u24,
     }
 
     impl ReceiveStatusErrorClear {
@@ -257,7 +252,7 @@ fn default() -> Self {
         }
     }
 
-    #[bitsize(16)]
+    #[bitsize(32)]
     #[derive(Clone, Copy, DebugBits, FromBits)]
     /// Flag Register, `UARTFR`
     #[doc(alias = "UARTFR")]
@@ -309,7 +304,7 @@ pub struct Flags {
         pub transmit_fifo_empty: bool,
         /// `RI`, is `true` when `nUARTRI` is `LOW`.
         pub ring_indicator: bool,
-        _reserved_zero_no_modify: u7,
+        _reserved_zero_no_modify: u23,
     }
 
     impl Flags {
@@ -328,7 +323,7 @@ fn default() -> Self {
         }
     }
 
-    #[bitsize(16)]
+    #[bitsize(32)]
     #[derive(Clone, Copy, DebugBits, FromBits)]
     /// Line Control Register, `UARTLCR_H`
     #[doc(alias = "UARTLCR_H")]
@@ -382,8 +377,8 @@ pub struct LineControl {
         /// 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,
-        /// 15:8 - Reserved, do not modify, read as zero.
-        _reserved_zero_no_modify: u8,
+        /// 31:8 - Reserved, do not modify, read as zero.
+        _reserved_zero_no_modify: u24,
     }
 
     impl LineControl {
@@ -454,7 +449,7 @@ pub enum WordLength {
     ///
     /// # Source
     /// ARM DDI 0183G, 3.3.8 Control Register, `UARTCR`, Table 3-12
-    #[bitsize(16)]
+    #[bitsize(32)]
     #[doc(alias = "UARTCR")]
     #[derive(Clone, Copy, DebugBits, FromBits)]
     pub struct Control {
@@ -532,6 +527,8 @@ pub struct Control {
         /// 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 Control {
-- 
2.47.1

[PATCH 6/7] rust: pl011: fix migration stream

[Paolo Bonzini]

The Rust vmstate macros lack the type-safety of their C equivalents (so
safe, much abstraction), and therefore they were predictably wrong.

The registers have already been changed to 32-bits in the previous patch,
but read_pos/read_count/read_trigger also have to be u32 instead of usize.
The easiest way to do so is to let the FIFO use u32 indices instead
of usize.

My plan for making VMStateField typesafe is to have a trait to retrieve
a basic VMStateField; for example something like vmstate_uint32 would
become an implementation of the VMState trait on u32.  Then you'd write
something like "vmstate_of!(Type, field).with_version_id(2)".  That is,
vmstate_of retrieves the basic VMStateField and fills in the offset,
and then more changes can be applied on top.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs       | 38 ++++++++++++++++++++++----
 rust/hw/char/pl011/src/device_class.rs |  8 +++---
 rust/qemu-api/src/vmstate.rs           | 22 ---------------
 3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 332e0a31a82..cfe2734703e 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -31,7 +31,7 @@
 const FBRD_MASK: u32 = 0x3f;
 
 /// QEMU sourced constant.
-pub const PL011_FIFO_DEPTH: usize = 16_usize;
+pub const PL011_FIFO_DEPTH: u32 = 16;
 
 #[derive(Clone, Copy)]
 struct DeviceId(&'static [u8; 8]);
@@ -49,6 +49,32 @@ impl DeviceId {
     const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
 }
 
+// FIFOs use 32-bit indices instead of usize, for compatibility with
+// the migration stream produced by the C version of this device.
+#[repr(transparent)]
+#[derive(Debug, Default)]
+pub struct Fifo([registers::Data; PL011_FIFO_DEPTH as usize]);
+
+impl Fifo {
+    const fn len(&self) -> u32 {
+        self.0.len() as u32
+    }
+}
+
+impl std::ops::IndexMut<u32> for Fifo {
+    fn index_mut(&mut self, idx: u32) -> &mut Self::Output {
+        &mut self.0[idx as usize]
+    }
+}
+
+impl std::ops::Index<u32> for Fifo {
+    type Output = registers::Data;
+
+    fn index(&self, idx: u32) -> &Self::Output {
+        &self.0[idx as usize]
+    }
+}
+
 #[repr(C)]
 #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
 /// PL011 Device Model in QEMU
@@ -66,14 +92,14 @@ pub struct PL011State {
     pub dmacr: u32,
     pub int_enabled: u32,
     pub int_level: u32,
-    pub read_fifo: [registers::Data; PL011_FIFO_DEPTH],
+    pub read_fifo: Fifo,
     pub ilpr: u32,
     pub ibrd: u32,
     pub fbrd: u32,
     pub ifl: u32,
-    pub read_pos: usize,
-    pub read_count: usize,
-    pub read_trigger: usize,
+    pub read_pos: u32,
+    pub read_count: u32,
+    pub read_trigger: u32,
     #[doc(alias = "chr")]
     pub char_backend: CharBackend,
     /// QEMU interrupts
@@ -485,7 +511,7 @@ pub fn loopback_enabled(&self) -> bool {
     }
 
     #[inline]
-    pub fn fifo_depth(&self) -> usize {
+    pub fn fifo_depth(&self) -> u32 {
         // Note: FIFO depth is expected to be power-of-2
         if self.fifo_enabled() {
             return PL011_FIFO_DEPTH;
diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
index 975c3d42be7..759c521a99e 100644
--- a/rust/hw/char/pl011/src/device_class.rs
+++ b/rust/hw/char/pl011/src/device_class.rs
@@ -6,7 +6,7 @@
 use std::os::raw::{c_int, c_void};
 
 use qemu_api::{
-    bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections,
+    bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_subsections,
     vmstate_uint32, vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
 };
 
@@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
         vmstate_uint32!(ibrd, PL011State),
         vmstate_uint32!(fbrd, PL011State),
         vmstate_uint32!(ifl, PL011State),
-        vmstate_int32!(read_pos, PL011State),
-        vmstate_int32!(read_count, PL011State),
-        vmstate_int32!(read_trigger, PL011State),
+        vmstate_uint32!(read_pos, PL011State),
+        vmstate_uint32!(read_count, PL011State),
+        vmstate_uint32!(read_trigger, PL011State),
     },
     subsections: vmstate_subsections! {
         VMSTATE_PL011_CLOCK
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 25c68b703ea..63c897abcdf 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -106,28 +106,6 @@ macro_rules! vmstate_uint32 {
     }};
 }
 
-#[doc(alias = "VMSTATE_INT32_V")]
-#[macro_export]
-macro_rules! vmstate_int32_v {
-    ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
-        $crate::vmstate_single!(
-            $field_name,
-            $struct_name,
-            $version_id,
-            ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32),
-            ::core::mem::size_of::<i32>()
-        )
-    }};
-}
-
-#[doc(alias = "VMSTATE_INT32")]
-#[macro_export]
-macro_rules! vmstate_int32 {
-    ($field_name:ident, $struct_name:ty) => {{
-        $crate::vmstate_int32_v!($field_name, $struct_name, 0)
-    }};
-}
-
 #[doc(alias = "VMSTATE_ARRAY")]
 #[macro_export]
 macro_rules! vmstate_array {
-- 
2.47.1

[PATCH 7/7] rust: pl011: simplify handling of the FIFO enabled bit in LCR

[Paolo Bonzini]

Use ==/!= instead of going through bool and xor.

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

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index cfe2734703e..169ff3779c6 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -307,9 +307,7 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
             Ok(LCR_H) => {
                 let new_val: registers::LineControl = value.into();
                 // Reset the FIFO state on FIFO enable or disable
-                if bool::from(self.line_control.fifos_enabled())
-                    ^ bool::from(new_val.fifos_enabled())
-                {
+                if self.line_control.fifos_enabled() != new_val.fifos_enabled() {
                     self.reset_rx_fifo();
                     self.reset_tx_fifo();
                 }
@@ -502,7 +500,7 @@ pub fn event(&mut self, event: QEMUChrEvent) {
 
     #[inline]
     pub fn fifo_enabled(&self) -> bool {
-        matches!(self.line_control.fifos_enabled(), registers::Mode::FIFO)
+        self.line_control.fifos_enabled() == registers::Mode::FIFO
     }
 
     #[inline]
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 0747e130cae..69064d6929b 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -419,12 +419,6 @@ pub enum Mode {
         FIFO = 1,
     }
 
-    impl From<Mode> for bool {
-        fn from(val: Mode) -> Self {
-            matches!(val, Mode::FIFO)
-        }
-    }
-
     #[bitsize(2)]
     #[derive(Clone, Copy, Debug, Eq, FromBits, PartialEq)]
     /// `WLEN` Word length, field of [Line Control register](LineControl).
-- 
2.47.1

Re: [PATCH 0/7] rust: pl011: bug fixes

[Philippe Mathieu-Daudé]

On 12/12/24 18:21, Paolo Bonzini wrote:
> While preparing the comparison at
> https://lists.nongnu.org/archive/html/qemu-rust/2024-12/msg00006.html,
> I noticed some bugs in the code.  These are the corresponding fixes.
> 
> CCing Philippe because he expressed interest in bringing the Rust
> version on par with the C version of the pl011 device.

Also cc'ing Zhao.

> Paolo Bonzini (7):
>    rust: pl011: fix declaration of LineControl bits
>    rust: pl011: match break logic of C version
>    rust: pl011: always use reset() method on registers
>    rust: pl011: fix break errors and definition of Data struct
>    rust: pl011: extend registers to 32 bits
>    rust: pl011: fix migration stream
>    rust: pl011: simplify handling of the FIFO enabled bit in LCR
> 
>   rust/hw/char/pl011/src/device.rs       | 119 +++++++++++-------
>   rust/hw/char/pl011/src/device_class.rs |   8 +-
>   rust/hw/char/pl011/src/lib.rs          | 161 +++++++++++++------------
>   rust/qemu-api/src/vmstate.rs           |  22 ----
>   4 files changed, 160 insertions(+), 150 deletions(-)
> 

Re: [PATCH 2/7] rust: pl011: match break logic of C version

[Philippe Mathieu-Daudé]

On 12/12/24 18:21, Paolo Bonzini wrote:
> Check loopback_enabled(), not fifo_enabled(), like the C code.
> 
> Also, set_break_error() must not happen until the break is read from
> the FIFO.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 7/7] rust: pl011: simplify handling of the FIFO enabled bit in LCR

[Philippe Mathieu-Daudé]

On 12/12/24 18:22, Paolo Bonzini wrote:
> Use ==/!= instead of going through bool and xor.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 6 ++----
>   rust/hw/char/pl011/src/lib.rs    | 6 ------
>   2 files changed, 2 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/7] rust: pl011: fix declaration of LineControl bits

[Philippe Mathieu-Daudé]

On 12/12/24 18:21, Paolo Bonzini wrote:
> The bits in the LineControl struct were backwards. :(
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/lib.rs | 82 +++++++++++++++++------------------
>   1 file changed, 41 insertions(+), 41 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/7] rust: pl011: always use reset() method on registers

[Philippe Mathieu-Daudé]

On 12/12/24 18:22, Paolo Bonzini wrote:
> For CR, the ugly-ish "0.into()" idiom is already hidden within the
> Default trait.  Do not repeat it.
> 
> For FR, standardize on reset() being equivalent to "*self = Self::default()"
> and let reset_fifo toggle only the bits that are related to FIFOs.  This
> commit also reproduces C commit 02b1f7f6192 ("hw/char/pl011: Split RX/TX
> path of pl011_reset_fifo()", 2024-09-13).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 23 ++++++++++++++++-------
>   rust/hw/char/pl011/src/lib.rs    | 13 +++++--------
>   2 files changed, 21 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 5/7] rust: pl011: extend registers to 32 bits

[Philippe Mathieu-Daudé]

On 12/12/24 18:22, Paolo Bonzini wrote:
> The PL011 Technical Reference Manual lists the "real" size of the
> registers in table 3-1, and only rounds up to the next byte when
> describing the registers; for example, UARTDR is listed as having
> width 12/8 (12 bits read, 8 written) and only bits 15:0 are listed
> in "Table 3-2 UARTDR Register".
> 
> However, in practice these are 32-bit registers, accessible only
> through 32-bit MMIO accesses; preserving the fiction that they're
> smaller introduces multiple casts (to go from the bilge bitfield
> type to e.g u16 to u64) and more importantly it breaks the
> migration stream (though only on big-endian machines) because
> the Rust vmstate macros are not yet type safe.

This also better matches PL011_OPS::impl_.

> 
> So, just make everything 32-bits wide.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 36 ++++++++++++++------------------
>   rust/hw/char/pl011/src/lib.rs    | 23 +++++++++-----------
>   2 files changed, 26 insertions(+), 33 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/7] rust: pl011: fix break errors and definition of Data struct

[Philippe Mathieu-Daudé]

On 12/12/24 18:22, Paolo Bonzini wrote:
> The Data struct is wrong, and does not show how bits 8-15 of DR
> are the receive status.  Fix it, and use it to fix break
> errors ("c >> 8" in the C code does not translate to
> "c.to_be_bytes()[3]").
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 15 ++++++------
>   rust/hw/char/pl011/src/lib.rs    | 41 ++++++++++++++++++++++----------
>   2 files changed, 36 insertions(+), 20 deletions(-)


>       impl ReceiveStatusErrorClear {
> +        pub fn set_from_data(&mut self, data: Data) {
> +            self.set_errors(data.errors());

It took me some time to understand where is 'c >> 8'.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        }
> +

Re: [PATCH 6/7] rust: pl011: fix migration stream

[Philippe Mathieu-Daudé]

LGTM, still happily deferring to Alex =)

On 12/12/24 18:22, Paolo Bonzini wrote:
> The Rust vmstate macros lack the type-safety of their C equivalents (so
> safe, much abstraction), and therefore they were predictably wrong.
> 
> The registers have already been changed to 32-bits in the previous patch,
> but read_pos/read_count/read_trigger also have to be u32 instead of usize.
> The easiest way to do so is to let the FIFO use u32 indices instead
> of usize.
> 
> My plan for making VMStateField typesafe is to have a trait to retrieve
> a basic VMStateField; for example something like vmstate_uint32 would
> become an implementation of the VMState trait on u32.  Then you'd write
> something like "vmstate_of!(Type, field).with_version_id(2)".  That is,
> vmstate_of retrieves the basic VMStateField and fills in the offset,
> and then more changes can be applied on top.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs       | 38 ++++++++++++++++++++++----
>   rust/hw/char/pl011/src/device_class.rs |  8 +++---
>   rust/qemu-api/src/vmstate.rs           | 22 ---------------
>   3 files changed, 36 insertions(+), 32 deletions(-)
> 
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 332e0a31a82..cfe2734703e 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -31,7 +31,7 @@
>   const FBRD_MASK: u32 = 0x3f;
>   
>   /// QEMU sourced constant.
> -pub const PL011_FIFO_DEPTH: usize = 16_usize;
> +pub const PL011_FIFO_DEPTH: u32 = 16;
>   
>   #[derive(Clone, Copy)]
>   struct DeviceId(&'static [u8; 8]);
> @@ -49,6 +49,32 @@ impl DeviceId {
>       const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
>   }
>   
> +// FIFOs use 32-bit indices instead of usize, for compatibility with
> +// the migration stream produced by the C version of this device.
> +#[repr(transparent)]
> +#[derive(Debug, Default)]
> +pub struct Fifo([registers::Data; PL011_FIFO_DEPTH as usize]);
> +
> +impl Fifo {
> +    const fn len(&self) -> u32 {
> +        self.0.len() as u32
> +    }
> +}
> +
> +impl std::ops::IndexMut<u32> for Fifo {
> +    fn index_mut(&mut self, idx: u32) -> &mut Self::Output {
> +        &mut self.0[idx as usize]
> +    }
> +}
> +
> +impl std::ops::Index<u32> for Fifo {
> +    type Output = registers::Data;
> +
> +    fn index(&self, idx: u32) -> &Self::Output {
> +        &self.0[idx as usize]
> +    }
> +}
> +
>   #[repr(C)]
>   #[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
>   /// PL011 Device Model in QEMU
> @@ -66,14 +92,14 @@ pub struct PL011State {
>       pub dmacr: u32,
>       pub int_enabled: u32,
>       pub int_level: u32,
> -    pub read_fifo: [registers::Data; PL011_FIFO_DEPTH],
> +    pub read_fifo: Fifo,
>       pub ilpr: u32,
>       pub ibrd: u32,
>       pub fbrd: u32,
>       pub ifl: u32,
> -    pub read_pos: usize,
> -    pub read_count: usize,
> -    pub read_trigger: usize,
> +    pub read_pos: u32,
> +    pub read_count: u32,
> +    pub read_trigger: u32,
>       #[doc(alias = "chr")]
>       pub char_backend: CharBackend,
>       /// QEMU interrupts
> @@ -485,7 +511,7 @@ pub fn loopback_enabled(&self) -> bool {
>       }
>   
>       #[inline]
> -    pub fn fifo_depth(&self) -> usize {
> +    pub fn fifo_depth(&self) -> u32 {
>           // Note: FIFO depth is expected to be power-of-2
>           if self.fifo_enabled() {
>               return PL011_FIFO_DEPTH;
> diff --git a/rust/hw/char/pl011/src/device_class.rs b/rust/hw/char/pl011/src/device_class.rs
> index 975c3d42be7..759c521a99e 100644
> --- a/rust/hw/char/pl011/src/device_class.rs
> +++ b/rust/hw/char/pl011/src/device_class.rs
> @@ -6,7 +6,7 @@
>   use std::os::raw::{c_int, c_void};
>   
>   use qemu_api::{
> -    bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_int32, vmstate_subsections,
> +    bindings::*, c_str, vmstate_clock, vmstate_fields, vmstate_subsections,
>       vmstate_uint32, vmstate_uint32_array, vmstate_unused, zeroable::Zeroable,
>   };
>   
> @@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
>           vmstate_uint32!(ibrd, PL011State),
>           vmstate_uint32!(fbrd, PL011State),
>           vmstate_uint32!(ifl, PL011State),
> -        vmstate_int32!(read_pos, PL011State),
> -        vmstate_int32!(read_count, PL011State),
> -        vmstate_int32!(read_trigger, PL011State),
> +        vmstate_uint32!(read_pos, PL011State),
> +        vmstate_uint32!(read_count, PL011State),
> +        vmstate_uint32!(read_trigger, PL011State),
>       },
>       subsections: vmstate_subsections! {
>           VMSTATE_PL011_CLOCK
> diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
> index 25c68b703ea..63c897abcdf 100644
> --- a/rust/qemu-api/src/vmstate.rs
> +++ b/rust/qemu-api/src/vmstate.rs
> @@ -106,28 +106,6 @@ macro_rules! vmstate_uint32 {
>       }};
>   }
>   
> -#[doc(alias = "VMSTATE_INT32_V")]
> -#[macro_export]
> -macro_rules! vmstate_int32_v {
> -    ($field_name:ident, $struct_name:ty, $version_id:expr) => {{
> -        $crate::vmstate_single!(
> -            $field_name,
> -            $struct_name,
> -            $version_id,
> -            ::core::ptr::addr_of!($crate::bindings::vmstate_info_int32),
> -            ::core::mem::size_of::<i32>()
> -        )
> -    }};
> -}
> -
> -#[doc(alias = "VMSTATE_INT32")]
> -#[macro_export]
> -macro_rules! vmstate_int32 {
> -    ($field_name:ident, $struct_name:ty) => {{
> -        $crate::vmstate_int32_v!($field_name, $struct_name, 0)
> -    }};
> -}
> -
>   #[doc(alias = "VMSTATE_ARRAY")]
>   #[macro_export]
>   macro_rules! vmstate_array {

Re: [PATCH 1/7] rust: pl011: fix declaration of LineControl bits

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:21:58PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:21:58 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/7] rust: pl011: fix declaration of LineControl bits
> X-Mailer: git-send-email 2.47.1
> 
> The bits in the LineControl struct were backwards. :(
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/lib.rs | 82 +++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 2/7] rust: pl011: match break logic of C version

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:21:59PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:21:59 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/7] rust: pl011: match break logic of C version
> X-Mailer: git-send-email 2.47.1
> 
> Check loopback_enabled(), not fifo_enabled(), like the C code.
> 
> Also, set_break_error() must not happen until the break is read from
> the FIFO.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Now the logic here matches C version, so

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

But...

> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 41220c99a83..c6a8dbe1af4 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -465,9 +465,8 @@ pub fn can_receive(&self) -> bool {
>      }
>  
>      pub fn event(&mut self, event: QEMUChrEvent) {
> -        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.fifo_enabled() {
> +        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
>              self.put_fifo(DATA_BREAK);
> -            self.receive_status_error_clear.set_break_error(true);
>          }
>      }
>   

...but when I double-check where to set up the rsr, I realized that the
rust version and the C version seem to be inconsistent?

C:

static uint32_t pl011_read_rxdata(PL011State *s)
{
    ...
    c = s->read_fifo[s->read_pos];
    ...
    s->rsr = c >> 8;
    ...
}

Rust:

    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
        ...
	std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
            ...
	    Ok(DR) => {
                ...
		let c = self.read_fifo[self.read_pos];
                ...
		// Update error bits.
                self.receive_status_error_clear = c.to_be_bytes()[3].into();
                ...
	    }
        ...
    }

`to_be_bytes()` retures a byte array in big-endian byte order [*], so rust
accesses the first 8 bits and C accesses the last 24 bits, am I understanding
this correctly and which one is correct?

IIUC, I guess Rust should uses to_be_bytes()[2].

[*]: https://doc.rust-lang.org/std/primitive.u32.html#method.to_be_bytes

Regards,
Zhao

Re: [PATCH 2/7] rust: pl011: match break logic of C version

[Paolo Bonzini]

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

Il gio 19 dic 2024, 05:20 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> But when I double-check where to set up the rsr, I realized that the
> rust version and the C version seem to be inconsistent?
>
> IIUC, I guess Rust should uses to_be_bytes()[2].


Yes, that's patch 4 in this series. :)

Paolo

[*]: https://doc.rust-lang.org/std/primitive.u32.html#method.to_be_bytes
>
> Regards,
> Zhao
>
>
>

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

Re: [PATCH 3/7] rust: pl011: always use reset() method on registers

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:22:00PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:00 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 3/7] rust: pl011: always use reset() method on registers
> X-Mailer: git-send-email 2.47.1
> 
> For CR, the ugly-ish "0.into()" idiom is already hidden within the
> Default trait.  Do not repeat it.
> 
> For FR, standardize on reset() being equivalent to "*self = Self::default()"
> and let reset_fifo toggle only the bits that are related to FIFOs.  This
> commit also reproduces C commit 02b1f7f6192 ("hw/char/pl011: Split RX/TX
> path of pl011_reset_fifo()", 2024-09-13).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 23 ++++++++++++++++-------
>  rust/hw/char/pl011/src/lib.rs    | 13 +++++--------
>  2 files changed, 21 insertions(+), 15 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 4/7] rust: pl011: fix break errors and definition of Data struct

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:22:01PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:01 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/7] rust: pl011: fix break errors and definition of Data
>  struct
> X-Mailer: git-send-email 2.47.1
> 
> The Data struct is wrong, and does not show how bits 8-15 of DR
> are the receive status.  Fix it, and use it to fix break
> errors ("c >> 8" in the C code does not translate to
> "c.to_be_bytes()[3]").
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 15 ++++++------
>  rust/hw/char/pl011/src/lib.rs    | 41 ++++++++++++++++++++++----------
>  2 files changed, 36 insertions(+), 20 deletions(-)

>      impl ReceiveStatusErrorClear {
> +        pub fn set_from_data(&mut self, data: Data) {
> +            self.set_errors(data.errors());
> +        }
> +

This is the clear and clever way.

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 5/7] rust: pl011: extend registers to 32 bits

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:22:02PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:02 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 5/7] rust: pl011: extend registers to 32 bits
> X-Mailer: git-send-email 2.47.1
> 
> The PL011 Technical Reference Manual lists the "real" size of the
> registers in table 3-1, and only rounds up to the next byte when
> describing the registers; for example, UARTDR is listed as having
> width 12/8 (12 bits read, 8 written) and only bits 15:0 are listed
> in "Table 3-2 UARTDR Register".
> 
> However, in practice these are 32-bit registers, accessible only
> through 32-bit MMIO accesses; preserving the fiction that they're
> smaller introduces multiple casts (to go from the bilge bitfield
> type to e.g u16 to u64) and more importantly it breaks the
> migration stream (though only on big-endian machines) because
> the Rust vmstate macros are not yet type safe.
> 
> So, just make everything 32-bits wide.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 36 ++++++++++++++------------------
>  rust/hw/char/pl011/src/lib.rs    | 23 +++++++++-----------
>  2 files changed, 26 insertions(+), 33 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 6/7] rust: pl011: fix migration stream

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:22:03PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:03 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 6/7] rust: pl011: fix migration stream
> X-Mailer: git-send-email 2.47.1
> 
> The Rust vmstate macros lack the type-safety of their C equivalents (so
> safe, much abstraction), and therefore they were predictably wrong.

Yes, this makes Rust more unsafe than C code...

> The registers have already been changed to 32-bits in the previous patch,
> but read_pos/read_count/read_trigger also have to be u32 instead of usize.
> The easiest way to do so is to let the FIFO use u32 indices instead
> of usize.
> 
> My plan for making VMStateField typesafe is to have a trait to retrieve
> a basic VMStateField; for example something like vmstate_uint32 would
> become an implementation of the VMState trait on u32.  Then you'd write
> something like "vmstate_of!(Type, field).with_version_id(2)".  That is,
> vmstate_of retrieves the basic VMStateField and fills in the offset,
> and then more changes can be applied on top.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs       | 38 ++++++++++++++++++++++----
>  rust/hw/char/pl011/src/device_class.rs |  8 +++---
>  rust/qemu-api/src/vmstate.rs           | 22 ---------------
>  3 files changed, 36 insertions(+), 32 deletions(-)
> 
> @@ -64,9 +64,9 @@ extern "C" fn pl011_post_load(opaque: *mut c_void, version_id: c_int) -> c_int {
>          vmstate_uint32!(ibrd, PL011State),
>          vmstate_uint32!(fbrd, PL011State),
>          vmstate_uint32!(ifl, PL011State),
> -        vmstate_int32!(read_pos, PL011State),
> -        vmstate_int32!(read_count, PL011State),
> -        vmstate_int32!(read_trigger, PL011State),
> +        vmstate_uint32!(read_pos, PL011State),
> +        vmstate_uint32!(read_count, PL011State),
> +        vmstate_uint32!(read_trigger, PL011State),

uint32 and int32 types both use `qemu_put_be32s` and `qemu_get_be32s`
to save and store vmstate, so I think it's safe to convert
vmstate_int32! to vmstate_uint32! here.

>      },

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 7/7] rust: pl011: simplify handling of the FIFO enabled bit in LCR

[Zhao Liu]

On Thu, Dec 12, 2024 at 06:22:04PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:22:04 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 7/7] rust: pl011: simplify handling of the FIFO enabled bit
>  in LCR
> X-Mailer: git-send-email 2.47.1
> 
> Use ==/!= instead of going through bool and xor.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 6 ++----
>  rust/hw/char/pl011/src/lib.rs    | 6 ------
>  2 files changed, 2 insertions(+), 10 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>