On Thu, 8 May 2025 at 09:31, Paolo Bonzini <pbonz...@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 <pbonz...@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
