[PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[PATCH v6 0/7] 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.

Since v5:
- Rebased (few patches already merged)
- Do not forbid disabled UART/receiver (Peter)
- Use fifo8_peek API for wrapped buffer (Mark)

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é (7):
  hw/char/pl011: Warn when using disabled receiver
  hw/char/pl011: Add transmit FIFO to PL011State
  hw/char/pl011: Introduce pl011_xmit() as GSource
  hw/char/pl011: Trace FIFO enablement
  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 |   2 +
 hw/char/pl011.c         | 140 +++++++++++++++++++++++++++++++++++++---
 hw/char/trace-events    |   9 +++
 3 files changed, 141 insertions(+), 10 deletions(-)

-- 
2.47.1

[PATCH v6 1/7] hw/char/pl011: Warn when using disabled receiver

[Philippe Mathieu-Daudé]

We shouldn't receive characters when the full UART or its
receiver 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.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 06ce851044d..60cea1d9a16 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)
@@ -487,6 +488,12 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     int r;
 
+    if (!(s->cr & CR_UARTEN)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled UART\n");
+    }
+    if (!(s->cr & CR_RXE)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "PL011 reading data on disabled TX UART\n");
+    }
     r = s->read_count < pl011_get_fifo_depth(s);
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
-- 
2.47.1

[PATCH v6 2/7] 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 4fcaf3d7d30..e8d95961f66 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 60cea1d9a16..807fcdee50b 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)
@@ -553,6 +555,24 @@ static const VMStateDescription vmstate_pl011_clock = {
     }
 };
 
+static bool pl011_xmit_fifo_state_needed(void *opaque)
+{
+    PL011State* s = opaque;
+
+    return pl011_is_fifo_enabled(s) && !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;
@@ -607,7 +627,11 @@ static const VMStateDescription vmstate_pl011 = {
     .subsections = (const VMStateDescription * const []) {
         &vmstate_pl011_clock,
         NULL
-    }
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_pl011_xmit_fifo,
+        NULL
+    },
 };
 
 static const Property pl011_properties[] = {
@@ -621,6 +645,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++) {
@@ -633,6 +658,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);
@@ -676,6 +708,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.47.1

[PATCH v6 3/7] 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      | 36 +++++++++++++++++++++++++++++-------
 hw/char/trace-events |  3 +++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 807fcdee50b..b9c9e5b5983 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -226,6 +226,32 @@ 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;
+    uint32_t count;
+
+    count = fifo8_num_used(&s->xmit_fifo);
+    trace_pl011_fifo_tx_xmit_used(count);
+
+    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_consumed(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)) {
@@ -237,14 +263,10 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
                       "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 b2e3d25ae34..3d07866be5c 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -65,6 +65,9 @@ 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_used(unsigned sent) "TX FIFO used %u chars"
+pl011_fifo_tx_xmit_consumed(unsigned sent) "TX FIFO consumed %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.47.1

Re: [PATCH v6 3/7] hw/char/pl011: Introduce pl011_xmit() as GSource

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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>

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

thanks
-- PMM

[PATCH v6 4/7] hw/char/pl011: Trace FIFO enablement

[Philippe Mathieu-Daudé]

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index b9c9e5b5983..447f185e2d5 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -148,6 +148,7 @@ static bool pl011_loopback_enabled(PL011State *s)
 
 static bool pl011_is_fifo_enabled(PL011State *s)
 {
+    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
     return (s->lcr & LCR_FEN) != 0;
 }
 
@@ -464,8 +465,9 @@ 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) {
+            /* Reset the FIFO state on FIFO enable or disable */
+            trace_pl011_fifo_enable(value & LCR_FEN);
             pl011_reset_rx_fifo(s);
             pl011_reset_tx_fifo(s);
         }
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 3d07866be5c..dd635ac6012 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -63,6 +63,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_fifo_enable(bool enable) "enable:%u"
+pl011_fifo_is_enabled(bool enabled) "enabled:%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.47.1

Re: [PATCH v6 4/7] hw/char/pl011: Trace FIFO enablement

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/char/pl011.c      | 4 +++-
>  hw/char/trace-events | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index b9c9e5b5983..447f185e2d5 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -148,6 +148,7 @@ static bool pl011_loopback_enabled(PL011State *s)
>
>  static bool pl011_is_fifo_enabled(PL011State *s)
>  {
> +    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
>      return (s->lcr & LCR_FEN) != 0;

Might be neater having a local variable rather than
repeating the expression twice.

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

thanks
-- PMM

Re: [PATCH v6 4/7] hw/char/pl011: Trace FIFO enablement

[Peter Maydell]

On Mon, 17 Feb 2025 at 14:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/char/pl011.c      | 4 +++-
> >  hw/char/trace-events | 2 ++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index b9c9e5b5983..447f185e2d5 100644
> > --- a/hw/char/pl011.c
> > +++ b/hw/char/pl011.c
> > @@ -148,6 +148,7 @@ static bool pl011_loopback_enabled(PL011State *s)
> >
> >  static bool pl011_is_fifo_enabled(PL011State *s)
> >  {
> > +    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
> >      return (s->lcr & LCR_FEN) != 0;
>
> Might be neater having a local variable rather than
> repeating the expression twice.

I'll squash in this tweak:

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -148,8 +148,10 @@ static bool pl011_loopback_enabled(PL011State *s)

 static bool pl011_is_fifo_enabled(PL011State *s)
 {
-    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
-    return (s->lcr & LCR_FEN) != 0;
+    bool enabled = (s->lcr & LCR_FEN) != 0;
+
+    trace_pl011_fifo_is_enabled(enabled);
+    return enabled;
 }

 static inline unsigned pl011_get_fifo_depth(PL011State *s)

thanks
-- PMM

Re: [PATCH v6 4/7] hw/char/pl011: Trace FIFO enablement

[Philippe Mathieu-Daudé]

On 17/2/25 15:39, Peter Maydell wrote:
> On Mon, 17 Feb 2025 at 14:27, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/char/pl011.c      | 4 +++-
>>>   hw/char/trace-events | 2 ++
>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>>> index b9c9e5b5983..447f185e2d5 100644
>>> --- a/hw/char/pl011.c
>>> +++ b/hw/char/pl011.c
>>> @@ -148,6 +148,7 @@ static bool pl011_loopback_enabled(PL011State *s)
>>>
>>>   static bool pl011_is_fifo_enabled(PL011State *s)
>>>   {
>>> +    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
>>>       return (s->lcr & LCR_FEN) != 0;
>>
>> Might be neater having a local variable rather than
>> repeating the expression twice.
> 
> I'll squash in this tweak:
> 
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -148,8 +148,10 @@ static bool pl011_loopback_enabled(PL011State *s)
> 
>   static bool pl011_is_fifo_enabled(PL011State *s)
>   {
> -    trace_pl011_fifo_is_enabled((s->lcr & LCR_FEN) != 0);
> -    return (s->lcr & LCR_FEN) != 0;
> +    bool enabled = (s->lcr & LCR_FEN) != 0;
> +
> +    trace_pl011_fifo_is_enabled(enabled);
> +    return enabled;
>   }

Thank you :)

[PATCH v6 5/7] 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      | 20 ++++++++++++++++++++
 hw/char/trace-events |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 447f185e2d5..ef39ab666a2 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)
@@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
     return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
 }
 
+static bool pl011_is_tx_fifo_full(PL011State *s)
+{
+    if (pl011_is_fifo_enabled(s)) {
+        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
+        return fifo8_is_full(&s->xmit_fifo);
+    }
+    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
+    return !fifo8_is_empty(&s->xmit_fifo);
+}
+
 static inline void pl011_reset_rx_fifo(PL011State *s)
 {
     s->read_count = 0;
@@ -264,6 +277,13 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
                       "PL011 data written to disabled TX UART\n");
     }
 
+    if (pl011_is_tx_fifo_full(s)) {
+        /* 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 dd635ac6012..8234f3afa13 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -67,9 +67,11 @@ pl011_fifo_enable(bool enable) "enable:%u"
 pl011_fifo_is_enabled(bool enabled) "enabled:%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_is_full(const char *desc, bool full) "mode:%s full:%u"
 pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
 pl011_fifo_tx_xmit_used(unsigned sent) "TX FIFO used %u chars"
 pl011_fifo_tx_xmit_consumed(unsigned sent) "TX FIFO consumed %u 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.47.1

Re: [PATCH v6 5/7] hw/char/pl011: Consider TX FIFO overrun error

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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      | 20 ++++++++++++++++++++
>  hw/char/trace-events |  2 ++
>  2 files changed, 22 insertions(+)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 447f185e2d5..ef39ab666a2 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)
> @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
>      return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
>  }
>
> +static bool pl011_is_tx_fifo_full(PL011State *s)
> +{
> +    if (pl011_is_fifo_enabled(s)) {
> +        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
> +        return fifo8_is_full(&s->xmit_fifo);
> +    }
> +    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
> +    return !fifo8_is_empty(&s->xmit_fifo);

More repetition of expressions, but anyway
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v6 5/7] hw/char/pl011: Consider TX FIFO overrun error

[Peter Maydell]

On Mon, 17 Feb 2025 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > 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      | 20 ++++++++++++++++++++
> >  hw/char/trace-events |  2 ++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index 447f185e2d5..ef39ab666a2 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)
> > @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
> >      return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
> >  }
> >
> > +static bool pl011_is_tx_fifo_full(PL011State *s)
> > +{
> > +    if (pl011_is_fifo_enabled(s)) {
> > +        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
> > +        return fifo8_is_full(&s->xmit_fifo);
> > +    }
> > +    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
> > +    return !fifo8_is_empty(&s->xmit_fifo);
>
> More repetition of expressions, but anyway
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Here I propose to squash in this tweak:

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -165,12 +165,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)

 static bool pl011_is_tx_fifo_full(PL011State *s)
 {
-    if (pl011_is_fifo_enabled(s)) {
-        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
-        return fifo8_is_full(&s->xmit_fifo);
-    }
-    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
-    return !fifo8_is_empty(&s->xmit_fifo);
+    bool fifo_enabled = pl011_is_fifo_enabled(s);
+    bool tx_fifo_full = fifo_enabled ?
+        fifo8_is_full(&s->xmit_fifo) : !fifo8_is_empty(&s->xmit_fifo);
+
+    trace_pl011_fifo_tx_is_full(fifo_enabled ? "FIFO" : "CHAR",
+                                tx_fifo_full);
+    return tx_fifo_full;
 }

thanks
-- PMM

Re: [PATCH v6 5/7] hw/char/pl011: Consider TX FIFO overrun error

[Philippe Mathieu-Daudé]

On 17/2/25 15:52, Peter Maydell wrote:
> On Mon, 17 Feb 2025 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> 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      | 20 ++++++++++++++++++++
>>>   hw/char/trace-events |  2 ++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>>> index 447f185e2d5..ef39ab666a2 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)
>>> @@ -158,6 +161,16 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
>>>       return pl011_is_fifo_enabled(s) ? PL011_FIFO_DEPTH : 1;
>>>   }
>>>
>>> +static bool pl011_is_tx_fifo_full(PL011State *s)
>>> +{
>>> +    if (pl011_is_fifo_enabled(s)) {
>>> +        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
>>> +        return fifo8_is_full(&s->xmit_fifo);
>>> +    }
>>> +    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
>>> +    return !fifo8_is_empty(&s->xmit_fifo);
>>
>> More repetition of expressions, but anyway
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Here I propose to squash in this tweak:
> 
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -165,12 +165,13 @@ static inline unsigned pl011_get_fifo_depth(PL011State *s)
> 
>   static bool pl011_is_tx_fifo_full(PL011State *s)
>   {
> -    if (pl011_is_fifo_enabled(s)) {
> -        trace_pl011_fifo_tx_is_full("FIFO", fifo8_is_full(&s->xmit_fifo));
> -        return fifo8_is_full(&s->xmit_fifo);
> -    }
> -    trace_pl011_fifo_tx_is_full("CHAR", !fifo8_is_empty(&s->xmit_fifo));
> -    return !fifo8_is_empty(&s->xmit_fifo);
> +    bool fifo_enabled = pl011_is_fifo_enabled(s);
> +    bool tx_fifo_full = fifo_enabled ?
> +        fifo8_is_full(&s->xmit_fifo) : !fifo8_is_empty(&s->xmit_fifo);
> +
> +    trace_pl011_fifo_tx_is_full(fifo_enabled ? "FIFO" : "CHAR",
> +                                tx_fifo_full);
> +    return tx_fifo_full;
>   }

Thank you, appreciated!

> 
> thanks
> -- PMM

[PATCH v6 6/7] 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 ef39ab666a2..3c4264869df 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -240,6 +240,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;
@@ -250,6 +257,12 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
     count = fifo8_num_used(&s->xmit_fifo);
     trace_pl011_fifo_tx_xmit_used(count);
 
+    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 8234f3afa13..7d1cba1b4f8 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -72,6 +72,7 @@ pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
 pl011_fifo_tx_xmit_used(unsigned sent) "TX FIFO used %u chars"
 pl011_fifo_tx_xmit_consumed(unsigned sent) "TX FIFO consumed %u 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.47.1

Re: [PATCH v6 6/7] hw/char/pl011: Drain TX FIFO when no backend connected

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> When no character backend is connected, the PL011 frontend
> just drains the FIFO.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

thanks
-- PMM

[PATCH v6 7/7] 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>
---
 hw/char/pl011.c      | 39 +++++++++++++++++++++++++++++++--------
 hw/char/trace-events |  1 +
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 3c4264869df..70eba224a9c 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -251,11 +251,15 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
 {
     PL011State *s = opaque;
     int bytes_consumed;
-    uint8_t data;
+    uint8_t buf[PL011_FIFO_DEPTH];
     uint32_t count;
 
     count = fifo8_num_used(&s->xmit_fifo);
     trace_pl011_fifo_tx_xmit_used(count);
+    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. */
@@ -263,19 +267,29 @@ static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
         return G_SOURCE_REMOVE;
     }
 
-    data = fifo8_pop(&s->xmit_fifo);
-    bytes_consumed = 1;
+    count = fifo8_peek_buf(&s->xmit_fifo, buf, fifo8_num_used(&s->xmit_fifo));
+    trace_pl011_fifo_tx_xmit_peek(count);
 
-    /*
-     * 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, count);
     trace_pl011_fifo_tx_xmit_consumed(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_drop(&s->xmit_fifo, bytes_consumed);
     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;
 }
 
@@ -300,6 +314,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 (pl011_is_tx_fifo_full(s)) {
+        s->flags |= PL011_FLAG_TXFF;
+    }
+
     pl011_xmit(NULL, G_IO_OUT, s);
 }
 
@@ -651,6 +669,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 7d1cba1b4f8..2d02c057483 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -69,6 +69,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_is_full(const char *desc, bool full) "mode:%s full:%u"
 pl011_fifo_tx_put(uint8_t byte) "TX FIFO push char [0x%02x]"
+pl011_fifo_tx_xmit_peek(unsigned sent) "TX FIFO peek %u chars"
 pl011_fifo_tx_xmit_used(unsigned sent) "TX FIFO used %u chars"
 pl011_fifo_tx_xmit_consumed(unsigned sent) "TX FIFO consumed %u chars"
 pl011_fifo_tx_overrun(void) "TX FIFO overrun"
-- 
2.47.1

Re: [PATCH v6 7/7] hw/char/pl011: Implement TX FIFO

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> 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>
> ---

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

thanks
-- PMM

Re: [PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> This series add support for (async) FIFO on the transmit path
> of the PL011 UART.
>

Applied to target-arm.next, thanks (with a couple of minor
tweaks to two of the patches).

-- PMM

Re: [PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Mon, 17 Feb 2025 at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > Hi,
> >
> > This series add support for (async) FIFO on the transmit path
> > of the PL011 UART.
> >
>
> Applied to target-arm.next, thanks (with a couple of minor
> tweaks to two of the patches).

Unfortunately I seem to get failures in 'make check-functional'
with the last patch of this series applied. This is with a
clang sanitizer build on ubuntu 24.04:
'../../configure' '--cc=clang' '--cxx=clang++' '--enable-ubsan'
'--target-list=arm-softmmu,arm-linux-user,aarch64-softmmu,aarch64-linux-user'

and the tests that fail are:
42/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_xlnx_versal     TIMEOUT          90.01s   killed
by signal 15 SIGTERM
43/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_raspi4          TIMEOUT         480.01s   killed
by signal 15 SIGTERM
44/44 qemu:func-thorough+func-aarch64-thorough+thorough /
func-aarch64-aarch64_virt            TIMEOUT         720.01s   killed
by signal 15 SIGTERM

Looking at the test logs it looks like the test framework
starts QEMU but there is never any output from the guest
console.

I've dropped the patchset from target-arm.next for the moment.

thanks
-- PMM

Re: [PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Tue, 18 Feb 2025 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 17 Feb 2025 at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > This series add support for (async) FIFO on the transmit path
> > > of the PL011 UART.
> > >
> >
> > Applied to target-arm.next, thanks (with a couple of minor
> > tweaks to two of the patches).
>
> Unfortunately I seem to get failures in 'make check-functional'
> with the last patch of this series applied.

I had a look at this this morning because I wondered if it
was a mistake in the style fixups I'd applied to the patches
on my end, and I found the bug fairly quickly. The problem is
that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
status flag bits when it removes characters from the FIFO.
So the guest kernel spins forever because TXFF is never unset.

The following patch fixes this for me (and also makes us not
set INT_TX for the case where we couldn't send any bytes to
the chardev, which I noticed reading the code rather than
because it had any visible bad effects):

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -256,6 +256,7 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
     int bytes_consumed;
     uint8_t buf[PL011_FIFO_DEPTH];
     uint32_t count;
+    bool emptied_fifo;

     count = fifo8_num_used(&s->xmit_fifo);
     trace_pl011_fifo_tx_xmit_used(count);
@@ -280,15 +281,24 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
         /* Error in back-end: drain the fifo. */
         pl011_drain_tx(s);
         return G_SOURCE_REMOVE;
+    } else if (bytes_consumed == 0) {
+        /* Couldn't send anything, try again later */
+        return G_SOURCE_CONTINUE;
     }

     /* Pop the data we could transmit. */
     fifo8_drop(&s->xmit_fifo, bytes_consumed);
     s->int_level |= INT_TX;
+    s->flags &= ~PL011_FLAG_TXFF;
+
+    emptied_fifo = fifo8_is_empty(&s->xmit_fifo);
+    if (emptied_fifo) {
+        s->flags |= PL011_FLAG_TXFE;
+    }

     pl011_update(s);

-    if (!fifo8_is_empty(&s->xmit_fifo)) {
+    if (!emptied_fifo) {
         /* Reschedule another transmission if we couldn't transmit all. */
         return G_SOURCE_CONTINUE;
     }

If you're OK with that as a fix then I'll squash it in
and keep the series in target-arm.next.

thanks
-- PMM

Re: [PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Thu, 20 Feb 2025 at 10:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 18 Feb 2025 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 17 Feb 2025 at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > This series add support for (async) FIFO on the transmit path
> > > > of the PL011 UART.
> > > >
> > >
> > > Applied to target-arm.next, thanks (with a couple of minor
> > > tweaks to two of the patches).
> >
> > Unfortunately I seem to get failures in 'make check-functional'
> > with the last patch of this series applied.
>
> I had a look at this this morning because I wondered if it
> was a mistake in the style fixups I'd applied to the patches
> on my end, and I found the bug fairly quickly. The problem is
> that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
> status flag bits when it removes characters from the FIFO.
> So the guest kernel spins forever because TXFF is never unset.
>
> The following patch fixes this for me (and also makes us not
> set INT_TX for the case where we couldn't send any bytes to
> the chardev, which I noticed reading the code rather than
> because it had any visible bad effects):

Hmm, but that's clearly not the only problem -- it fixed the
"no output at all issue", but now I see a test failure because
of garbled console output:

2025-02-20 10:34:51,562: Booting Linux on physical CPU 0x0
2025-02-20 10:34:51,563: Linux version 4.18.16-300.fc29.armv7hl
(mockbuild@buildvm-armv7-06.arm.fedoraproject.org) (gcc version 8.2.1
20180801 (Red Hat 8.2.1-2) (GCC)) #1 SMP Sun Oct 21 00:56:28 UTC 2018
2025-02-20 10:34:51,563: CPU: ARMv7 Processor [414fc0f0] revision 0
(ARMv7), cr=10c5387d
2025-02-20 10:34:51,564: CPU: div instructions available: patching division code
2025-02-20 10:34:51,564: CPU: PIPT / VIPT nonaliasing data cache, PIPT
instruction cache
2025-02-20 10:34:51,565: OF: fdt: Machine model: linux,dummy-virt
2025-02-20 10:34:51,565: Memory policy: Data cache writealloc
2025-02-20 10:34:51,565: efi: Getting EFI parameters from FDT:
2025-02-20 10:34:51,566: efi: UEFI not found.
2025-02-20 10:34:51,566: cma: aetrsv6i
2025-02-20 10:34:51,566: psci: oi ocdteofmT
2025-02-20 10:34:51,566: ps:Sv1ecdniwe^Mps:Usdvni spsc:rt  gtnot psci:
SMC Calling Conventiov0
2025-02-20 10:34:51,567: pec:Eee1pec pvls9 1 2865^MBul1olt bi4^MKerloa
n:itte nltAA
2025-02-20 10:34:51,568: Det c stlere 3 rr4 5i 9(dr336bt)Memr 06112aib
6Keece11K ra0 ,21 sv,Kmrsv,Kim)Virtual kernel memory layout:
2025-02-20 10:34:51,568: vt :xf0  fa  f00 0f00 30 (0M^M lm  c00
0800xe0 0000  2M^M mue:x00 0f00  4B
2025-02-20 10:34:51,568: et x(tvl) (rl 97B
2025-02-20 10:34:51,569: n 0(trvl-xta) 0  0pv) 3 B
2025-02-20 10:34:51,570: bs prl-xta) 5 )random: get_random_u32 ca
___+:ag6 d=3MOes,P=,os^Mfta:aotg78ni  1asHierhlC pmti.        RCrtcnC
oN_S2orpi=.   TasCebd
2025-02-20 10:34:51,571: RC jtgeeyor_o_a1 __d^MNRIS1 rr GIC physical
location is 0x800000^MGIC2 n[mx00-02f,P81]arcte 1tm(
nna65H(r.cemk0fffffacl:0d20,aish_o:6i  M,routo1n per49419sn^MCocrm
dexCalibrating delay ose eceiife..oIl2^Mpxdl6i3
2025-02-20 10:34:51,572: SertFmo ia^MYemgmdl
2025-02-20 10:34:51,572: SEn: ili.Moutcea b te 2(d:,4t^MMoupnch
stlers14oe  9oeyo
2025-02-20 10:34:51,572: CP0Screv:iwein tuia norieI tst lrl
2025-02-20 10:34:51,573: /cpus/cpu@0 missing clock-frequen or^MCPU
rd1c  k  i 00^MSetgptiintm rx30 000Hircc Cmento^MEF rc ln  aae^Msm ii
cdrCs.smp ohu1o, USMPTao1reo cvae(15. gP.CPUA U)tt  Code.devtmpfs:
initialize^Mmo4aheu pr3vit e0cloejfe s fffm_cs0fffm_d_:924250sfut stbs
2025-02-20 10:34:51,574: piclo:niidit uye
2025-02-20 10:34:51,574: DM tre  vi^MNETeDM:rlae2  o tmi oerent aloaios
2025-02-20 10:34:51,574: audit: initializing netlink subsys (disabled)
2025-02-20 10:34:51,574: audit: type=2000 audit(0.480:1):
sat=ntalzdadt_nbe= e=^Mcpuidl:usn oenrmnu
2025-02-20 10:34:51,574: No ATG?
2025-02-20 10:34:51,575: hw-brapit fud5(1rsrvd rapon n  achon
eitrs^Mhw-brapit:mxmmwthontsz s8bts^MSerial MAP01 Ap1:ttAA tMIO09000ud
= 0) saP01 e1
2025-02-20 10:34:51,575: console [ttyAMA0] enabled
2025-02-20 10:34:51,668: cryptd: max_cpu_qlen set to 1000
2025-02-20 10:34:51,690: vgaarb: loaded
2025-02-20 10:34:51,697: SCSI subsystem initialized
2025-02-20 10:34:51,702: usbcore: registered new interface driver usbfs
2025-02-20 10:34:51,703: usbcore: registered new interface driver hub

(the log has no garbling after that so it's presumably OK with
the interrupt-driven real UART driver but the polling one you get
for earlycon has trouble.)

I also noticed that pl011_write_txdata() doesn't clear TXFE
when it puts a byte into the fifo -- I'm testing to see if
fixing that helps.

-- PMM

Re: [PATCH v6 0/7] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Thu, 20 Feb 2025 at 10:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 20 Feb 2025 at 10:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 18 Feb 2025 at 13:54, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Mon, 17 Feb 2025 at 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > >
> > > > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > This series add support for (async) FIFO on the transmit path
> > > > > of the PL011 UART.
> > > > >
> > > >
> > > > Applied to target-arm.next, thanks (with a couple of minor
> > > > tweaks to two of the patches).
> > >
> > > Unfortunately I seem to get failures in 'make check-functional'
> > > with the last patch of this series applied.
> >
> > I had a look at this this morning because I wondered if it
> > was a mistake in the style fixups I'd applied to the patches
> > on my end, and I found the bug fairly quickly. The problem is
> > that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
> > status flag bits when it removes characters from the FIFO.
> > So the guest kernel spins forever because TXFF is never unset.
> >
> > The following patch fixes this for me (and also makes us not
> > set INT_TX for the case where we couldn't send any bytes to
> > the chardev, which I noticed reading the code rather than
> > because it had any visible bad effects):
>
> Hmm, but that's clearly not the only problem -- it fixed the
> "no output at all issue", but now I see a test failure because
> of garbled console output:

> I also noticed that pl011_write_txdata() doesn't clear TXFE
> when it puts a byte into the fifo -- I'm testing to see if
> fixing that helps.

Yes, with this patch also:

--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -330,6 +330,7 @@ static void pl011_write_txdata(PL011State *s, uint8_t data)
     if (pl011_is_tx_fifo_full(s)) {
         s->flags |= PL011_FLAG_TXFF;
     }
+    s->flags &= ~PL011_FLAG_TXFE;

     pl011_xmit(NULL, G_IO_OUT, s);
 }

this remaining failure is fixed and I get a clean pass (other than
the gpu test failure, but that's not related to the pl011).

-- PMM