[PATCH v5 00/16] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[PATCH v5 00/16] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

Hi,

This series add support for (async) FIFO on the transmit path
of the PL011 UART.

Last patch still broken (chars are emitted async),
still patches 1-11 could be merged for this release...

Since v4:
- Rebased (loopback)
- Addressed Richard & Juan migration comments
- Split in smaller patches

Since v3:
- Document migration bits (Alex, Richard)
- Just check FIFO is not empty in pl011_xmit_fifo_state_needed (rth)
- In pl011_xmit check TX enabled first, and ignore < 8-bit TX (rth)

Since v2:
- Added R-b tags
- Addressed Richard comments on migration

Since v1:
- Restrict pl011_ops[] impl access_size,
- Do not check transmitter is enabled (Peter),
- Addressed Alex's review comments,
- Simplified migration trying to care about backward compat,
  but still unsure...

Philippe Mathieu-Daudé (16):
  tests/avocado: Add 'device:pl011' tag to tests exercising PL011 UART
  hw/char/pl011: Remove unused 'readbuff' field
  hw/char/pl011: Move pl011_put_fifo() earlier
  hw/char/pl011: Move pl011_loopback_enabled|tx() around
  hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
  hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
  hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
  hw/char/pl011: Warn when using disabled transmitter
  tests/qtest: Update tests using PL011 UART
  hw/char/pl011: Check if receiver is enabled
  hw/char/pl011: Rename RX FIFO methods
  hw/char/pl011: Add transmit FIFO to PL011State
  hw/char/pl011: Introduce pl011_xmit() as GSource
  hw/char/pl011: Consider TX FIFO overrun error
  hw/char/pl011: Drain TX FIFO when no backend connected
  hw/char/pl011: Implement TX FIFO

 include/hw/char/pl011.h                  |   3 +-
 hw/char/pl011.c                          | 339 ++++++++++++++++-------
 tests/qtest/boot-serial-test.c           |  15 +-
 hw/char/trace-events                     |   9 +-
 tests/avocado/boot_linux.py              |   1 +
 tests/avocado/boot_linux_console.py      |   2 +
 tests/avocado/boot_xen.py                |   1 +
 tests/avocado/machine_aarch64_sbsaref.py |   1 +
 tests/avocado/machine_aarch64_virt.py    |   1 +
 tests/avocado/smmu.py                    |   1 +
 tests/avocado/tuxrun_baselines.py        |   5 +
 11 files changed, 277 insertions(+), 101 deletions(-)

-- 
2.41.0

[PATCH v5 01/16] tests/avocado: Add 'device:pl011' tag to tests exercising PL011 UART

[Philippe Mathieu-Daudé]

Add the 'device:pl011' tag to various tests using the
PL011 UART, so we can run them all at once using:

  $ make check-avocado AVOCADO_TAGS='device:pl011'

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/avocado/boot_linux.py              | 1 +
 tests/avocado/boot_linux_console.py      | 2 ++
 tests/avocado/boot_xen.py                | 1 +
 tests/avocado/machine_aarch64_sbsaref.py | 1 +
 tests/avocado/machine_aarch64_virt.py    | 1 +
 tests/avocado/smmu.py                    | 1 +
 tests/avocado/tuxrun_baselines.py        | 5 +++++
 7 files changed, 12 insertions(+)

diff --git a/tests/avocado/boot_linux.py b/tests/avocado/boot_linux.py
index df6cf209ef..6717f452eb 100644
--- a/tests/avocado/boot_linux.py
+++ b/tests/avocado/boot_linux.py
@@ -70,6 +70,7 @@ class BootLinuxAarch64(LinuxTest):
     def test_virt_kvm(self):
         """
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=accel:kvm
         :avocado: tags=cpu:host
         """
diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index c35fc5e9ba..f595324979 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -362,6 +362,7 @@ def test_arm_virt(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=accel:tcg
         """
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
@@ -1380,6 +1381,7 @@ def test_arm_vexpressa9(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:vexpress-a9
+        :avocado: tags=device:pl011
         """
         tar_hash = '32b7677ce8b6f1471fb0059865f451169934245b'
         self.vm.add_args('-dtb', self.workdir + '/day16/vexpress-v2p-ca9.dtb')
diff --git a/tests/avocado/boot_xen.py b/tests/avocado/boot_xen.py
index 93bfb0c161..c4c01afa76 100644
--- a/tests/avocado/boot_xen.py
+++ b/tests/avocado/boot_xen.py
@@ -66,6 +66,7 @@ class BootXen(BootXenBase):
         :avocado: tags=accel:tcg
         :avocado: tags=cpu:cortex-a57
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         """
 
     def test_arm64_xen_411_and_dom0(self):
diff --git a/tests/avocado/machine_aarch64_sbsaref.py b/tests/avocado/machine_aarch64_sbsaref.py
index e920bbf08c..f04ac2b11c 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -20,6 +20,7 @@ class Aarch64SbsarefMachine(QemuSystemTest):
     """
     :avocado: tags=arch:aarch64
     :avocado: tags=machine:sbsa-ref
+    :avocado: tags=device:pl011
     :avocado: tags=accel:tcg
 
     As firmware runs at a higher privilege level than the hypervisor we
diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
index 0ef6df4b0d..2d586c8459 100644
--- a/tests/avocado/machine_aarch64_virt.py
+++ b/tests/avocado/machine_aarch64_virt.py
@@ -23,6 +23,7 @@ class Aarch64VirtMachine(QemuSystemTest):
     """
     :avocado: tags=arch:aarch64
     :avocado: tags=machine:virt
+    :avocado: tags=device:pl011
     """
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
     timeout = 360
diff --git a/tests/avocado/smmu.py b/tests/avocado/smmu.py
index 4ebfa7128c..4b265e2435 100644
--- a/tests/avocado/smmu.py
+++ b/tests/avocado/smmu.py
@@ -21,6 +21,7 @@ class SMMU(LinuxTest):
     :avocado: tags=arch:aarch64
     :avocado: tags=machine:virt
     :avocado: tags=distro:fedora
+    :avocado: tags=device:pl011
     :avocado: tags=smmu
     :avocado: tags=flaky
     """
diff --git a/tests/avocado/tuxrun_baselines.py b/tests/avocado/tuxrun_baselines.py
index 736e4aa289..98ab40bbb5 100644
--- a/tests/avocado/tuxrun_baselines.py
+++ b/tests/avocado/tuxrun_baselines.py
@@ -254,6 +254,7 @@ def test_arm64(self):
         :avocado: tags=arch:aarch64
         :avocado: tags=cpu:cortex-a57
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=tuxboot:arm64
         :avocado: tags=console:ttyAMA0
         :avocado: tags=shutdown:nowait
@@ -270,6 +271,7 @@ def test_arm64be(self):
         :avocado: tags=cpu:cortex-a57
         :avocado: tags=endian:big
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=tuxboot:arm64be
         :avocado: tags=console:ttyAMA0
         :avocado: tags=shutdown:nowait
@@ -285,6 +287,7 @@ def test_armv5(self):
         :avocado: tags=arch:arm
         :avocado: tags=cpu:arm926
         :avocado: tags=machine:versatilepb
+        :avocado: tags=device:pl011
         :avocado: tags=tuxboot:armv5
         :avocado: tags=image:zImage
         :avocado: tags=console:ttyAMA0
@@ -306,6 +309,7 @@ def test_armv7(self):
         :avocado: tags=arch:arm
         :avocado: tags=cpu:cortex-a15
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=tuxboot:armv7
         :avocado: tags=image:zImage
         :avocado: tags=console:ttyAMA0
@@ -324,6 +328,7 @@ def test_armv7be(self):
         :avocado: tags=cpu:cortex-a15
         :avocado: tags=endian:big
         :avocado: tags=machine:virt
+        :avocado: tags=device:pl011
         :avocado: tags=tuxboot:armv7be
         :avocado: tags=image:zImage
         :avocado: tags=console:ttyAMA0
-- 
2.41.0

[PATCH v5 02/16] hw/char/pl011: Remove unused 'readbuff' field

[Philippe Mathieu-Daudé]

Since its introduction in commit cdbdb648b7 ("ARM Versatile
Platform Baseboard emulation.") PL011State::readbuff as never
been used. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/char/pl011.h | 1 -
 hw/char/pl011.c         | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index d853802132..4fcaf3d7d3 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -32,7 +32,6 @@ struct PL011State {
     SysBusDevice parent_obj;
 
     MemoryRegion iomem;
-    uint32_t readbuff;
     uint32_t flags;
     uint32_t lcr;
     uint32_t rsr;
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f8078aa216..260f5fc0bc 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -549,7 +549,7 @@ static const VMStateDescription vmstate_pl011 = {
     .minimum_version_id = 2,
     .post_load = pl011_post_load,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINT32(readbuff, PL011State),
+        VMSTATE_UNUSED(sizeof(uint32_t)),
         VMSTATE_UINT32(flags, PL011State),
         VMSTATE_UINT32(lcr, PL011State),
         VMSTATE_UINT32(rsr, PL011State),
-- 
2.41.0

Re: [PATCH v5 02/16] hw/char/pl011: Remove unused 'readbuff' field

[Peter Maydell]

On Fri, 19 Jul 2024 at 19:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Since its introduction in commit cdbdb648b7 ("ARM Versatile
> Platform Baseboard emulation.") PL011State::readbuff as never
> been used. Remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

thanks
-- PMM

[PATCH v5 03/16] hw/char/pl011: Move pl011_put_fifo() earlier

[Philippe Mathieu-Daudé]

Avoid forward-declaring pl011_put_fifo() by moving it earlier.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 260f5fc0bc..edb5395fb8 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -159,6 +159,28 @@ static inline void pl011_reset_fifo(PL011State *s)
     s->flags |= PL011_FLAG_RXFE | PL011_FLAG_TXFE;
 }
 
+static void pl011_put_fifo(void *opaque, uint32_t value)
+{
+    PL011State *s = (PL011State *)opaque;
+    int slot;
+    unsigned pipe_depth;
+
+    pipe_depth = pl011_get_fifo_depth(s);
+    slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
+    s->read_fifo[slot] = value;
+    s->read_count++;
+    s->flags &= ~PL011_FLAG_RXFE;
+    trace_pl011_put_fifo(value, s->read_count);
+    if (s->read_count == pipe_depth) {
+        trace_pl011_put_fifo_full();
+        s->flags |= PL011_FLAG_RXFF;
+    }
+    if (s->read_count == s->read_trigger) {
+        s->int_level |= INT_RX;
+        pl011_update(s);
+    }
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -314,8 +336,6 @@ static void pl011_loopback_mdmctrl(PL011State *s)
     pl011_update(s);
 }
 
-static void pl011_put_fifo(void *opaque, uint32_t value);
-
 static void pl011_loopback_tx(PL011State *s, uint32_t value)
 {
     if (!pl011_loopback_enabled(s)) {
@@ -440,28 +460,6 @@ static int pl011_can_receive(void *opaque)
     return r;
 }
 
-static void pl011_put_fifo(void *opaque, uint32_t value)
-{
-    PL011State *s = (PL011State *)opaque;
-    int slot;
-    unsigned pipe_depth;
-
-    pipe_depth = pl011_get_fifo_depth(s);
-    slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
-    s->read_fifo[slot] = value;
-    s->read_count++;
-    s->flags &= ~PL011_FLAG_RXFE;
-    trace_pl011_put_fifo(value, s->read_count);
-    if (s->read_count == pipe_depth) {
-        trace_pl011_put_fifo_full();
-        s->flags |= PL011_FLAG_RXFF;
-    }
-    if (s->read_count == s->read_trigger) {
-        s->int_level |= INT_RX;
-        pl011_update(s);
-    }
-}
-
 static void pl011_receive(void *opaque, const uint8_t *buf, int size)
 {
     /*
-- 
2.41.0

Re: [PATCH v5 03/16] hw/char/pl011: Move pl011_put_fifo() earlier

[Peter Maydell]

On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Avoid forward-declaring pl011_put_fifo() by moving it earlier.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

thanks
-- PMM

[PATCH v5 04/16] hw/char/pl011: Move pl011_loopback_enabled|tx() around

[Philippe Mathieu-Daudé]

We'll soon use pl011_loopback_enabled() and pl011_loopback_tx()
from functions defined before their declarations. In order to
avoid forward-declaring them, move them around.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c | 66 ++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index edb5395fb8..22195ead7b 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -138,6 +138,11 @@ static void pl011_update(PL011State *s)
     }
 }
 
+static bool pl011_loopback_enabled(PL011State *s)
+{
+    return !!(s->cr & CR_LBE);
+}
+
 static bool pl011_is_fifo_enabled(PL011State *s)
 {
     return (s->lcr & LCR_FEN) != 0;
@@ -181,6 +186,34 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
     }
 }
 
+static void pl011_loopback_tx(PL011State *s, uint32_t value)
+{
+    if (!pl011_loopback_enabled(s)) {
+        return;
+    }
+
+    /*
+     * Caveat:
+     *
+     * In real hardware, TX loopback happens at the serial-bit level
+     * and then reassembled by the RX logics back into bytes and placed
+     * into the RX fifo. That is, loopback happens after TX fifo.
+     *
+     * Because the real hardware TX fifo is time-drained at the frame
+     * rate governed by the configured serial format, some loopback
+     * bytes in TX fifo may still be able to get into the RX fifo
+     * that could be full at times while being drained at software
+     * pace.
+     *
+     * In such scenario, the RX draining pace is the major factor
+     * deciding which loopback bytes get into the RX fifo, unless
+     * hardware flow-control is enabled.
+     *
+     * For simplicity, the above described is not emulated.
+     */
+    pl011_put_fifo(s, value);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -290,11 +323,6 @@ static void pl011_trace_baudrate_change(const PL011State *s)
                                 s->ibrd, s->fbrd);
 }
 
-static bool pl011_loopback_enabled(PL011State *s)
-{
-    return !!(s->cr & CR_LBE);
-}
-
 static void pl011_loopback_mdmctrl(PL011State *s)
 {
     uint32_t cr, fr, il;
@@ -336,34 +364,6 @@ static void pl011_loopback_mdmctrl(PL011State *s)
     pl011_update(s);
 }
 
-static void pl011_loopback_tx(PL011State *s, uint32_t value)
-{
-    if (!pl011_loopback_enabled(s)) {
-        return;
-    }
-
-    /*
-     * Caveat:
-     *
-     * In real hardware, TX loopback happens at the serial-bit level
-     * and then reassembled by the RX logics back into bytes and placed
-     * into the RX fifo. That is, loopback happens after TX fifo.
-     *
-     * Because the real hardware TX fifo is time-drained at the frame
-     * rate governed by the configured serial format, some loopback
-     * bytes in TX fifo may still be able to get into the RX fifo
-     * that could be full at times while being drained at software
-     * pace.
-     *
-     * In such scenario, the RX draining pace is the major factor
-     * deciding which loopback bytes get into the RX fifo, unless
-     * hardware flow-control is enabled.
-     *
-     * For simplicity, the above described is not emulated.
-     */
-    pl011_put_fifo(s, value);
-}
-
 static void pl011_loopback_break(PL011State *s, int brk_enable)
 {
     if (brk_enable) {
-- 
2.41.0

Re: [PATCH v5 04/16] hw/char/pl011: Move pl011_loopback_enabled|tx() around

[Peter Maydell]

On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We'll soon use pl011_loopback_enabled() and pl011_loopback_tx()
> from functions defined before their declarations. In order to
> avoid forward-declaring them, move them around.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/char/pl011.c | 66 ++++++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)

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

thanks
-- PMM

[PATCH v5 05/16] hw/char/pl011: Split RX/TX path of pl011_reset_fifo()

[Philippe Mathieu-Daudé]

To be able to reset the RX or TX FIFO separately,
split pl011_reset_fifo() in two.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/char/pl011.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 22195ead7b..3d294c3b52 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -154,14 +154,21 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
     return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
 }
 
-static inline void pl011_reset_fifo(PL011State *s)
+static inline void pl011_reset_rx_fifo(PL011State *s)
 {
     s->read_count = 0;
     s->read_pos = 0;
 
     /* Reset FIFO flags */
-    s->flags &= ~(PL011_FLAG_RXFF | PL011_FLAG_TXFF);
-    s->flags |= PL011_FLAG_RXFE | PL011_FLAG_TXFE;
+    s->flags &= ~PL011_FLAG_RXFF;
+    s->flags |= PL011_FLAG_RXFE;
+}
+
+static inline void pl011_reset_tx_fifo(PL011State *s)
+{
+    /* Reset FIFO flags */
+    s->flags &= ~PL011_FLAG_TXFF;
+    s->flags |= PL011_FLAG_TXFE;
 }
 
 static void pl011_put_fifo(void *opaque, uint32_t value)
@@ -410,7 +417,8 @@ static void pl011_write(void *opaque, hwaddr offset,
     case 11: /* UARTLCR_H */
         /* Reset the FIFO state on FIFO enable or disable */
         if ((s->lcr ^ value) & LCR_FEN) {
-            pl011_reset_fifo(s);
+            pl011_reset_rx_fifo(s);
+            pl011_reset_tx_fifo(s);
         }
         if ((s->lcr ^ value) & LCR_BRK) {
             int break_enable = value & LCR_BRK;
@@ -619,7 +627,8 @@ static void pl011_reset(DeviceState *dev)
     s->ifl = 0x12;
     s->cr = 0x300;
     s->flags = 0;
-    pl011_reset_fifo(s);
+    pl011_reset_rx_fifo(s);
+    pl011_reset_tx_fifo(s);
 }
 
 static void pl011_class_init(ObjectClass *oc, void *data)
-- 
2.41.0

[PATCH v5 06/16] hw/char/pl011: Extract pl011_write_txdata() from pl011_write()

[Philippe Mathieu-Daudé]

When implementing FIFO, this code will become more complex.
Start by factoring it out to a new pl011_write_txdata() function.
No functional change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/char/pl011.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 3d294c3b52..c2ee9b0321 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -221,6 +221,18 @@ static void pl011_loopback_tx(PL011State *s, uint32_t value)
     pl011_put_fifo(s, value);
 }
 
+static void pl011_write_txdata(PL011State *s, uint8_t data)
+{
+    /* ??? Check if transmitter is enabled.  */
+
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(&s->chr, &data, 1);
+    pl011_loopback_tx(s, data);
+    s->int_level |= INT_TX;
+    pl011_update(s);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -388,14 +400,8 @@ static void pl011_write(void *opaque, hwaddr offset,
 
     switch (offset >> 2) {
     case 0: /* UARTDR */
-        /* ??? Check if transmitter is enabled.  */
         ch = value;
-        /* XXX this blocks entire thread. Rewrite to use
-         * qemu_chr_fe_write and background I/O callbacks */
-        qemu_chr_fe_write_all(&s->chr, &ch, 1);
-        pl011_loopback_tx(s, ch);
-        s->int_level |= INT_TX;
-        pl011_update(s);
+        pl011_write_txdata(s, ch);
         break;
     case 1: /* UARTRSR/UARTECR */
         s->rsr = 0;
-- 
2.41.0

[PATCH v5 07/16] hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()

[Philippe Mathieu-Daudé]

To keep MemoryRegionOps read/write handlers with similar logic,
factor pl011_read_txdata() out of pl011_read(), similar to what
the previous commit did to pl011_write().
No functional change intended.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c2ee9b0321..5e44837206 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -233,31 +233,38 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
     pl011_update(s);
 }
 
+static uint32_t pl011_read_rxdata(PL011State *s)
+{
+    uint32_t c;
+
+    s->flags &= ~PL011_FLAG_RXFF;
+    c = s->read_fifo[s->read_pos];
+    if (s->read_count > 0) {
+        s->read_count--;
+        s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
+    }
+    if (s->read_count == 0) {
+        s->flags |= PL011_FLAG_RXFE;
+    }
+    if (s->read_count == s->read_trigger - 1) {
+        s->int_level &= ~ INT_RX;
+    }
+    trace_pl011_read_fifo(s->read_count);
+    s->rsr = c >> 8;
+    pl011_update(s);
+    qemu_chr_fe_accept_input(&s->chr);
+    return c;
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
     PL011State *s = (PL011State *)opaque;
-    uint32_t c;
     uint64_t r;
 
     switch (offset >> 2) {
     case 0: /* UARTDR */
-        s->flags &= ~PL011_FLAG_RXFF;
-        c = s->read_fifo[s->read_pos];
-        if (s->read_count > 0) {
-            s->read_count--;
-            s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
-        }
-        if (s->read_count == 0) {
-            s->flags |= PL011_FLAG_RXFE;
-        }
-        if (s->read_count == s->read_trigger - 1)
-            s->int_level &= ~ INT_RX;
-        trace_pl011_read_fifo(s->read_count);
-        s->rsr = c >> 8;
-        pl011_update(s);
-        qemu_chr_fe_accept_input(&s->chr);
-        r = c;
+        r = pl011_read_rxdata(s);
         break;
     case 1: /* UARTRSR */
         r = s->rsr;
-- 
2.41.0

[PATCH v5 08/16] hw/char/pl011: Warn when using disabled transmitter

[Philippe Mathieu-Daudé]

We shouldn't transmit characters when the full UART or its
transmitter is disabled. However we don't want to break the
possibly incomplete "my first bare metal assembly program"s,
so we choose to simply display a warning when this occurs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/char/pl011.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 5e44837206..c76283dccf 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -85,7 +85,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define CR_OUT1     (1 << 12)
 #define CR_RTS      (1 << 11)
 #define CR_DTR      (1 << 10)
+#define CR_TXE      (1 << 8)
 #define CR_LBE      (1 << 7)
+#define CR_UARTEN   (1 << 0)
 
 /* Integer Baud Rate Divider, UARTIBRD */
 #define IBRD_MASK 0x3f
@@ -223,7 +225,12 @@ static void pl011_loopback_tx(PL011State *s, uint32_t value)
 
 static void pl011_write_txdata(PL011State *s, uint8_t data)
 {
-    /* ??? Check if transmitter is enabled.  */
+    if (!(s->cr & CR_UARTEN)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n");
+    }
+    if (!(s->cr & CR_TXE)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
+    }
 
     /* XXX this blocks entire thread. Rewrite to use
      * qemu_chr_fe_write and background I/O callbacks */
-- 
2.41.0

[PATCH v5 09/16] tests/qtest: Update tests using PL011 UART

[Philippe Mathieu-Daudé]

We weren't enabling the PL011 TX UART before using it
on the raspi and virt machines. Update the ASM code
prefixing:

  *UART_CTRL = UART_ENABLE | TX_ENABLE;

to:

  while (true) {
      *UART_DATA = 'T';
  }

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/qtest/boot-serial-test.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3b92fa5d50..5cb309ccf0 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = {
 };
 
 static const uint8_t bios_raspi2[] = {
-    0x08, 0x30, 0x9f, 0xe5,                 /* ldr   r3,[pc,#8]    Get base */
+    0x10, 0x30, 0x9f, 0xe5,                 /* ldr     r3,[pc,#8]    Get base */
+    0x10, 0x20, 0x9f, 0xe5,                 /* ldr     r2,[pc,#8]    Get CR */
+    0xb0, 0x23, 0xc3, 0xe1,                 /* strh    r2,[r3, #48]  Set CR */
     0x54, 0x20, 0xa0, 0xe3,                 /* mov     r2,#'T' */
-    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3] */
-    0xfb, 0xff, 0xff, 0xea,                 /* b       loop */
+    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3]       loop: */
+    0xfd, 0xff, 0xff, 0xea,                 /* b       loop */
     0x00, 0x10, 0x20, 0x3f,                 /* 0x3f201000 = UART0 base addr */
+    0x01, 0x01, 0x00, 0x00,                 /* 0x101      = CR UARTEN|TXE */
 };
 
 static const uint8_t kernel_aarch64[] = {
-    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
     0x02, 0x20, 0xa1, 0xd2,                 /* mov     x2, #0x9000000 */
+    0x21, 0x20, 0x80, 0x52,                 /* mov     w1, #0x101 */
+    0x41, 0x60, 0x00, 0x79,                 /* strh    w1, [x2, #48] */
+    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
     0x41, 0x00, 0x00, 0x39,                 /* strb    w1, [x2] */
-    0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
+    0xff, 0xff, 0xff, 0x17,                 /* b       -4 (loop) */
 };
 
 static const uint8_t kernel_nrf51[] = {
-- 
2.41.0

Re: [PATCH v5 09/16] tests/qtest: Update tests using PL011 UART

[Peter Maydell]

On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We weren't enabling the PL011 TX UART before using it
> on the raspi and virt machines. Update the ASM code
> prefixing:
>
>   *UART_CTRL = UART_ENABLE | TX_ENABLE;
>
> to:
>
>   while (true) {
>       *UART_DATA = 'T';
>   }
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/qtest/boot-serial-test.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index 3b92fa5d50..5cb309ccf0 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = {
>  };
>
>  static const uint8_t bios_raspi2[] = {
> -    0x08, 0x30, 0x9f, 0xe5,                 /* ldr   r3,[pc,#8]    Get base */
> +    0x10, 0x30, 0x9f, 0xe5,                 /* ldr     r3,[pc,#8]    Get base */

The instruction bytes have changed but the disassembly comment has not...

> +    0x10, 0x20, 0x9f, 0xe5,                 /* ldr     r2,[pc,#8]    Get CR */
> +    0xb0, 0x23, 0xc3, 0xe1,                 /* strh    r2,[r3, #48]  Set CR */
>      0x54, 0x20, 0xa0, 0xe3,                 /* mov     r2,#'T' */
> -    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3] */
> -    0xfb, 0xff, 0xff, 0xea,                 /* b       loop */
> +    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3]       loop: */

This placement of the "loop:" label is odd -- usually the label
goes before the instruction that a branch to the label will
start executing at.

> +    0xfd, 0xff, 0xff, 0xea,                 /* b       loop */

Here also the bytes changed but not the disassembly. Since
'b' is a relative branch, why does the offset change?

>      0x00, 0x10, 0x20, 0x3f,                 /* 0x3f201000 = UART0 base addr */
> +    0x01, 0x01, 0x00, 0x00,                 /* 0x101      = CR UARTEN|TXE */
>  };
>
>  static const uint8_t kernel_aarch64[] = {
> -    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>      0x02, 0x20, 0xa1, 0xd2,                 /* mov     x2, #0x9000000 */
> +    0x21, 0x20, 0x80, 0x52,                 /* mov     w1, #0x101 */
> +    0x41, 0x60, 0x00, 0x79,                 /* strh    w1, [x2, #48] */
> +    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>      0x41, 0x00, 0x00, 0x39,                 /* strb    w1, [x2] */
> -    0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
> +    0xff, 0xff, 0xff, 0x17,                 /* b       -4 (loop) */

Another unexplained offset change here.

>  };
>
>  static const uint8_t kernel_nrf51[] = {
> --
> 2.41.0

thanks
-- PMM

Re: [PATCH v5 09/16] tests/qtest: Update tests using PL011 UART

[Philippe Mathieu-Daudé]

On 29/7/24 17:47, Peter Maydell wrote:
> On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> We weren't enabling the PL011 TX UART before using it
>> on the raspi and virt machines. Update the ASM code
>> prefixing:
>>
>>    *UART_CTRL = UART_ENABLE | TX_ENABLE;
>>
>> to:
>>
>>    while (true) {
>>        *UART_DATA = 'T';
>>    }
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/qtest/boot-serial-test.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index 3b92fa5d50..5cb309ccf0 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -70,18 +70,23 @@ static const uint8_t kernel_plml605[] = {
>>   };
>>
>>   static const uint8_t bios_raspi2[] = {
>> -    0x08, 0x30, 0x9f, 0xe5,                 /* ldr   r3,[pc,#8]    Get base */
>> +    0x10, 0x30, 0x9f, 0xe5,                 /* ldr     r3,[pc,#8]    Get base */
> 
> The instruction bytes have changed but the disassembly comment has not...
> 
>> +    0x10, 0x20, 0x9f, 0xe5,                 /* ldr     r2,[pc,#8]    Get CR */
>> +    0xb0, 0x23, 0xc3, 0xe1,                 /* strh    r2,[r3, #48]  Set CR */
>>       0x54, 0x20, 0xa0, 0xe3,                 /* mov     r2,#'T' */
>> -    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3] */
>> -    0xfb, 0xff, 0xff, 0xea,                 /* b       loop */
>> +    0x00, 0x20, 0xc3, 0xe5,                 /* strb    r2,[r3]       loop: */
> 
> This placement of the "loop:" label is odd -- usually the label
> goes before the instruction that a branch to the label will
> start executing at.
> 
>> +    0xfd, 0xff, 0xff, 0xea,                 /* b       loop */
> 
> Here also the bytes changed but not the disassembly. Since
> 'b' is a relative branch, why does the offset change?

It felt simpler while single-stepping to just fill the TXDATA register
with the byte to send, and not reset the other registers with unchanged
values. Anyway you are right, I'll split the changes for clarity.

> 
>>       0x00, 0x10, 0x20, 0x3f,                 /* 0x3f201000 = UART0 base addr */
>> +    0x01, 0x01, 0x00, 0x00,                 /* 0x101      = CR UARTEN|TXE */
>>   };
>>
>>   static const uint8_t kernel_aarch64[] = {
>> -    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>>       0x02, 0x20, 0xa1, 0xd2,                 /* mov     x2, #0x9000000 */
>> +    0x21, 0x20, 0x80, 0x52,                 /* mov     w1, #0x101 */
>> +    0x41, 0x60, 0x00, 0x79,                 /* strh    w1, [x2, #48] */
>> +    0x81, 0x0a, 0x80, 0x52,                 /* mov     w1, #0x54 */
>>       0x41, 0x00, 0x00, 0x39,                 /* strb    w1, [x2] */
>> -    0xfd, 0xff, 0xff, 0x17,                 /* b       -12 (loop) */
>> +    0xff, 0xff, 0xff, 0x17,                 /* b       -4 (loop) */
> 
> Another unexplained offset change here.
> 
>>   };
>>
>>   static const uint8_t kernel_nrf51[] = {
>> --
>> 2.41.0
> 
> thanks
> -- PMM

[PATCH v5 10/16] hw/char/pl011: Check if receiver is enabled

[Philippe Mathieu-Daudé]

Do not receive characters when UART or receiver are disabled.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/char/pl011.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c76283dccf..0ce91c13d3 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define CR_OUT1     (1 << 12)
 #define CR_RTS      (1 << 11)
 #define CR_DTR      (1 << 10)
+#define CR_RXE      (1 << 9)
 #define CR_TXE      (1 << 8)
 #define CR_LBE      (1 << 7)
 #define CR_UARTEN   (1 << 0)
@@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset,
 static int pl011_can_receive(void *opaque)
 {
     PL011State *s = (PL011State *)opaque;
-    int r;
+    int r = 0;
 
-    r = s->read_count < pl011_get_fifo_depth(s);
+    if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) {
+        r = s->read_count < pl011_get_fifo_depth(s);
+    }
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
 }
-- 
2.41.0

Re: [PATCH v5 10/16] hw/char/pl011: Check if receiver is enabled

[Peter Maydell]

On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Do not receive characters when UART or receiver are disabled.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/char/pl011.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index c76283dccf..0ce91c13d3 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
>  #define CR_OUT1     (1 << 12)
>  #define CR_RTS      (1 << 11)
>  #define CR_DTR      (1 << 10)
> +#define CR_RXE      (1 << 9)
>  #define CR_TXE      (1 << 8)
>  #define CR_LBE      (1 << 7)
>  #define CR_UARTEN   (1 << 0)
> @@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset,
>  static int pl011_can_receive(void *opaque)
>  {
>      PL011State *s = (PL011State *)opaque;
> -    int r;
> +    int r = 0;
>
> -    r = s->read_count < pl011_get_fifo_depth(s);
> +    if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) {
> +        r = s->read_count < pl011_get_fifo_depth(s);
> +    }
>      trace_pl011_can_receive(s->lcr, s->read_count, r);
>      return r;

I have a vague recollection of a previous conversation
where we discussed whether tightening up the UART-enable
checks would break existing only-tested-on-QEMU baremetal
simple example code. But I can't find it in my email
archive -- do you remember a discussion about this or
am I misremembering?

thanks
-- PMM

Re: [PATCH v5 10/16] hw/char/pl011: Check if receiver is enabled

[Philippe Mathieu-Daudé]

Hi Peter,

On 29/7/24 17:51, Peter Maydell wrote:
> On Fri, 19 Jul 2024 at 19:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Do not receive characters when UART or receiver are disabled.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   hw/char/pl011.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index c76283dccf..0ce91c13d3 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -85,6 +85,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
>>   #define CR_OUT1     (1 << 12)
>>   #define CR_RTS      (1 << 11)
>>   #define CR_DTR      (1 << 10)
>> +#define CR_RXE      (1 << 9)
>>   #define CR_TXE      (1 << 8)
>>   #define CR_LBE      (1 << 7)
>>   #define CR_UARTEN   (1 << 0)
>> @@ -481,9 +482,11 @@ static void pl011_write(void *opaque, hwaddr offset,
>>   static int pl011_can_receive(void *opaque)
>>   {
>>       PL011State *s = (PL011State *)opaque;
>> -    int r;
>> +    int r = 0;
>>
>> -    r = s->read_count < pl011_get_fifo_depth(s);
>> +    if ((s->cr & CR_UARTEN) && (s->cr & CR_RXE)) {
>> +        r = s->read_count < pl011_get_fifo_depth(s);
>> +    }
>>       trace_pl011_can_receive(s->lcr, s->read_count, r);
>>       return r;
> 
> I have a vague recollection of a previous conversation
> where we discussed whether tightening up the UART-enable
> checks would break existing only-tested-on-QEMU baremetal
> simple example code. But I can't find it in my email
> archive -- do you remember a discussion about this or
> am I misremembering?

The previous conversation is:
https://lore.kernel.org/qemu-devel/CAFEAcA_7dkWB9qPkzYmW6_F1eaAet0PPk0PHywGS2EpAkFAsUQ@mail.gmail.com/

I understood 'my first assembly "Hello, World" program' has to keep
working, which only uses the TX channel; and the boot-serial-qtest
is fixed in a previous patch. Now I get you were thinking of a wider
baremetal example code. I'll convert to GUEST_ERROR log level,
similar to the TX path, since it is still relevant for developers
who plan to run their guest code on real hardware.

Regards,

Phil.

[PATCH v5 11/16] hw/char/pl011: Rename RX FIFO methods

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/char/pl011.c      | 12 ++++++------
 hw/char/trace-events |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 0ce91c13d3..c42c6d1ac2 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -174,7 +174,7 @@ static inline void pl011_reset_tx_fifo(PL011State *s)
     s->flags |= PL011_FLAG_TXFE;
 }
 
-static void pl011_put_fifo(void *opaque, uint32_t value)
+static void pl011_fifo_rx_put(void *opaque, uint32_t value)
 {
     PL011State *s = (PL011State *)opaque;
     int slot;
@@ -185,9 +185,9 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
     s->read_fifo[slot] = value;
     s->read_count++;
     s->flags &= ~PL011_FLAG_RXFE;
-    trace_pl011_put_fifo(value, s->read_count);
+    trace_pl011_fifo_rx_put(value, s->read_count);
     if (s->read_count == pipe_depth) {
-        trace_pl011_put_fifo_full();
+        trace_pl011_fifo_rx_full();
         s->flags |= PL011_FLAG_RXFF;
     }
     if (s->read_count == s->read_trigger) {
@@ -221,7 +221,7 @@ static void pl011_loopback_tx(PL011State *s, uint32_t value)
      *
      * For simplicity, the above described is not emulated.
      */
-    pl011_put_fifo(s, value);
+    pl011_fifo_rx_put(s, value);
 }
 
 static void pl011_write_txdata(PL011State *s, uint8_t data)
@@ -502,13 +502,13 @@ static void pl011_receive(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    pl011_put_fifo(opaque, *buf);
+    pl011_fifo_rx_put(opaque, *buf);
 }
 
 static void pl011_event(void *opaque, QEMUChrEvent event)
 {
     if (event == CHR_EVENT_BREAK && !pl011_loopback_enabled(opaque)) {
-        pl011_put_fifo(opaque, DR_BE);
+        pl011_fifo_rx_put(opaque, DR_BE);
     }
 }
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 8875758076..59e1f734a7 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -58,8 +58,8 @@ pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x valu
 pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
 pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
 pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d"
-pl011_put_fifo(uint32_t c, int read_count) "new char 0x%x read_count now %d"
-pl011_put_fifo_full(void) "FIFO now full, RXFF set"
+pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d"
+pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
 pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, uint32_t fbrd) "new baudrate %u (clk: %" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: %" PRIu32 ")"
 
 # cmsdk-apb-uart.c
-- 
2.41.0

[PATCH v5 12/16] hw/char/pl011: Add transmit FIFO to PL011State

[Philippe Mathieu-Daudé]

In order to make the next commit easier to review,
introduce the transmit FIFO, but do not yet use it.

We only migrate the TX FIFO if it is in use.

When migrating from new to old VM:
- if the fifo is empty, migration will still work because
   of the subsection.
- if the fifo is not empty, the subsection will be ignored,
  with the only consequence being that some characters will
  be dropped.

Since the FIFO is created empty, we don't need a migration
pre_load() handler.

Uninline pl011_reset_tx_fifo().

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/char/pl011.h |  2 ++
 hw/char/pl011.c         | 37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index 4fcaf3d7d3..e8d95961f6 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -18,6 +18,7 @@
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
 #include "qom/object.h"
+#include "qemu/fifo8.h"
 
 #define TYPE_PL011 "pl011"
 OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011)
@@ -52,6 +53,7 @@ struct PL011State {
     Clock *clk;
     bool migrate_clk;
     const unsigned char *id;
+    Fifo8 xmit_fifo;
 };
 
 DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr);
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c42c6d1ac2..6519340b50 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -167,11 +167,13 @@ static inline void pl011_reset_rx_fifo(PL011State *s)
     s->flags |= PL011_FLAG_RXFE;
 }
 
-static inline void pl011_reset_tx_fifo(PL011State *s)
+static void pl011_reset_tx_fifo(PL011State *s)
 {
     /* Reset FIFO flags */
     s->flags &= ~PL011_FLAG_TXFF;
     s->flags |= PL011_FLAG_TXFE;
+
+    fifo8_reset(&s->xmit_fifo);
 }
 
 static void pl011_fifo_rx_put(void *opaque, uint32_t value)
@@ -545,6 +547,24 @@ static const VMStateDescription vmstate_pl011_clock = {
     }
 };
 
+static bool pl011_xmit_fifo_state_needed(void *opaque)
+{
+    PL011State* s = opaque;
+
+    return (s->lcr & LCR_FEN) && !fifo8_is_empty(&s->xmit_fifo);
+}
+
+static const VMStateDescription vmstate_pl011_xmit_fifo = {
+    .name = "pl011/xmit_fifo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pl011_xmit_fifo_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_FIFO8(xmit_fifo, PL011State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int pl011_post_load(void *opaque, int version_id)
 {
     PL011State* s = opaque;
@@ -599,7 +619,11 @@ static const VMStateDescription vmstate_pl011 = {
     .subsections = (const VMStateDescription * const []) {
         &vmstate_pl011_clock,
         NULL
-    }
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_pl011_xmit_fifo,
+        NULL
+    },
 };
 
 static Property pl011_properties[] = {
@@ -614,6 +638,7 @@ static void pl011_init(Object *obj)
     PL011State *s = PL011(obj);
     int i;
 
+    fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH);
     memory_region_init_io(&s->iomem, OBJECT(s), &pl011_ops, s, "pl011", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
@@ -626,6 +651,13 @@ static void pl011_init(Object *obj)
     s->id = pl011_id_arm;
 }
 
+static void pl011_finalize(Object *obj)
+{
+    PL011State *s = PL011(obj);
+
+    fifo8_destroy(&s->xmit_fifo);
+}
+
 static void pl011_realize(DeviceState *dev, Error **errp)
 {
     PL011State *s = PL011(dev);
@@ -669,6 +701,7 @@ static const TypeInfo pl011_arm_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PL011State),
     .instance_init = pl011_init,
+    .instance_finalize = pl011_finalize,
     .class_init    = pl011_class_init,
 };
 
-- 
2.41.0

[PATCH v5 13/16] hw/char/pl011: Introduce pl011_xmit() as GSource

[Philippe Mathieu-Daudé]

Extract pl011_xmit() from pl011_write_txdata(). Use the
FIFO to pass the character to be transmitted.

Implement it using the FEWatchFunc prototype, since we want
to register it as GSource later. While the return value is
not yet used, we return G_SOURCE_REMOVE meaning the GSource
is removed from the main loop (because we only send one char).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c      | 30 +++++++++++++++++++++++++-----
 hw/char/trace-events |  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 6519340b50..6394d6eb36 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -226,6 +226,28 @@ static void pl011_loopback_tx(PL011State *s, uint32_t value)
     pl011_fifo_rx_put(s, value);
 }
 
+static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
+{
+    PL011State *s = opaque;
+    int bytes_consumed;
+    uint8_t data;
+
+    data = fifo8_pop(&s->xmit_fifo);
+    bytes_consumed = 1;
+
+    /*
+     * XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks
+     */
+    qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed);
+    trace_pl011_fifo_tx_xmit(bytes_consumed);
+    s->int_level |= INT_TX;
+
+    pl011_update(s);
+
+    return G_SOURCE_REMOVE;
+}
+
 static void pl011_write_txdata(PL011State *s, uint8_t data)
 {
     if (!(s->cr & CR_UARTEN)) {
@@ -235,12 +257,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
     }
 
-    /* XXX this blocks entire thread. Rewrite to use
-     * qemu_chr_fe_write and background I/O callbacks */
-    qemu_chr_fe_write_all(&s->chr, &data, 1);
+    trace_pl011_fifo_tx_put(data);
     pl011_loopback_tx(s, data);
-    s->int_level |= INT_TX;
-    pl011_update(s);
+    fifo8_push(&s->xmit_fifo, data);
+    pl011_xmit(NULL, G_IO_OUT, s);
 }
 
 static uint32_t pl011_read_rxdata(PL011State *s)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 59e1f734a7..30d06a2383 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -60,6 +60,8 @@ pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x val
 pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d"
 pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d"
 pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
+pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
+pl011_fifo_tx_xmit(int count) "TX FIFO pop %d chars"
 pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, uint32_t fbrd) "new baudrate %u (clk: %" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: %" PRIu32 ")"
 
 # cmsdk-apb-uart.c
-- 
2.41.0

[PATCH v5 14/16] hw/char/pl011: Consider TX FIFO overrun error

[Philippe Mathieu-Daudé]

When transmission is disabled, characters are still queued
to the FIFO which eventually overruns. Report that error
condition in the status register.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c      | 17 +++++++++++++++++
 hw/char/trace-events |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 6394d6eb36..b8cde03f98 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -61,6 +61,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 /* Data Register, UARTDR */
 #define DR_BE   (1 << 10)
 
+/* Receive Status Register/Error Clear Register, UARTRSR/UARTECR */
+#define RSR_OE  (1 << 3)
+
 /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */
 #define INT_OE (1 << 10)
 #define INT_BE (1 << 9)
@@ -232,6 +235,13 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
     int bytes_consumed;
     uint8_t data;
 
+    if (!(s->cr & CR_UARTEN)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n");
+    }
+    if (!(s->cr & CR_TXE)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
+    }
+
     data = fifo8_pop(&s->xmit_fifo);
     bytes_consumed = 1;
 
@@ -257,6 +267,13 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
     }
 
+    if (fifo8_is_full(&s->xmit_fifo)) {
+        /* The FIFO is already full. Content remains valid. */
+        trace_pl011_fifo_tx_overrun();
+        s->rsr |= RSR_OE;
+        return;
+    }
+
     trace_pl011_fifo_tx_put(data);
     pl011_loopback_tx(s, data);
     fifo8_push(&s->xmit_fifo, data);
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 30d06a2383..4a9c0bd271 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -62,6 +62,7 @@ pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d
 pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
 pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
 pl011_fifo_tx_xmit(int count) "TX FIFO pop %d chars"
+pl011_fifo_tx_overrun(void) "TX FIFO overrun"
 pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, uint32_t fbrd) "new baudrate %u (clk: %" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: %" PRIu32 ")"
 
 # cmsdk-apb-uart.c
-- 
2.41.0

[PATCH v5 15/16] hw/char/pl011: Drain TX FIFO when no backend connected

[Philippe Mathieu-Daudé]

When no character backend is connected, the PL011 frontend
just drains the FIFO.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/pl011.c      | 13 +++++++++++++
 hw/char/trace-events |  1 +
 2 files changed, 14 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index b8cde03f98..cfa3fd3da4 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -229,6 +229,13 @@ static void pl011_loopback_tx(PL011State *s, uint32_t value)
     pl011_fifo_rx_put(s, value);
 }
 
+static void pl011_drain_tx(PL011State *s)
+{
+    trace_pl011_fifo_tx_drain(fifo8_num_used(&s->xmit_fifo));
+    pl011_reset_tx_fifo(s);
+    s->rsr &= ~RSR_OE;
+}
+
 static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
 {
     PL011State *s = opaque;
@@ -242,6 +249,12 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
     }
 
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
+        /* Instant drain the fifo when there's no back-end. */
+        pl011_drain_tx(s);
+        return G_SOURCE_REMOVE;
+    }
+
     data = fifo8_pop(&s->xmit_fifo);
     bytes_consumed = 1;
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 4a9c0bd271..bf586ba664 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -63,6 +63,7 @@ pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
 pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
 pl011_fifo_tx_xmit(int count) "TX FIFO pop %d chars"
 pl011_fifo_tx_overrun(void) "TX FIFO overrun"
+pl011_fifo_tx_drain(unsigned drained) "TX FIFO draining %u chars"
 pl011_baudrate_change(unsigned int baudrate, uint64_t clock, uint32_t ibrd, uint32_t fbrd) "new baudrate %u (clk: %" PRIu64 "hz, ibrd: %" PRIu32 ", fbrd: %" PRIu32 ")"
 
 # cmsdk-apb-uart.c
-- 
2.41.0

[RFC PATCH v5 16/16] hw/char/pl011: Implement TX FIFO

[Philippe Mathieu-Daudé]

If the UART back-end chardev doesn't drain data as fast as stdout
does or blocks, buffer in the TX FIFO to try again later.

This avoids having the IO-thread busy waiting on chardev back-ends,
reported recently when testing the Trusted Reference Stack and
using the socket backend.

Implement registering a front-end 'watch' callback on back-end
events, so we can resume transmitting when the back-end is writable
again, not blocking the main loop.

Similarly to the RX FIFO path, FIFO level selection is not
implemented (interrupt is triggered when a single byte is available
in the FIFO).

Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org>
Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Something is still broken, some characters are emitted async...
---
 hw/char/pl011.c      | 60 ++++++++++++++++++++++++++++++++++++--------
 hw/char/trace-events |  1 +
 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index cfa3fd3da4..9f72b6a765 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
 {
     PL011State *s = opaque;
     int bytes_consumed;
-    uint8_t data;
+    const uint8_t *buf;
+    uint32_t buflen;
+    uint32_t count;
 
     if (!(s->cr & CR_UARTEN)) {
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n");
@@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
         qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
     }
 
+    count = fifo8_num_used(&s->xmit_fifo);
+    if (count < 1) {
+        /* FIFO empty */
+        return G_SOURCE_REMOVE;
+    }
+
     if (!qemu_chr_fe_backend_connected(&s->chr)) {
         /* Instant drain the fifo when there's no back-end. */
         pl011_drain_tx(s);
         return G_SOURCE_REMOVE;
     }
 
-    data = fifo8_pop(&s->xmit_fifo);
-    bytes_consumed = 1;
+    buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen);
 
-    /*
-     * XXX this blocks entire thread. Rewrite to use
-     * qemu_chr_fe_write and background I/O callbacks
-     */
-    qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed);
+    /* Transmit as much data as we can. */
+    bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen);
     trace_pl011_fifo_tx_xmit(bytes_consumed);
+    if (bytes_consumed < 0) {
+        /* Error in back-end: drain the fifo. */
+        pl011_drain_tx(s);
+        return G_SOURCE_REMOVE;
+    }
+
+    /* Pop the data we could transmit. */
+    fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL);
     s->int_level |= INT_TX;
 
     pl011_update(s);
 
+    if (!fifo8_is_empty(&s->xmit_fifo)) {
+        /* Reschedule another transmission if we couldn't transmit all. */
+        return G_SOURCE_CONTINUE;
+    }
+
     return G_SOURCE_REMOVE;
 }
 
@@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
     trace_pl011_fifo_tx_put(data);
     pl011_loopback_tx(s, data);
     fifo8_push(&s->xmit_fifo, data);
+    if (fifo8_is_full(&s->xmit_fifo)) {
+        s->flags |= PL011_FLAG_TXFF;
+    }
+
     pl011_xmit(NULL, G_IO_OUT, s);
 }
 
@@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr offset,
         pl011_trace_baudrate_change(s);
         break;
     case 11: /* UARTLCR_H */
-        /* Reset the FIFO state on FIFO enable or disable */
         if ((s->lcr ^ value) & LCR_FEN) {
-            pl011_reset_rx_fifo(s);
+            bool fifo_enabled = value & LCR_FEN;
+
+            trace_pl011_fifo_enable(fifo_enabled);
+            if (fifo_enabled) {
+                /* Transmit and receive FIFO buffers are enabled (FIFO mode). */
+                fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH);
+            } else {
+                /*
+                 * FIFOs are disabled (character mode) that is, the FIFOs
+                 * become 1-byte-deep holding registers.
+                 */
+                pl011_drain_tx(s);
+                fifo8_change_capacity(&s->xmit_fifo, 1);
+            }
+            /* Reset the FIFO state on FIFO enable or disable */
             pl011_reset_tx_fifo(s);
+            pl011_reset_rx_fifo(s);
         }
         if ((s->lcr ^ value) & LCR_BRK) {
             int break_enable = value & LCR_BRK;
@@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int version_id)
         s->read_pos = 0;
     }
 
+    if (!fifo8_is_empty(&s->xmit_fifo)) {
+        /* Reschedule another transmission */
+        qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, pl011_xmit, s);
+    }
+
     s->ibrd &= IBRD_MASK;
     s->fbrd &= FBRD_MASK;
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index bf586ba664..2405819812 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -58,6 +58,7 @@ pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x valu
 pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
 pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
 pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d"
+pl011_fifo_enable(bool enable) "enable:%u"
 pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d"
 pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
 pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
-- 
2.41.0

Re: [RFC PATCH v5 16/16] hw/char/pl011: Implement TX FIFO

[Mark Cave-Ayland]

On 19/07/2024 19:10, Philippe Mathieu-Daudé wrote:

> If the UART back-end chardev doesn't drain data as fast as stdout
> does or blocks, buffer in the TX FIFO to try again later.
> 
> This avoids having the IO-thread busy waiting on chardev back-ends,
> reported recently when testing the Trusted Reference Stack and
> using the socket backend.
> 
> Implement registering a front-end 'watch' callback on back-end
> events, so we can resume transmitting when the back-end is writable
> again, not blocking the main loop.
> 
> Similarly to the RX FIFO path, FIFO level selection is not
> implemented (interrupt is triggered when a single byte is available
> in the FIFO).
> 
> Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Something is still broken, some characters are emitted async...
> ---
>   hw/char/pl011.c      | 60 ++++++++++++++++++++++++++++++++++++--------
>   hw/char/trace-events |  1 +
>   2 files changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index cfa3fd3da4..9f72b6a765 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
>   {
>       PL011State *s = opaque;
>       int bytes_consumed;
> -    uint8_t data;
> +    const uint8_t *buf;
> +    uint32_t buflen;
> +    uint32_t count;
>   
>       if (!(s->cr & CR_UARTEN)) {
>           qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled UART\n");
> @@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
>           qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to disabled TX UART\n");
>       }
>   
> +    count = fifo8_num_used(&s->xmit_fifo);
> +    if (count < 1) {
> +        /* FIFO empty */
> +        return G_SOURCE_REMOVE;
> +    }
> +
>       if (!qemu_chr_fe_backend_connected(&s->chr)) {
>           /* Instant drain the fifo when there's no back-end. */
>           pl011_drain_tx(s);
>           return G_SOURCE_REMOVE;
>       }
>   
> -    data = fifo8_pop(&s->xmit_fifo);
> -    bytes_consumed = 1;
> +    buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen);
>   
> -    /*
> -     * XXX this blocks entire thread. Rewrite to use
> -     * qemu_chr_fe_write and background I/O callbacks
> -     */
> -    qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed);
> +    /* Transmit as much data as we can. */
> +    bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen);
>       trace_pl011_fifo_tx_xmit(bytes_consumed);
> +    if (bytes_consumed < 0) {
> +        /* Error in back-end: drain the fifo. */
> +        pl011_drain_tx(s);
> +        return G_SOURCE_REMOVE;
> +    }
> +
> +    /* Pop the data we could transmit. */
> +    fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL);
>       s->int_level |= INT_TX;
>   
>       pl011_update(s);

One of the gotchas with Fifo8 is that whilst fifo8_push(), fifo8_pop() and 
fifo8_push_all() will wrap the FIFO buffer, fifo8_{peek,pop}_buf() do not. For 
example fifo8_num_used() could return 15, but if xmit_fifo->head is set to 15 then 
fifo8_{peek_pop}_buf() would return 1 leaving 14 characters in the FIFO at the end of 
pl011_xmit().

Possible solutions could be to use a loop to send one character at a time similar to:

     while (fifo8_num_used(&s->xmit_fifo)) {
         uint8_t c = fifo8_pop(&s->xmit_fifo);

         if (qemu_chr_fe_write(&s->chr, &c, 1) == -1) {
             fifo8_push(&s->xmit_fifo, c);
             break;
         }
     }

Or else use a solution similar to the one I used for ESP at 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c?ref_type=heads#L200. 
I did think about whether it was worth adding a function similar to the one used for 
ESP to the Fifo8 API, but wasn't sure it was worth it at the time.

> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
> +        /* Reschedule another transmission if we couldn't transmit all. */
> +        return G_SOURCE_CONTINUE;
> +    }
> +
>       return G_SOURCE_REMOVE;
>   }
>   
> @@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
>       trace_pl011_fifo_tx_put(data);
>       pl011_loopback_tx(s, data);
>       fifo8_push(&s->xmit_fifo, data);
> +    if (fifo8_is_full(&s->xmit_fifo)) {
> +        s->flags |= PL011_FLAG_TXFF;
> +    }
> +
>       pl011_xmit(NULL, G_IO_OUT, s);
>   }
>   
> @@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr offset,
>           pl011_trace_baudrate_change(s);
>           break;
>       case 11: /* UARTLCR_H */
> -        /* Reset the FIFO state on FIFO enable or disable */
>           if ((s->lcr ^ value) & LCR_FEN) {
> -            pl011_reset_rx_fifo(s);
> +            bool fifo_enabled = value & LCR_FEN;
> +
> +            trace_pl011_fifo_enable(fifo_enabled);
> +            if (fifo_enabled) {
> +                /* Transmit and receive FIFO buffers are enabled (FIFO mode). */
> +                fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH);
> +            } else {
> +                /*
> +                 * FIFOs are disabled (character mode) that is, the FIFOs
> +                 * become 1-byte-deep holding registers.
> +                 */
> +                pl011_drain_tx(s);
> +                fifo8_change_capacity(&s->xmit_fifo, 1);
> +            }

Presumably this is the part where fifo8_change_capacity() is required: what does 
changing the FIFO size to 1 do here? Is it possible to move the fifo_enabled check 
into pl011_read() and pop/clear the buffer there instead of changing the FIFO size?

> +            /* Reset the FIFO state on FIFO enable or disable */
>               pl011_reset_tx_fifo(s);
> +            pl011_reset_rx_fifo(s);
>           }
>           if ((s->lcr ^ value) & LCR_BRK) {
>               int break_enable = value & LCR_BRK;
> @@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int version_id)
>           s->read_pos = 0;
>       }
>   
> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
> +        /* Reschedule another transmission */
> +        qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, pl011_xmit, s);
> +    }
> +
>       s->ibrd &= IBRD_MASK;
>       s->fbrd &= FBRD_MASK;
>   
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index bf586ba664..2405819812 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -58,6 +58,7 @@ pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x valu
>   pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
>   pl011_write(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
>   pl011_can_receive(uint32_t lcr, int read_count, int r) "LCR 0x%08x read_count %d returning %d"
> +pl011_fifo_enable(bool enable) "enable:%u"
>   pl011_fifo_rx_put(uint32_t c, int read_count) "new char 0x%02x read_count now %d"
>   pl011_fifo_rx_full(void) "RX FIFO now full, RXFF set"
>   pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"


ATB,

Mark.

Re: [RFC PATCH v5 16/16] hw/char/pl011: Implement TX FIFO

[Philippe Mathieu-Daudé]

On 19/7/24 23:25, Mark Cave-Ayland wrote:
> On 19/07/2024 19:10, Philippe Mathieu-Daudé wrote:
> 
>> If the UART back-end chardev doesn't drain data as fast as stdout
>> does or blocks, buffer in the TX FIFO to try again later.
>>
>> This avoids having the IO-thread busy waiting on chardev back-ends,
>> reported recently when testing the Trusted Reference Stack and
>> using the socket backend.
>>
>> Implement registering a front-end 'watch' callback on back-end
>> events, so we can resume transmitting when the back-end is writable
>> again, not blocking the main loop.
>>
>> Similarly to the RX FIFO path, FIFO level selection is not
>> implemented (interrupt is triggered when a single byte is available
>> in the FIFO).
>>
>> Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org>
>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC: Something is still broken, some characters are emitted async...

[*]

>> ---
>>   hw/char/pl011.c      | 60 ++++++++++++++++++++++++++++++++++++--------
>>   hw/char/trace-events |  1 +
>>   2 files changed, 51 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index cfa3fd3da4..9f72b6a765 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -240,7 +240,9 @@ static gboolean pl011_xmit(void *do_not_use, 
>> GIOCondition cond, void *opaque)
>>   {
>>       PL011State *s = opaque;
>>       int bytes_consumed;
>> -    uint8_t data;
>> +    const uint8_t *buf;
>> +    uint32_t buflen;
>> +    uint32_t count;
>>       if (!(s->cr & CR_UARTEN)) {
>>           qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to 
>> disabled UART\n");
>> @@ -249,25 +251,40 @@ static gboolean pl011_xmit(void *do_not_use, 
>> GIOCondition cond, void *opaque)
>>           qemu_log_mask(LOG_GUEST_ERROR, "PL011 data written to 
>> disabled TX UART\n");
>>       }
>> +    count = fifo8_num_used(&s->xmit_fifo);
>> +    if (count < 1) {
>> +        /* FIFO empty */
>> +        return G_SOURCE_REMOVE;
>> +    }
>> +
>>       if (!qemu_chr_fe_backend_connected(&s->chr)) {
>>           /* Instant drain the fifo when there's no back-end. */
>>           pl011_drain_tx(s);
>>           return G_SOURCE_REMOVE;
>>       }
>> -    data = fifo8_pop(&s->xmit_fifo);
>> -    bytes_consumed = 1;
>> +    buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen);
>> -    /*
>> -     * XXX this blocks entire thread. Rewrite to use
>> -     * qemu_chr_fe_write and background I/O callbacks
>> -     */
>> -    qemu_chr_fe_write_all(&s->chr, &data, bytes_consumed);
>> +    /* Transmit as much data as we can. */
>> +    bytes_consumed = qemu_chr_fe_write(&s->chr, buf, buflen);
>>       trace_pl011_fifo_tx_xmit(bytes_consumed);
>> +    if (bytes_consumed < 0) {
>> +        /* Error in back-end: drain the fifo. */
>> +        pl011_drain_tx(s);
>> +        return G_SOURCE_REMOVE;
>> +    }
>> +
>> +    /* Pop the data we could transmit. */
>> +    fifo8_pop_buf(&s->xmit_fifo, bytes_consumed, NULL);
>>       s->int_level |= INT_TX;
>>       pl011_update(s);
> 
> One of the gotchas with Fifo8 is that whilst fifo8_push(), fifo8_pop() 
> and fifo8_push_all() will wrap the FIFO buffer, fifo8_{peek,pop}_buf() 
> do not. For example fifo8_num_used() could return 15, but if 
> xmit_fifo->head is set to 15 then fifo8_{peek_pop}_buf() would return 1 
> leaving 14 characters in the FIFO at the end of pl011_xmit().

Wow, indeed this is the problem [*] I was facing :) <3

> Possible solutions could be to use a loop to send one character at a 
> time similar to:
> 
>      while (fifo8_num_used(&s->xmit_fifo)) {
>          uint8_t c = fifo8_pop(&s->xmit_fifo);
> 
>          if (qemu_chr_fe_write(&s->chr, &c, 1) == -1) {
>              fifo8_push(&s->xmit_fifo, c);
>              break;
>          }
>      }
> 
> Or else use a solution similar to the one I used for ESP at 
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c?ref_type=heads#L200. I did think about whether it was worth adding a function similar to the one used for ESP to the Fifo8 API, but wasn't sure it was worth it at the time.

Yeah clearly this DTRT. I'll extract this method.

> 
>> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
>> +        /* Reschedule another transmission if we couldn't transmit 
>> all. */
>> +        return G_SOURCE_CONTINUE;
>> +    }
>> +
>>       return G_SOURCE_REMOVE;
>>   }
>> @@ -290,6 +307,10 @@ static void pl011_write_txdata(PL011State *s, 
>> uint8_t data)
>>       trace_pl011_fifo_tx_put(data);
>>       pl011_loopback_tx(s, data);
>>       fifo8_push(&s->xmit_fifo, data);
>> +    if (fifo8_is_full(&s->xmit_fifo)) {
>> +        s->flags |= PL011_FLAG_TXFF;
>> +    }
>> +
>>       pl011_xmit(NULL, G_IO_OUT, s);
>>   }
>> @@ -488,10 +509,24 @@ static void pl011_write(void *opaque, hwaddr 
>> offset,
>>           pl011_trace_baudrate_change(s);
>>           break;
>>       case 11: /* UARTLCR_H */
>> -        /* Reset the FIFO state on FIFO enable or disable */
>>           if ((s->lcr ^ value) & LCR_FEN) {
>> -            pl011_reset_rx_fifo(s);
>> +            bool fifo_enabled = value & LCR_FEN;
>> +
>> +            trace_pl011_fifo_enable(fifo_enabled);
>> +            if (fifo_enabled) {
>> +                /* Transmit and receive FIFO buffers are enabled 
>> (FIFO mode). */
>> +                fifo8_change_capacity(&s->xmit_fifo, PL011_FIFO_DEPTH);
>> +            } else {
>> +                /*
>> +                 * FIFOs are disabled (character mode) that is, the 
>> FIFOs
>> +                 * become 1-byte-deep holding registers.
>> +                 */
>> +                pl011_drain_tx(s);
>> +                fifo8_change_capacity(&s->xmit_fifo, 1);
>> +            }
> 
> Presumably this is the part where fifo8_change_capacity() is required: 
> what does changing the FIFO size to 1 do here? Is it possible to move 
> the fifo_enabled check into pl011_read() and pop/clear the buffer there 
> instead of changing the FIFO size?

Again I tried to stay close to the datasheet description:

   3.3.7 Line Control Register, UARTLCR_H

     Bit 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).

But indeed it could be clearer to handle the FEN bit and do FIFO
enabled check elsewhere, I'll have a look.

Many thanks for your review!!

>> +            /* Reset the FIFO state on FIFO enable or disable */
>>               pl011_reset_tx_fifo(s);
>> +            pl011_reset_rx_fifo(s);
>>           }
>>           if ((s->lcr ^ value) & LCR_BRK) {
>>               int break_enable = value & LCR_BRK;
>> @@ -636,6 +671,11 @@ static int pl011_post_load(void *opaque, int 
>> version_id)
>>           s->read_pos = 0;
>>       }
>> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
>> +        /* Reschedule another transmission */
>> +        qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP, 
>> pl011_xmit, s);
>> +    }

Re: [PATCH v5 00/16] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

Hi Peter,

On 19/7/24 20:10, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (16):

>    hw/char/pl011: Remove unused 'readbuff' field
>    hw/char/pl011: Move pl011_put_fifo() earlier
>    hw/char/pl011: Move pl011_loopback_enabled|tx() around
>    hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
>    hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
>    hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
>    hw/char/pl011: Warn when using disabled transmitter

>    hw/char/pl011: Rename RX FIFO methods


If you don't mind I'll queue the reviewed 2-8 & 11 to ease my workflow,
before respining the next version.

Thanks,

Phil.

Re: [PATCH v5 00/16] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Sat, 7 Sept 2024 at 06:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 19/7/24 20:10, Philippe Mathieu-Daudé wrote:
>
> > Philippe Mathieu-Daudé (16):
>
> >    hw/char/pl011: Remove unused 'readbuff' field
> >    hw/char/pl011: Move pl011_put_fifo() earlier
> >    hw/char/pl011: Move pl011_loopback_enabled|tx() around
> >    hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
> >    hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
> >    hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
> >    hw/char/pl011: Warn when using disabled transmitter
>
> >    hw/char/pl011: Rename RX FIFO methods
>
>
> If you don't mind I'll queue the reviewed 2-8 & 11 to ease my workflow,
> before respining the next version.

Sure, that's fine. I don't have anything pl011 related in
my queue that would conflict.

-- PMM

Re: [PATCH v5 00/16] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

On 7/9/24 12:38, Peter Maydell wrote:
> On Sat, 7 Sept 2024 at 06:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 19/7/24 20:10, Philippe Mathieu-Daudé wrote:
>>
>>> Philippe Mathieu-Daudé (16):
>>
>>>     hw/char/pl011: Remove unused 'readbuff' field
>>>     hw/char/pl011: Move pl011_put_fifo() earlier
>>>     hw/char/pl011: Move pl011_loopback_enabled|tx() around
>>>     hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
>>>     hw/char/pl011: Extract pl011_write_txdata() from pl011_write()
>>>     hw/char/pl011: Extract pl011_read_rxdata() from pl011_read()
>>>     hw/char/pl011: Warn when using disabled transmitter
>>
>>>     hw/char/pl011: Rename RX FIFO methods
>>
>>
>> If you don't mind I'll queue the reviewed 2-8 & 11 to ease my workflow,
>> before respining the next version.
> 
> Sure, that's fine. I don't have anything pl011 related in
> my queue that would conflict.

Great, thank you!