[PATCH 0/2] rust: pl011: really use RX FIFO depth

[PATCH 0/2] rust: pl011: really use RX FIFO depth

[Paolo Bonzini]

Forward port a couple more commits from the C version.

Paolo

Paolo Bonzini (2):
  rust: pl011: Rename RX FIFO methods
  rust: pl011: Really use RX FIFO depth

 docs/devel/rust.rst              |  2 +-
 rust/hw/char/pl011/src/device.rs | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.49.0

[PATCH 1/2] rust: pl011: Rename RX FIFO methods

[Paolo Bonzini]

In preparation of having a TX FIFO, rename the RX FIFO methods.

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

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 7c563ade9cd..94b31659849 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -329,7 +329,7 @@ fn loopback_tx(&mut self, value: registers::Data) -> bool {
         // hardware flow-control is enabled.
         //
         // For simplicity, the above described is not emulated.
-        self.loopback_enabled() && self.put_fifo(value)
+        self.loopback_enabled() && self.fifo_rx_put(value)
     }
 
     #[must_use]
@@ -439,7 +439,7 @@ pub fn fifo_depth(&self) -> u32 {
     }
 
     #[must_use]
-    pub fn put_fifo(&mut self, value: registers::Data) -> bool {
+    pub fn fifo_rx_put(&mut self, value: registers::Data) -> bool {
         let depth = self.fifo_depth();
         assert!(depth > 0);
         let slot = (self.read_pos + self.read_count) & (depth - 1);
@@ -589,7 +589,7 @@ fn receive(&self, buf: &[u8]) {
         }
         let mut regs = self.regs.borrow_mut();
         let c: u32 = buf[0].into();
-        let update_irq = !regs.loopback_enabled() && regs.put_fifo(c.into());
+        let update_irq = !regs.loopback_enabled() && regs.fifo_rx_put(c.into());
         // Release the BqlRefCell before calling self.update()
         drop(regs);
 
@@ -602,7 +602,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);
+            update_irq = regs.fifo_rx_put(registers::Data::BREAK);
         }
         // Release the BqlRefCell before calling self.update()
         drop(regs);
-- 
2.49.0

Re: [PATCH 1/2] rust: pl011: Rename RX FIFO methods

[Philippe Mathieu-Daudé]

On 8/5/25 10:29, Paolo Bonzini wrote:
> In preparation of having a TX FIFO, rename the RX FIFO methods.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

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

Re: [PATCH 1/2] rust: pl011: Rename RX FIFO methods

[Peter Maydell]

On Thu, 8 May 2025 at 09:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> In preparation of having a TX FIFO, rename the RX FIFO methods.

Is it worth mentioning in the commit messages of these
two patches which the C commit is that they're matching?
For instance this one is 40871ca758cf ("hw/char/pl011:
Rename RX FIFO methods").

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/2] rust: pl011: Really use RX FIFO depth

[Paolo Bonzini]

While we model a 16-elements RX FIFO since the PL011 model was
introduced in commit cdbdb648b7c ("ARM Versatile Platform Baseboard
emulation"), we only read 1 char at a time!

Have the IOCanReadHandler handler return how many elements are
available, and use that in the IOReadHandler handler.

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

diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 4de86375021..171d908e0b0 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -119,7 +119,7 @@ QEMU includes four crates:
   for the ``hw/char/pl011.c`` and ``hw/timer/hpet.c`` files.
 
 .. [#issues] The ``pl011`` crate is synchronized with ``hw/char/pl011.c``
-   as of commit 02b1f7f61928.  The ``hpet`` crate is synchronized as of
+   as of commit 3e0f118f82.  The ``hpet`` crate is synchronized as of
    commit 1433e38cc8.  Both are lacking tracing functionality.
 
 This section explains how to work with them.
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 94b31659849..7f28bb25a9b 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -580,19 +580,19 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
     fn can_receive(&self) -> u32 {
         let regs = self.regs.borrow();
         // trace_pl011_can_receive(s->lcr, s->read_count, r);
-        u32::from(regs.read_count < regs.fifo_depth())
+        regs.fifo_depth() - regs.read_count
     }
 
     fn receive(&self, buf: &[u8]) {
-        if buf.is_empty() {
-            return;
-        }
         let mut regs = self.regs.borrow_mut();
-        let c: u32 = buf[0].into();
-        let update_irq = !regs.loopback_enabled() && regs.fifo_rx_put(c.into());
+        let mut update_irq = false;
+        for &c in buf {
+            let c: u32 = c.into();
+            update_irq |= !regs.loopback_enabled() && regs.fifo_rx_put(c.into());
+        }
+
         // Release the BqlRefCell before calling self.update()
         drop(regs);
-
         if update_irq {
             self.update();
         }
-- 
2.49.0

Re: [PATCH 2/2] rust: pl011: Really use RX FIFO depth

[Philippe Mathieu-Daudé]

On 8/5/25 10:29, Paolo Bonzini wrote:
> While we model a 16-elements RX FIFO since the PL011 model was
> introduced in commit cdbdb648b7c ("ARM Versatile Platform Baseboard
> emulation"), we only read 1 char at a time!
> 

This comment ...

> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.

... is not relevant, otherwise:

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

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

Re: [PATCH 2/2] rust: pl011: Really use RX FIFO depth

[Peter Maydell]

On Thu, 8 May 2025 at 09:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> While we model a 16-elements RX FIFO since the PL011 model was
> introduced in commit cdbdb648b7c ("ARM Versatile Platform Baseboard
> emulation"), we only read 1 char at a time!

We could say that this is the Rust version of the C implementation's
commit 3e0f118f8259 ("hw/char/pl011: Really use RX FIFO depth").

> Have the IOCanReadHandler handler return how many elements are
> available, and use that in the IOReadHandler handler.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  docs/devel/rust.rst              |  2 +-
>  rust/hw/char/pl011/src/device.rs | 14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
> index 4de86375021..171d908e0b0 100644
> --- a/docs/devel/rust.rst
> +++ b/docs/devel/rust.rst
> @@ -119,7 +119,7 @@ QEMU includes four crates:
>    for the ``hw/char/pl011.c`` and ``hw/timer/hpet.c`` files.
>
>  .. [#issues] The ``pl011`` crate is synchronized with ``hw/char/pl011.c``
> -   as of commit 02b1f7f61928.  The ``hpet`` crate is synchronized as of
> +   as of commit 3e0f118f82.  The ``hpet`` crate is synchronized as of
>     commit 1433e38cc8.  Both are lacking tracing functionality.
>
>  This section explains how to work with them.
> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 94b31659849..7f28bb25a9b 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -580,19 +580,19 @@ fn write(&self, offset: hwaddr, value: u64, _size: u32) {
>      fn can_receive(&self) -> u32 {
>          let regs = self.regs.borrow();
>          // trace_pl011_can_receive(s->lcr, s->read_count, r);

This commented out tracepoint needs adjusting: the C version
is now
  trace_pl011_can_receive(s->lcr, s->read_count, fifo_depth, fifo_available)
where fifo_available is what we here open-code as
"regs.fifo_depth() - regs.read_count" in the return value.

I mention this because it affects whether you want to write
this open-coded or with a local variable.

> -        u32::from(regs.read_count < regs.fifo_depth())
> +        regs.fifo_depth() - regs.read_count
>      }
>
>      fn receive(&self, buf: &[u8]) {
> -        if buf.is_empty() {
> -            return;
> -        }
>          let mut regs = self.regs.borrow_mut();
> -        let c: u32 = buf[0].into();
> -        let update_irq = !regs.loopback_enabled() && regs.fifo_rx_put(c.into());
> +        let mut update_irq = false;
> +        for &c in buf {
> +            let c: u32 = c.into();
> +            update_irq |= !regs.loopback_enabled() && regs.fifo_rx_put(c.into());
> +        }

We're now checking loopback_enabled() on every iteration
(admittedly we previously also checked it for every character
but with the extra overhead of calling receive() too ;-))

Could we write this the same way the C code does with an
early-return before the for loop?

Also, the C version has a helpful comment about that:
    /*
     * In loopback mode, the RX input signal is internally disconnected
     * from the entire receiving logics; thus, all inputs are ignored,
     * and BREAK detection on RX input signal is also not performed.
     */

that it would be good to have in the Rust implementation so
we don't lose it as and when we drop the C impl.

> +
>          // Release the BqlRefCell before calling self.update()
>          drop(regs);
> -
>          if update_irq {
>              self.update();
>          }

thanks
-- PMM