[PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

Missing review: #10

Hi,

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

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é (10):
  util/fifo8: Allow fifo8_pop_buf() to not populate popped length
  util/fifo8: Introduce fifo8_peek_buf()
  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: Check if receiver is enabled
  hw/char/pl011: Rename RX FIFO methods
  hw/char/pl011: Add transmit FIFO to PL011State
  hw/char/pl011: Implement TX FIFO

 include/hw/char/pl011.h |   2 +
 include/qemu/fifo8.h    |  37 ++++++-
 hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
 util/fifo8.c            |  28 ++++-
 hw/char/trace-events    |   8 +-
 5 files changed, 263 insertions(+), 51 deletions(-)

-- 
2.41.0

[PATCH-for-8.2 v4 01/10] util/fifo8: Allow fifo8_pop_buf() to not populate popped length

[Philippe Mathieu-Daudé]

There might be cases where we know the number of bytes we can
pop from the FIFO, or we simply don't care how many bytes is
returned. Allow fifo8_pop_buf() to take a NULL numptr.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/fifo8.h | 10 +++++-----
 util/fifo8.c         | 12 ++++++++----
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index 16be02f361..d0d02bc73d 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -71,7 +71,7 @@ uint8_t fifo8_pop(Fifo8 *fifo);
  * fifo8_pop_buf:
  * @fifo: FIFO to pop from
  * @max: maximum number of bytes to pop
- * @num: actual number of returned bytes
+ * @numptr: pointer filled with number of bytes returned (can be NULL)
  *
  * Pop a number of elements from the FIFO up to a maximum of max. The buffer
  * containing the popped data is returned. This buffer points directly into
@@ -82,16 +82,16 @@ uint8_t fifo8_pop(Fifo8 *fifo);
  * around in the ring buffer; in this case only a contiguous part of the data
  * is returned.
  *
- * The number of valid bytes returned is populated in *num; will always return
- * at least 1 byte. max must not be 0 or greater than the number of bytes in
- * the FIFO.
+ * The number of valid bytes returned is populated in *numptr; will always
+ * return at least 1 byte. max must not be 0 or greater than the number of
+ * bytes in the FIFO.
  *
  * Clients are responsible for checking the availability of requested data
  * using fifo8_num_used().
  *
  * Returns: A pointer to popped data.
  */
-const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num);
+const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
 
 /**
  * fifo8_reset:
diff --git a/util/fifo8.c b/util/fifo8.c
index d4d1c135e0..032e985440 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -66,16 +66,20 @@ uint8_t fifo8_pop(Fifo8 *fifo)
     return ret;
 }
 
-const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
+const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
 {
     uint8_t *ret;
+    uint32_t num;
 
     assert(max > 0 && max <= fifo->num);
-    *num = MIN(fifo->capacity - fifo->head, max);
+    num = MIN(fifo->capacity - fifo->head, max);
     ret = &fifo->data[fifo->head];
-    fifo->head += *num;
+    fifo->head += num;
     fifo->head %= fifo->capacity;
-    fifo->num -= *num;
+    fifo->num -= num;
+    if (numptr) {
+        *numptr = num;
+    }
     return ret;
 }
 
-- 
2.41.0

[PATCH-for-8.2 v4 02/10] util/fifo8: Introduce fifo8_peek_buf()

[Philippe Mathieu-Daudé]

To be able to peek at FIFO content without popping it,
introduce the fifo8_peek_buf() method by factoring
common content from fifo8_pop_buf().

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/fifo8.h | 27 +++++++++++++++++++++++++++
 util/fifo8.c         | 22 ++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index d0d02bc73d..c6295c6ff0 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -93,6 +93,33 @@ uint8_t fifo8_pop(Fifo8 *fifo);
  */
 const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
 
+/**
+ * fifo8_peek_buf: read upto max bytes from the fifo
+ * @fifo: FIFO to read from
+ * @max: maximum number of bytes to peek
+ * @numptr: pointer filled with number of bytes returned (can be NULL)
+ *
+ * Peek into a number of elements from the FIFO up to a maximum of max.
+ * The buffer containing the data peeked into is returned. This buffer points
+ * directly into the FIFO backing store. Since data is invalidated once any
+ * of the fifo8_* APIs are called on the FIFO, it is the caller responsibility
+ * to access it before doing further API calls.
+ *
+ * The function may return fewer bytes than requested when the data wraps
+ * around in the ring buffer; in this case only a contiguous part of the data
+ * is returned.
+ *
+ * The number of valid bytes returned is populated in *numptr; will always
+ * return at least 1 byte. max must not be 0 or greater than the number of
+ * bytes in the FIFO.
+ *
+ * Clients are responsible for checking the availability of requested data
+ * using fifo8_num_used().
+ *
+ * Returns: A pointer to peekable data.
+ */
+const uint8_t *fifo8_peek_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
+
 /**
  * fifo8_reset:
  * @fifo: FIFO to reset
diff --git a/util/fifo8.c b/util/fifo8.c
index 032e985440..e12477843e 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -66,7 +66,8 @@ uint8_t fifo8_pop(Fifo8 *fifo)
     return ret;
 }
 
-const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
+static const uint8_t *fifo8_peekpop_buf(Fifo8 *fifo, uint32_t max,
+                                        uint32_t *numptr, bool do_pop)
 {
     uint8_t *ret;
     uint32_t num;
@@ -74,15 +75,28 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
     assert(max > 0 && max <= fifo->num);
     num = MIN(fifo->capacity - fifo->head, max);
     ret = &fifo->data[fifo->head];
-    fifo->head += num;
-    fifo->head %= fifo->capacity;
-    fifo->num -= num;
+
+    if (do_pop) {
+        fifo->head += num;
+        fifo->head %= fifo->capacity;
+        fifo->num -= num;
+    }
     if (numptr) {
         *numptr = num;
     }
     return ret;
 }
 
+const uint8_t *fifo8_peek_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
+{
+    return fifo8_peekpop_buf(fifo, max, numptr, false);
+}
+
+const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr)
+{
+    return fifo8_peekpop_buf(fifo, max, numptr, true);
+}
+
 void fifo8_reset(Fifo8 *fifo)
 {
     fifo->num = 0;
-- 
2.41.0

[PATCH-for-8.2 v4 03/10] 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 58edeb9ddb..1f07c7b021 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -132,14 +132,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 uint64_t pl011_read(void *opaque, hwaddr offset,
@@ -289,7 +296,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;
@@ -506,7 +514,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-for-8.2 v4 04/10] 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 | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 1f07c7b021..1cb9015ea2 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -149,6 +149,17 @@ static inline void pl011_reset_tx_fifo(PL011State *s)
     s->flags |= PL011_FLAG_TXFE;
 }
 
+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);
+    s->int_level |= INT_TX;
+    pl011_update(s);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -262,19 +273,13 @@ static void pl011_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
     PL011State *s = (PL011State *)opaque;
-    unsigned char ch;
 
     trace_pl011_write(offset, value, pl011_regname(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);
-        s->int_level |= INT_TX;
-        pl011_update(s);
+        s->readbuff = value;
+        pl011_write_txdata(s, value);
         break;
     case 1: /* UARTRSR/UARTECR */
         s->rsr = 0;
-- 
2.41.0

[PATCH-for-8.2 v4 05/10] 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 1cb9015ea2..30309337b1 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -160,31 +160,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-for-8.2 v4 06/10] 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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 30309337b1..9c43cb47bf 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -76,6 +76,10 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define LCR_FEN     (1 << 4)
 #define LCR_BRK     (1 << 0)
 
+/* Control Register, UARTCR */
+#define CR_TXE      (1 << 8)
+#define CR_UARTEN   (1 << 0)
+
 static const unsigned char pl011_id_arm[8] =
   { 0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
 static const unsigned char pl011_id_luminary[8] =
@@ -151,7 +155,12 @@ static inline void pl011_reset_tx_fifo(PL011State *s)
 
 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-for-8.2 v4 07/10] 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 9c43cb47bf..ca931be139 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -77,6 +77,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define LCR_BRK     (1 << 0)
 
 /* Control Register, UARTCR */
+#define CR_RXE      (1 << 9)
 #define CR_TXE      (1 << 8)
 #define CR_UARTEN   (1 << 0)
 
@@ -359,9 +360,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

[PATCH-for-8.2 v4 08/10] 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      | 10 +++++-----
 hw/char/trace-events |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index ca931be139..727decd428 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -369,7 +369,7 @@ static int pl011_can_receive(void *opaque)
     return r;
 }
 
-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;
@@ -380,9 +380,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) {
@@ -393,13 +393,13 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
 
 static void pl011_receive(void *opaque, const uint8_t *buf, int size)
 {
-    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_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 7a398c82a5..bc9e84261f 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-for-8.2 v4 09/10] 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.

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.

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 d853802132..20898f43a6 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)
@@ -53,6 +54,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 727decd428..f474f56780 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -147,11 +147,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_write_txdata(PL011State *s, uint8_t data)
@@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
     }
 };
 
+static bool pl011_xmit_fifo_state_needed(void *opaque)
+{
+    PL011State* s = opaque;
+
+    return !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;
@@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
     .subsections = (const VMStateDescription * []) {
         &vmstate_pl011_clock,
         NULL
-    }
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_pl011_xmit_fifo,
+        NULL
+    },
 };
 
 static Property pl011_properties[] = {
@@ -502,6 +526,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++) {
@@ -514,6 +539,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);
@@ -557,6 +589,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-for-8.2 v4 10/10] 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:
https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574

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

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

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      | 107 ++++++++++++++++++++++++++++++++++++++++---
 hw/char/trace-events |   4 ++
 2 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f474f56780..a14ece4f07 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -57,6 +57,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)
@@ -156,6 +159,68 @@ static void pl011_reset_tx_fifo(PL011State *s)
     fifo8_reset(&s->xmit_fifo);
 }
 
+static gboolean 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;
+    return G_SOURCE_REMOVE;
+}
+
+static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
+{
+    PL011State *s = opaque;
+    int ret;
+    const uint8_t *buf;
+    uint32_t buflen;
+    uint32_t count;
+    bool tx_enabled;
+
+    tx_enabled = (s->cr & CR_UARTEN) && (s->cr & CR_TXE);
+    if (!tx_enabled) {
+        /*
+         * If TX is disabled, nothing to do.
+         * Keep the potentially used FIFO as is.
+         */
+        return G_SOURCE_REMOVE;
+    }
+
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
+        /* Instant drain the fifo when there's no back-end */
+        return pl011_drain_tx(s);
+    }
+
+    count = fifo8_num_used(&s->xmit_fifo);
+    if (count < 1) {
+        /* FIFO empty */
+        return G_SOURCE_REMOVE;
+    }
+
+    /* Transmit as much data as we can */
+    buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen);
+    ret = qemu_chr_fe_write(&s->chr, buf, buflen);
+    if (ret >= 0) {
+        /* Pop the data we could transmit */
+        trace_pl011_fifo_tx_xmit(ret);
+        fifo8_pop_buf(&s->xmit_fifo, ret, NULL);
+        s->int_level |= INT_TX;
+    }
+
+    if (!fifo8_is_empty(&s->xmit_fifo)) {
+        /* Reschedule another transmission if we couldn't transmit all */
+        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                        pl011_xmit, s);
+        if (!r) {
+            /* Error in back-end? */
+            return pl011_drain_tx(s);
+        }
+    }
+
+    pl011_update(s);
+
+    return G_SOURCE_REMOVE;
+}
+
 static void pl011_write_txdata(PL011State *s, uint8_t data)
 {
     if (!(s->cr & CR_UARTEN)) {
@@ -165,11 +230,25 @@ 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);
-    s->int_level |= INT_TX;
-    pl011_update(s);
+    if (fifo8_is_full(&s->xmit_fifo)) {
+        /*
+         * The FIFO contents remain valid because no more data is
+         * written when the FIFO is full, only the contents of the
+         * shift register are overwritten. The CPU must now read
+         * the data, to empty the FIFO.
+         */
+        trace_pl011_fifo_tx_overrun();
+        s->rsr |= RSR_OE;
+        return;
+    }
+
+    trace_pl011_fifo_tx_put(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);
 }
 
 static uint32_t pl011_read_rxdata(PL011State *s)
@@ -331,10 +410,21 @@ static void pl011_write(void *opaque, hwaddr offset,
         s->lcr = value;
         pl011_set_read_trigger(s);
         break;
-    case 12: /* UARTCR */
+    case 12: /* UARTCR */ {
+        uint16_t en_bits = s->cr & (CR_UARTEN | CR_TXE | CR_RXE);
+        uint16_t dis_bits = value & (CR_UARTEN | CR_TXE | CR_RXE);
+        if (en_bits ^ dis_bits && !fifo8_is_empty(&s->xmit_fifo)) {
+            /*
+             * If the UART is disabled in the middle of transmission
+             * or reception, it completes the current character before
+             * stopping.
+             */
+            pl011_xmit(NULL, G_IO_OUT, s);
+        }
         /* ??? Need to implement the enable and loopback bits.  */
         s->cr = value;
         break;
+    }
     case 13: /* UARTIFS */
         s->ifl = value;
         pl011_set_read_trigger(s);
@@ -477,6 +567,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);
+    }
+
     return 0;
 }
 
diff --git a/hw/char/trace-events b/hw/char/trace-events
index bc9e84261f..ee00af0c66 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -60,6 +60,10 @@ 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 [0x%02x]"
+pl011_fifo_tx_xmit(int count) "TX FIFO pop %d"
+pl011_fifo_tx_overrun(void) "TX FIFO overrun"
+pl011_fifo_tx_drain(unsigned drained) "TX FIFO draining %u"
 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

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Missing review: #10
>
> Hi,
>
> This series add support for (async) FIFO on the transmit path
> of the PL011 UART.

Hi; what's the rationale for the "for-8.2" targeting here?
What bug are we fixing?

thanks
-- PMM

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

Hi Peter,

On 9/11/23 20:29, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Missing review: #10
>>
>> Hi,
>>
>> This series add support for (async) FIFO on the transmit path
>> of the PL011 UART.
> 
> Hi; what's the rationale for the "for-8.2" targeting here?
> What bug are we fixing?

The bug is on Trusted Substrate when the ZynqMP machine is used:
https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574

Regards,

Phil.

Re: [PATCH-for-8.2 v4 01/10] util/fifo8: Allow fifo8_pop_buf() to not populate popped length

[Richard Henderson]

On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> There might be cases where we know the number of bytes we can
> pop from the FIFO, or we simply don't care how many bytes is
> returned. Allow fifo8_pop_buf() to take a NULL numptr.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-8.2 v4 02/10] util/fifo8: Introduce fifo8_peek_buf()

[Richard Henderson]

On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> To be able to peek at FIFO content without popping it,
> introduce the fifo8_peek_buf() method by factoring
> common content from fifo8_pop_buf().
> 
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/qemu/fifo8.h | 27 +++++++++++++++++++++++++++
>   util/fifo8.c         | 22 ++++++++++++++++++----
>   2 files changed, 45 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH-for-8.2 v4 04/10] hw/char/pl011: Extract pl011_write_txdata() from pl011_write()

[Richard Henderson]

On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> 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.

...
> @@ -262,19 +273,13 @@ static void pl011_write(void *opaque, hwaddr offset,
>                           uint64_t value, unsigned size)
>   {
>       PL011State *s = (PL011State *)opaque;
> -    unsigned char ch;
>   
>       trace_pl011_write(offset, value, pl011_regname(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);
> -        s->int_level |= INT_TX;
> -        pl011_update(s);
> +        s->readbuff = value;

Why the write to readbuff?


r~

Re: [PATCH-for-8.2 v4 09/10] hw/char/pl011: Add transmit FIFO to PL011State

[Richard Henderson]

On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>       }
>   };
>   
> +static bool pl011_xmit_fifo_state_needed(void *opaque)
> +{
> +    PL011State* s = opaque;
> +
> +    return !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;
> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>       .subsections = (const VMStateDescription * []) {
>           &vmstate_pl011_clock,
>           NULL
> -    }
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_pl011_xmit_fifo,
> +        NULL
> +    },
>   };

It just occurred to me that you may need a vmstate_pl011 pre_load() to empty the FIFO, 
which will then be filled if and only if the saved vmstate_pl011_xmit_fifo subsection is 
present.

Juan, have I got this correct about how migration would or should handle a missing subsection?


r~

Re: [PATCH-for-8.2 v4 10/10] hw/char/pl011: Implement TX FIFO

[Richard Henderson]

On 11/9/23 11:28, 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:
> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
> 
> 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).
> 
> We only migrate the TX FIFO if it is in use.
> 
> 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      | 107 ++++++++++++++++++++++++++++++++++++++++---
>   hw/char/trace-events |   4 ++
>   2 files changed, 105 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index f474f56780..a14ece4f07 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -57,6 +57,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)
> @@ -156,6 +159,68 @@ static void pl011_reset_tx_fifo(PL011State *s)
>       fifo8_reset(&s->xmit_fifo);
>   }
>   
> +static gboolean 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;
> +    return G_SOURCE_REMOVE;
> +}
> +
> +static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
> +{
> +    PL011State *s = opaque;
> +    int ret;
> +    const uint8_t *buf;
> +    uint32_t buflen;
> +    uint32_t count;
> +    bool tx_enabled;
> +
> +    tx_enabled = (s->cr & CR_UARTEN) && (s->cr & CR_TXE);
> +    if (!tx_enabled) {
> +        /*
> +         * If TX is disabled, nothing to do.
> +         * Keep the potentially used FIFO as is.
> +         */
> +        return G_SOURCE_REMOVE;
> +    }
> +
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        /* Instant drain the fifo when there's no back-end */
> +        return pl011_drain_tx(s);
> +    }
> +
> +    count = fifo8_num_used(&s->xmit_fifo);
> +    if (count < 1) {
> +        /* FIFO empty */
> +        return G_SOURCE_REMOVE;
> +    }

Could swap these two blocks.  Certainly the fifo does not need draining if it is empty...

> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
> +        /* Reschedule another transmission if we couldn't transmit all */
> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                        pl011_xmit, s);
> +        if (!r) {
> +            /* Error in back-end? */
> +            return pl011_drain_tx(s);
> +        }
> +    }
> +
> +    pl011_update(s);
> +
> +    return G_SOURCE_REMOVE;

The documentation for FEWatchFunc says you should be returning G_SOURCE_CONTINUE to leave 
the source in the main loop.  That certainly makes more sense than re-adding a watch when 
the fifo is not empty.  There's also no error handling to do then.  Just


     pl011_update(s);
     return fifo8_is_empty(&s->xmit_fifo) ? G_SOURCE_REMOVE : G_SOURCE_CONTINUE;

> +    case 12: /* UARTCR */ {
> +        uint16_t en_bits = s->cr & (CR_UARTEN | CR_TXE | CR_RXE);
> +        uint16_t dis_bits = value & (CR_UARTEN | CR_TXE | CR_RXE);
> +        if (en_bits ^ dis_bits && !fifo8_is_empty(&s->xmit_fifo)) {
> +            /*
> +             * If the UART is disabled in the middle of transmission
> +             * or reception, it completes the current character before
> +             * stopping.
> +             */
> +            pl011_xmit(NULL, G_IO_OUT, s);

This seems wrong.  You don't know that the host device is ready.

We will never transmit a partial character because the host will never accept less than 
one character at a time.  Therefore there's absolutely nothing we need to do in order to 
"complete the current character", which is the one we started with the *previous* pl011_xmit.

Just set the CR bits with no further comment.


r~

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Peter Maydell]

On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 9/11/23 20:29, Peter Maydell wrote:
> > On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Missing review: #10
> >>
> >> Hi,
> >>
> >> This series add support for (async) FIFO on the transmit path
> >> of the PL011 UART.
> >
> > Hi; what's the rationale for the "for-8.2" targeting here?
> > What bug are we fixing?
>
> The bug is on Trusted Substrate when the ZynqMP machine is used:
> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574

And have we confirmed that the async FIFO support fixes that problem?
That bug report seems to have mostly just speculation in it that
maybe this XXX comment is why...

-- PMM

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Philippe Mathieu-Daudé]

Hi Peter,

Cc'ing Mikko.

On 13/11/23 14:11, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 9/11/23 20:29, Peter Maydell wrote:
>>> On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> Missing review: #10
>>>>
>>>> Hi,
>>>>
>>>> This series add support for (async) FIFO on the transmit path
>>>> of the PL011 UART.
>>>
>>> Hi; what's the rationale for the "for-8.2" targeting here?
>>> What bug are we fixing?
>>
>> The bug is on Trusted Substrate when the ZynqMP machine is used:
>> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
> 
> And have we confirmed that the async FIFO support fixes that problem?
> That bug report seems to have mostly just speculation in it that
> maybe this XXX comment is why...

Mikko tested the v2 (or v1?) and confirmed it was fixing their problem
with TRS.

That said, besides the v4 last-minute review from Richard just before
his US holiday WE, I have to sadly recognize -- although I could argue
this is a bug fix -- this series is not yet ready, thus will miss the
8.2 release :(

(Mikko: no need to test this one, I'll Cc you on the next one to get
  your Tested-by tag on the list).

Regards,

Phil.

Re: [PATCH-for-8.2 v4 09/10] hw/char/pl011: Add transmit FIFO to PL011State

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>>       }
>>   };
>>   +static bool pl011_xmit_fifo_state_needed(void *opaque)
>> +{
>> +    PL011State* s = opaque;
>> +
>> +    return !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;
>> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>>       .subsections = (const VMStateDescription * []) {
>>           &vmstate_pl011_clock,
>>           NULL
>> -    }
>> +    },
>> +    .subsections = (const VMStateDescription * []) {
>> +        &vmstate_pl011_xmit_fifo,
>> +        NULL
>> +    },
>>   };
>
> It just occurred to me that you may need a vmstate_pl011 pre_load() to
> empty the FIFO, which will then be filled if and only if the saved
> vmstate_pl011_xmit_fifo subsection is present.
>
> Juan, have I got this correct about how migration would or should handle a missing subsection?

I hav'nt looked about how the device is created. But if it is created
with the fifo empty you don't need the pre_load().

I have no idea about this device, but sometimes it just happens that if
the fifo has data, you need to put an irq somewhere or mark it some
place that there is pending job on this device.

Later, Juan.

Re: [PATCH-for-8.2 v4 10/10] hw/char/pl011: Implement TX FIFO

[Marc-André Lureau]

Hi

On Thu, Nov 9, 2023 at 11:30 PM 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:
> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
>
> 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.

I do not have access to that Jira issue.

In general, chardev backends should have some buffering already
(socket, files etc).

If we want more, or extra control over buffering, maybe this should be
implemented at the chardev level, rather than each frontend implement
its own extra buffering logic...

Regardless, I think frontends should have an option to "drop" data
when the chardev/buffer is full, rather than hanging.


>
> Similarly to the RX FIFO path, FIFO level selection is not
> implemented (interrupt is triggered when a single byte is available
> in the FIFO).
>
> We only migrate the TX FIFO if it is in use.
>
> 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      | 107 ++++++++++++++++++++++++++++++++++++++++---
>  hw/char/trace-events |   4 ++
>  2 files changed, 105 insertions(+), 6 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index f474f56780..a14ece4f07 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -57,6 +57,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)
> @@ -156,6 +159,68 @@ static void pl011_reset_tx_fifo(PL011State *s)
>      fifo8_reset(&s->xmit_fifo);
>  }
>
> +static gboolean 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;
> +    return G_SOURCE_REMOVE;
> +}
> +
> +static gboolean pl011_xmit(void *do_not_use, GIOCondition cond, void *opaque)
> +{
> +    PL011State *s = opaque;
> +    int ret;
> +    const uint8_t *buf;
> +    uint32_t buflen;
> +    uint32_t count;
> +    bool tx_enabled;
> +
> +    tx_enabled = (s->cr & CR_UARTEN) && (s->cr & CR_TXE);
> +    if (!tx_enabled) {
> +        /*
> +         * If TX is disabled, nothing to do.
> +         * Keep the potentially used FIFO as is.
> +         */
> +        return G_SOURCE_REMOVE;
> +    }
> +
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
> +        /* Instant drain the fifo when there's no back-end */
> +        return pl011_drain_tx(s);
> +    }
> +
> +    count = fifo8_num_used(&s->xmit_fifo);
> +    if (count < 1) {
> +        /* FIFO empty */
> +        return G_SOURCE_REMOVE;
> +    }
> +
> +    /* Transmit as much data as we can */
> +    buf = fifo8_peek_buf(&s->xmit_fifo, count, &buflen);
> +    ret = qemu_chr_fe_write(&s->chr, buf, buflen);
> +    if (ret >= 0) {
> +        /* Pop the data we could transmit */
> +        trace_pl011_fifo_tx_xmit(ret);
> +        fifo8_pop_buf(&s->xmit_fifo, ret, NULL);
> +        s->int_level |= INT_TX;
> +    }
> +
> +    if (!fifo8_is_empty(&s->xmit_fifo)) {
> +        /* Reschedule another transmission if we couldn't transmit all */
> +        guint r = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
> +                                        pl011_xmit, s);
> +        if (!r) {
> +            /* Error in back-end? */
> +            return pl011_drain_tx(s);
> +        }
> +    }
> +
> +    pl011_update(s);
> +
> +    return G_SOURCE_REMOVE;
> +}
> +
>  static void pl011_write_txdata(PL011State *s, uint8_t data)
>  {
>      if (!(s->cr & CR_UARTEN)) {
> @@ -165,11 +230,25 @@ 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);
> -    s->int_level |= INT_TX;
> -    pl011_update(s);
> +    if (fifo8_is_full(&s->xmit_fifo)) {
> +        /*
> +         * The FIFO contents remain valid because no more data is
> +         * written when the FIFO is full, only the contents of the
> +         * shift register are overwritten. The CPU must now read
> +         * the data, to empty the FIFO.
> +         */
> +        trace_pl011_fifo_tx_overrun();
> +        s->rsr |= RSR_OE;
> +        return;
> +    }
> +
> +    trace_pl011_fifo_tx_put(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);
>  }
>
>  static uint32_t pl011_read_rxdata(PL011State *s)
> @@ -331,10 +410,21 @@ static void pl011_write(void *opaque, hwaddr offset,
>          s->lcr = value;
>          pl011_set_read_trigger(s);
>          break;
> -    case 12: /* UARTCR */
> +    case 12: /* UARTCR */ {
> +        uint16_t en_bits = s->cr & (CR_UARTEN | CR_TXE | CR_RXE);
> +        uint16_t dis_bits = value & (CR_UARTEN | CR_TXE | CR_RXE);
> +        if (en_bits ^ dis_bits && !fifo8_is_empty(&s->xmit_fifo)) {
> +            /*
> +             * If the UART is disabled in the middle of transmission
> +             * or reception, it completes the current character before
> +             * stopping.
> +             */
> +            pl011_xmit(NULL, G_IO_OUT, s);
> +        }
>          /* ??? Need to implement the enable and loopback bits.  */
>          s->cr = value;
>          break;
> +    }
>      case 13: /* UARTIFS */
>          s->ifl = value;
>          pl011_set_read_trigger(s);
> @@ -477,6 +567,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);
> +    }
> +
>      return 0;
>  }
>
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index bc9e84261f..ee00af0c66 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -60,6 +60,10 @@ 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 [0x%02x]"
> +pl011_fifo_tx_xmit(int count) "TX FIFO pop %d"
> +pl011_fifo_tx_overrun(void) "TX FIFO overrun"
> +pl011_fifo_tx_drain(unsigned drained) "TX FIFO draining %u"
>  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
>
>


--
Marc-André Lureau

Re: [PATCH-for-8.2 v4 10/10] hw/char/pl011: Implement TX FIFO

[Daniel P. Berrangé]

On Wed, Nov 22, 2023 at 02:31:29PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Nov 9, 2023 at 11:30 PM 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:
> > https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
> >
> > 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.
> 
> I do not have access to that Jira issue.
> 
> In general, chardev backends should have some buffering already
> (socket, files etc).
> 
> If we want more, or extra control over buffering, maybe this should be
> implemented at the chardev level, rather than each frontend implement
> its own extra buffering logic...
> 
> Regardless, I think frontends should have an option to "drop" data
> when the chardev/buffer is full, rather than hanging.

Does anyone really want data to be dropped by QEMU ? Every time I've seen
a scenario where data has been dropped or lost, it has been considered
a bug to be solved.

Sure, we don't want QEMU to block on chardev writes, but we want that
more than throwing away data.

What's the use case for capturing data from the serial port, but throwing
it away if the other end of a socket doesn't read quickly enough ?

If someone does want lossy serial ports, they could configure the UDP
charedev backend already.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 9 Nov 2023 at 20:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 9/11/23 20:29, Peter Maydell wrote:
>> > On Thu, 9 Nov 2023 at 19:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> >>
>> >> Missing review: #10
>> >>
>> >> Hi,
>> >>
>> >> This series add support for (async) FIFO on the transmit path
>> >> of the PL011 UART.
>> >
>> > Hi; what's the rationale for the "for-8.2" targeting here?
>> > What bug are we fixing?
>>
>> The bug is on Trusted Substrate when the ZynqMP machine is used:
>> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
>
> And have we confirmed that the async FIFO support fixes that problem?
> That bug report seems to have mostly just speculation in it that
> maybe this XXX comment is why...

I've been fighting with numerous issues with the TRS build over the last
week so I can confirm I have seen a) a lock up with pl011_write blocking
everything under the BQL because data wasn't read fast enough and b) the
problem goes away with Philippe's patches. So have a:

Tested-by: Alex Bennée <alex.bennee@linaro.org>

for the series.

>
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH-for-8.2 v4 10/10] hw/char/pl011: Implement TX FIFO

[Philippe Mathieu-Daudé]

On 22/11/23 11:38, Daniel P. Berrangé wrote:
> On Wed, Nov 22, 2023 at 02:31:29PM +0400, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Nov 9, 2023 at 11:30 PM 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:
>>> https://linaro.atlassian.net/browse/TRS-149?focusedCommentId=149574
>>>
>>> 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.
>>
>> I do not have access to that Jira issue.
>>
>> In general, chardev backends should have some buffering already
>> (socket, files etc).
>>
>> If we want more, or extra control over buffering, maybe this should be
>> implemented at the chardev level, rather than each frontend implement
>> its own extra buffering logic...
>>
>> Regardless, I think frontends should have an option to "drop" data
>> when the chardev/buffer is full, rather than hanging.
> 
> Does anyone really want data to be dropped by QEMU ? Every time I've seen
> a scenario where data has been dropped or lost, it has been considered
> a bug to be solved.

A kind of counter example is the RX UART model, which is used in
embedded world and respects the baudrate timing. I guess some scripts
were working with the QEMU UART chardev, but them the same script
failed when using HW UART, so realistic HW baudrate was emulated using
the timer API. See the chardev frontend handlers:

static int can_receive(void *opaque)
{
     RSCIState *sci = RSCI(opaque);
     if (sci->rx_next > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
         return 0;
     } else {
         return FIELD_EX8(sci->scr, SCR, RE);
     }
}

The TX path also use a timer:

static void send_byte(RSCIState *sci)
{
     if (qemu_chr_fe_backend_connected(&sci->chr)) {
         qemu_chr_fe_write_all(&sci->chr, &sci->tdr, 1);
     }
     timer_mod(&sci->timer,
              qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + sci->trtime);

The more complex 16550A UART model also use timer in FIFO mode.

> Sure, we don't want QEMU to block on chardev writes, but we want that
> more than throwing away data.
> 
> What's the use case for capturing data from the serial port, but throwing
> it away if the other end of a socket doesn't read quickly enough ?
> 
> If someone does want lossy serial ports, they could configure the UDP
> charedev backend already.
> 
> With regards,
> Daniel

Re: [PATCH-for-8.2 v4 04/10] hw/char/pl011: Extract pl011_write_txdata() from pl011_write()

[Philippe Mathieu-Daudé]

On 10/11/23 00:17, Richard Henderson wrote:
> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>> 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.
> 
> ...
>> @@ -262,19 +273,13 @@ static void pl011_write(void *opaque, hwaddr 
>> offset,
>>                           uint64_t value, unsigned size)
>>   {
>>       PL011State *s = (PL011State *)opaque;
>> -    unsigned char ch;
>>       trace_pl011_write(offset, value, pl011_regname(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);
>> -        s->int_level |= INT_TX;
>> -        pl011_update(s);
>> +        s->readbuff = value;
> 
> Why the write to readbuff?

I think I wanted to use it when FIFO is disabled due to:

https://developer.arm.com/documentation/ddi0183/g/programmers-model/register-descriptions/line-control-register--uartlcr-h?lang=en

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

and we don't have a fifo8_change_capacity() method.

Otherwise, not sure what for is this field... I'll see if we can
just remove it.

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Mark Cave-Ayland]

On 09/11/2023 19:28, Philippe Mathieu-Daudé wrote:

> Missing review: #10
> 
> Hi,
> 
> This series add support for (async) FIFO on the transmit path
> of the PL011 UART.
> 
> 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é (10):
>    util/fifo8: Allow fifo8_pop_buf() to not populate popped length
>    util/fifo8: Introduce fifo8_peek_buf()
>    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: Check if receiver is enabled
>    hw/char/pl011: Rename RX FIFO methods
>    hw/char/pl011: Add transmit FIFO to PL011State
>    hw/char/pl011: Implement TX FIFO
> 
>   include/hw/char/pl011.h |   2 +
>   include/qemu/fifo8.h    |  37 ++++++-
>   hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
>   util/fifo8.c            |  28 ++++-
>   hw/char/trace-events    |   8 +-
>   5 files changed, 263 insertions(+), 51 deletions(-)

Hi Phil,

Happy New Year! Are there plans to queue this series for 9.0 soon? I'm particularly 
interested in the first 2 patches as I've made use of the new fifo8_peek_buf() 
function as part of my latest ESP updates.


ATB,

Mark.

Re: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop

[Mark Cave-Ayland]

On 05/01/2024 07:50, Mark Cave-Ayland wrote:

> On 09/11/2023 19:28, Philippe Mathieu-Daudé wrote:
> 
>> Missing review: #10
>>
>> Hi,
>>
>> This series add support for (async) FIFO on the transmit path
>> of the PL011 UART.
>>
>> 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é (10):
>>    util/fifo8: Allow fifo8_pop_buf() to not populate popped length
>>    util/fifo8: Introduce fifo8_peek_buf()
>>    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: Check if receiver is enabled
>>    hw/char/pl011: Rename RX FIFO methods
>>    hw/char/pl011: Add transmit FIFO to PL011State
>>    hw/char/pl011: Implement TX FIFO
>>
>>   include/hw/char/pl011.h |   2 +
>>   include/qemu/fifo8.h    |  37 ++++++-
>>   hw/char/pl011.c         | 239 +++++++++++++++++++++++++++++++++-------
>>   util/fifo8.c            |  28 ++++-
>>   hw/char/trace-events    |   8 +-
>>   5 files changed, 263 insertions(+), 51 deletions(-)
> 
> Hi Phil,
> 
> Happy New Year! Are there plans to queue this series for 9.0 soon? I'm particularly 
> interested in the first 2 patches as I've made use of the new fifo8_peek_buf() 
> function as part of my latest ESP updates.

I've spoken to Phil, and as patches 1 and 2 implementing fifo8_peek_buf() have R-B 
tags he is happy for me to take them separately via my qemu-sparc branch. I'll send a 
PR with those patches shortly.


ATB,

Mark.

Re: [PATCH-for-8.2 v4 09/10] hw/char/pl011: Add transmit FIFO to PL011State

[Philippe Mathieu-Daudé]

On 16/11/23 16:48, Juan Quintela wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
>> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>>> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>>>        }
>>>    };
>>>    +static bool pl011_xmit_fifo_state_needed(void *opaque)
>>> +{
>>> +    PL011State* s = opaque;
>>> +
>>> +    return !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;
>>> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>>>        .subsections = (const VMStateDescription * []) {
>>>            &vmstate_pl011_clock,
>>>            NULL
>>> -    }
>>> +    },
>>> +    .subsections = (const VMStateDescription * []) {
>>> +        &vmstate_pl011_xmit_fifo,
>>> +        NULL
>>> +    },
>>>    };
>>
>> It just occurred to me that you may need a vmstate_pl011 pre_load() to
>> empty the FIFO, which will then be filled if and only if the saved
>> vmstate_pl011_xmit_fifo subsection is present.
>>
>> Juan, have I got this correct about how migration would or should handle a missing subsection?
> 
> I hav'nt looked about how the device is created. But if it is created
> with the fifo empty you don't need the pre_load().

This is indeed the case. Thank you Juan!