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

[PATCH 00/12] 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.

- First we add fifo8_peek_buf() to the fifo8 API (patches 1-3),
- Then we massage a bit the pl011 model to ease development
  (patches 4-11),
- Finally we add the TX FIFO implementation using a chardev
  front-end 'watch' callback.

The last patch is RFC mostly because I don't know what is the
best way to resolve adding a new migratable field without
breaking the migration stream.

Tested with qtest/avocado/TRS test suites:

 trs-workspace$ make test
 Startup finished in 6min 40.596s (kernel) + 38.786s (userspace) = 7min 19.382s.
 RESULTS - date.DateTest.test_date: PASSED (46.90s)
 RESULTS - df.DfTest.test_df: PASSED (2.29s)
 RESULTS - parselogs.ParseLogsTest.test_parselogs: PASSED (23.45s)
 RESULTS - ping.PingTest.test_ping: PASSED (0.05s)
 RESULTS - python.PythonTest.test_python3: PASSED (2.72s)
 RESULTS - rtc.RTCTest.test_rtc: PASSED (51.84s)
 RESULTS - ssh.SSHTest.test_ssh: PASSED (4.56s)
 RESULTS - systemd.SystemdBasicTests.test_systemd_basic: PASSED (2.33s)
 RESULTS - systemd.SystemdBasicTests.test_systemd_failed: PASSED (4.41s)
 RESULTS - systemd.SystemdBasicTests.test_systemd_list: PASSED (30.07s)
 RESULTS - systemd.SystemdJournalTests.test_systemd_boot_time: PASSED (2.42s)
 RESULTS - systemd.SystemdJournalTests.test_systemd_journal: PASSED (2.62s)
 RESULTS - systemd.SystemdServiceTests.test_systemd_disable_enable: PASSED (40.34s)
 RESULTS - systemd.SystemdServiceTests.test_systemd_status: PASSED (2.43s)
 RESULTS - systemd.SystemdServiceTests.test_systemd_stop_start: PASSED (9.23s)
 RESULTS - opteetest.OpteeTestSuite.test_opteetest_xtest: PASSED (329.89s)
 RESULTS - soafeetestsuite.SoafeeTestSuite.test_soafee: PASSED (272.07s)
 RESULTS - tpm.TpmTestSuite.test_tpm: PASSED (53.61s)
 RESULTS - ptest.PtestRunnerTest.test_ptestrunner_expectfail: SKIPPED (0.00s)
 RESULTS - ptest.PtestRunnerTest.test_ptestrunner_expectsuccess: SKIPPED (0.00s)
 RESULTS - systemd.SystemdServiceTests.test_systemd_disable_enable_ro: SKIPPED (0.00s)
 SUMMARY:
 trs-image () - Ran 21 tests in 883.291s
 trs-image - OK - All required tests passed (successes=18, skipped=3, failures=0, errors=0)

Philippe Mathieu-Daudé (12):
  util/fifo8: Fix typo in fifo8_push_all() description
  util/fifo8: Allow fifo8_pop_buf() to not populate popped length
  util/fifo8: Introduce fifo8_peek_buf()
  hw/char/pl011: Display register name in trace events
  hw/char/pl011: Remove duplicated PL011_INT_[RT]X definitions
  hw/char/pl011: Replace magic values by register field definitions
  hw/char/pl011: Split RX/TX path of pl011_reset_fifo()
  hw/char/pl011: Extract pl011_write_tx() from pl011_write()
  hw/char/pl011: Check if transmitter is enabled
  hw/char/pl011: Check if receiver is enabled
  hw/char/pl011: Rename RX FIFO methods
  hw/char/pl011: Implement TX FIFO

 include/hw/char/pl011.h |   2 +
 include/qemu/fifo8.h    |  38 ++++++--
 hw/char/pl011.c         | 209 +++++++++++++++++++++++++++++++++-------
 util/fifo8.c            |  28 +++++-
 hw/char/trace-events    |  12 ++-
 5 files changed, 239 insertions(+), 50 deletions(-)

-- 
2.38.1

[PATCH 01/12] util/fifo8: Fix typo in fifo8_push_all() description

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/fifo8.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index 28bf2cee57..16be02f361 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -46,7 +46,7 @@ void fifo8_push(Fifo8 *fifo, uint8_t data);
  * fifo8_push_all:
  * @fifo: FIFO to push to
  * @data: data to push
- * @size: number of bytes to push
+ * @num: number of bytes to push
  *
  * Push a byte array to the FIFO. Behaviour is undefined if the FIFO is full.
  * Clients are responsible for checking the space left in the FIFO using
-- 
2.38.1

Re: [PATCH 01/12] util/fifo8: Fix typo in fifo8_push_all() description

[Francisco Iglesias]

On [2023 May 22] Mon 17:31:33, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  include/qemu/fifo8.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
> index 28bf2cee57..16be02f361 100644
> --- a/include/qemu/fifo8.h
> +++ b/include/qemu/fifo8.h
> @@ -46,7 +46,7 @@ void fifo8_push(Fifo8 *fifo, uint8_t data);
>   * fifo8_push_all:
>   * @fifo: FIFO to push to
>   * @data: data to push
> - * @size: number of bytes to push
> + * @num: number of bytes to push
>   *
>   * Push a byte array to the FIFO. Behaviour is undefined if the FIFO is full.
>   * Clients are responsible for checking the space left in the FIFO using
> -- 
> 2.38.1
> 
> 

Re: [PATCH 01/12] util/fifo8: Fix typo in fifo8_push_all() description

[Alex Bennée]

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

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

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 02/12] 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>
---
 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.38.1

Re: [PATCH 02/12] util/fifo8: Allow fifo8_pop_buf() to not populate popped length

[Francisco Iglesias]

On [2023 May 22] Mon 17:31:34, 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>


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

Re: [PATCH 02/12] util/fifo8: Allow fifo8_pop_buf() to not populate popped length

[Alex Bennée]

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

> 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: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 03/12] util/fifo8: Introduce fifo8_peek_buf()

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/fifo8.h | 26 ++++++++++++++++++++++++++
 util/fifo8.c         | 22 ++++++++++++++++++----
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
index d0d02bc73d..7acf6d1347 100644
--- a/include/qemu/fifo8.h
+++ b/include/qemu/fifo8.h
@@ -93,6 +93,32 @@ uint8_t fifo8_pop(Fifo8 *fifo);
  */
 const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
 
+/**
+ * fifo8_peek_buf:
+ * @fifo: FIFO to poke from
+ * @max: maximum number of bytes to pop
+ * @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
+ * the FIFO backing store and data is invalidated once any of the fifo8_* APIs
+ * are called on the FIFO.
+ *
+ * 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.38.1

Re: [PATCH 03/12] util/fifo8: Introduce fifo8_peek_buf()

[Francisco Iglesias]

On [2023 May 22] Mon 17:31:35, Philippe Mathieu-Daudé wrote:
> To be able to poke at FIFO content without popping it,
> introduce the fifo8_peek_buf() method by factoring
> common content from fifo8_pop_buf().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qemu/fifo8.h | 26 ++++++++++++++++++++++++++
>  util/fifo8.c         | 22 ++++++++++++++++++----
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
> index d0d02bc73d..7acf6d1347 100644
> --- a/include/qemu/fifo8.h
> +++ b/include/qemu/fifo8.h
> @@ -93,6 +93,32 @@ uint8_t fifo8_pop(Fifo8 *fifo);
>   */
>  const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
>  
> +/**
> + * fifo8_peek_buf:
> + * @fifo: FIFO to poke from
> + * @max: maximum number of bytes to pop
> + * @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

s/Pop/Peek into/

> + * containing the popped data is returned. This buffer points directly into

s/popped data/data peeked into/

If above sounds good:

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>


> + * the FIFO backing store and data is invalidated once any of the fifo8_* APIs
> + * are called on the FIFO.

(Above sounds as if it happens automatically to me but I'm not english native,
a suggestion could be to put something as below "clients are responsible for
tracking this")

> + *
> + * 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.38.1
> 
> 

Re: [PATCH 03/12] util/fifo8: Introduce fifo8_peek_buf()

[Alex Bennée]

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

> To be able to poke at FIFO content without popping it,

peek/poke are different operations in my head. You peek to read stuff
and poke to change stuff, or at least thats what I did in BASIC back in
the 80s.

Either way I don't think peek and poke are synonyms.

> introduce the fifo8_peek_buf() method by factoring
> common content from fifo8_pop_buf().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qemu/fifo8.h | 26 ++++++++++++++++++++++++++
>  util/fifo8.c         | 22 ++++++++++++++++++----
>  2 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/fifo8.h b/include/qemu/fifo8.h
> index d0d02bc73d..7acf6d1347 100644
> --- a/include/qemu/fifo8.h
> +++ b/include/qemu/fifo8.h
> @@ -93,6 +93,32 @@ uint8_t fifo8_pop(Fifo8 *fifo);
>   */
>  const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *numptr);
>  
> +/**
> + * fifo8_peek_buf:

fifo8_peek_buf() - read upto max bytes from the fifo


> + * @fifo: FIFO to poke from
> + * @max: maximum number of bytes to pop
> + * @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
> + * the FIFO backing store and data is invalidated once any of the fifo8_* APIs
> + * are called on the FIFO.
> + *
> + * 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;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 04/12] hw/char/pl011: Display register name in trace events

[Philippe Mathieu-Daudé]

To avoid knowing the register addresses by heart,
display their name along in the trace events.

Since the MMIO region is 4K wide (0x1000 bytes),
displaying the address with 3 digits is enough,
so reduce the address format.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 77bbc2a982..274e48003f 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -51,6 +51,7 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define PL011_INT_TX 0x20
 #define PL011_INT_RX 0x10
 
+/* Flag Register, UARTFR */
 #define PL011_FLAG_TXFE 0x80
 #define PL011_FLAG_RXFF 0x40
 #define PL011_FLAG_TXFF 0x20
@@ -76,6 +77,24 @@ static const unsigned char pl011_id_arm[8] =
 static const unsigned char pl011_id_luminary[8] =
   { 0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1 };
 
+static const char *pl011_regname(hwaddr offset)
+{
+    static const char *const rname[] = {
+        [0] = "DR", [1] = "RSR", [6] = "FR", [8] = "ILPR", [9] = "IBRD",
+        [10] = "FBRD", [11] = "LCRH", [12] = "CR", [13] = "IFLS", [14] = "IMSC",
+        [15] = "RIS", [16] = "MIS", [17] = "ICR", [18] = "DMACR",
+    };
+    unsigned idx = offset >> 2;
+
+    if (idx < ARRAY_SIZE(rname) && rname[idx]) {
+        return rname[idx];
+    }
+    if (idx >= 0x3f8 && idx <= 0x400) {
+        return "ID";
+    }
+    return "UNKN";
+}
+
 /* Which bits in the interrupt status matter for each outbound IRQ line ? */
 static const uint32_t irqmask[] = {
     INT_E | INT_MS | INT_RT | INT_TX | INT_RX, /* combined IRQ */
@@ -191,7 +210,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
         break;
     }
 
-    trace_pl011_read(offset, r);
+    trace_pl011_read(offset, r, pl011_regname(offset));
     return r;
 }
 
@@ -234,7 +253,7 @@ static void pl011_write(void *opaque, hwaddr offset,
     PL011State *s = (PL011State *)opaque;
     unsigned char ch;
 
-    trace_pl011_write(offset, value);
+    trace_pl011_write(offset, value, pl011_regname(offset));
 
     switch (offset >> 2) {
     case 0: /* UARTDR */
@@ -252,7 +271,7 @@ static void pl011_write(void *opaque, hwaddr offset,
     case 6: /* UARTFR */
         /* Writes to Flag register are ignored.  */
         break;
-    case 8: /* UARTUARTILPR */
+    case 8: /* UARTILPR */
         s->ilpr = value;
         break;
     case 9: /* UARTIBRD */
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 2ecb36232e..babf4d35ea 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -54,9 +54,9 @@ escc_sunmouse_event(int dx, int dy, int buttons_state) "dx=%d dy=%d buttons=0x%0
 
 # pl011.c
 pl011_irq_state(int level) "irq state %d"
-pl011_read(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
+pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
 pl011_read_fifo(int read_count) "FIFO read, read_count now %d"
-pl011_write(uint32_t addr, uint32_t value) "addr 0x%08x value 0x%08x"
+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"
-- 
2.38.1

Re: [PATCH 04/12] hw/char/pl011: Display register name in trace events

[Alex Bennée]

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

> To avoid knowing the register addresses by heart,
> display their name along in the trace events.
>
> Since the MMIO region is 4K wide (0x1000 bytes),
> displaying the address with 3 digits is enough,
> so reduce the address format.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 05/12] hw/char/pl011: Remove duplicated PL011_INT_[RT]X definitions

[Philippe Mathieu-Daudé]

PL011_INT_TX duplicates INT_TX, and PL011_INT_RX INT_RX.
Follow other register fields definitions from this file,
keep the shorter form.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 274e48003f..93e19b2c40 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -48,9 +48,6 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
     return dev;
 }
 
-#define PL011_INT_TX 0x20
-#define PL011_INT_RX 0x10
-
 /* Flag Register, UARTFR */
 #define PL011_FLAG_TXFE 0x80
 #define PL011_FLAG_RXFF 0x40
@@ -157,7 +154,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
             s->flags |= PL011_FLAG_RXFE;
         }
         if (s->read_count == s->read_trigger - 1)
-            s->int_level &= ~ PL011_INT_RX;
+            s->int_level &= ~ INT_RX;
         trace_pl011_read_fifo(s->read_count);
         s->rsr = c >> 8;
         pl011_update(s);
@@ -262,7 +259,7 @@ static void pl011_write(void *opaque, hwaddr offset,
         /* 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 |= PL011_INT_TX;
+        s->int_level |= INT_TX;
         pl011_update(s);
         break;
     case 1: /* UARTRSR/UARTECR */
@@ -350,7 +347,7 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
         s->flags |= PL011_FLAG_RXFF;
     }
     if (s->read_count == s->read_trigger) {
-        s->int_level |= PL011_INT_RX;
+        s->int_level |= INT_RX;
         pl011_update(s);
     }
 }
-- 
2.38.1

Re: [PATCH 05/12] hw/char/pl011: Remove duplicated PL011_INT_[RT]X definitions

[Alex Bennée]

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

> PL011_INT_TX duplicates INT_TX, and PL011_INT_RX INT_RX.
> Follow other register fields definitions from this file,
> keep the shorter form.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 06/12] hw/char/pl011: Replace magic values by register field definitions

[Philippe Mathieu-Daudé]

0x400 is Data Register Break Error (DR_BE),
0x10 is Line Control Register Fifo Enabled (LCR_FEN)
and 0x1 is Send Break (LCR_BRK).

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 93e19b2c40..98c5268388 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -54,6 +54,9 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define PL011_FLAG_TXFF 0x20
 #define PL011_FLAG_RXFE 0x10
 
+/* Data Register, UARTDR */
+#define DR_BE   (1 << 10)
+
 /* Interrupt status bits in UARTRIS, UARTMIS, UARTIMSC */
 #define INT_OE (1 << 10)
 #define INT_BE (1 << 9)
@@ -69,6 +72,10 @@ DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr)
 #define INT_E (INT_OE | INT_BE | INT_PE | INT_FE)
 #define INT_MS (INT_RI | INT_DSR | INT_DCD | INT_CTS)
 
+/* Line Control Register, UARTLCR_H */
+#define LCR_FEN     (1 << 4)
+#define LCR_BRK     (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] =
@@ -116,7 +123,7 @@ static void pl011_update(PL011State *s)
 
 static bool pl011_is_fifo_enabled(PL011State *s)
 {
-    return (s->lcr & 0x10) != 0;
+    return (s->lcr & LCR_FEN) != 0;
 }
 
 static inline unsigned pl011_get_fifo_depth(PL011State *s)
@@ -218,7 +225,7 @@ static void pl011_set_read_trigger(PL011State *s)
        the threshold.  However linux only reads the FIFO in response to an
        interrupt.  Triggering the interrupt when the FIFO is non-empty seems
        to make things work.  */
-    if (s->lcr & 0x10)
+    if (s->lcr & LCR_FEN)
         s->read_trigger = (s->ifl >> 1) & 0x1c;
     else
 #endif
@@ -281,11 +288,11 @@ static void pl011_write(void *opaque, hwaddr offset,
         break;
     case 11: /* UARTLCR_H */
         /* Reset the FIFO state on FIFO enable or disable */
-        if ((s->lcr ^ value) & 0x10) {
+        if ((s->lcr ^ value) & LCR_FEN) {
             pl011_reset_fifo(s);
         }
-        if ((s->lcr ^ value) & 0x1) {
-            int break_enable = value & 0x1;
+        if ((s->lcr ^ value) & LCR_BRK) {
+            int break_enable = value & LCR_BRK;
             qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
                               &break_enable);
         }
@@ -359,8 +366,9 @@ static void pl011_receive(void *opaque, const uint8_t *buf, int size)
 
 static void pl011_event(void *opaque, QEMUChrEvent event)
 {
-    if (event == CHR_EVENT_BREAK)
-        pl011_put_fifo(opaque, 0x400);
+    if (event == CHR_EVENT_BREAK) {
+        pl011_put_fifo(opaque, DR_BE);
+    }
 }
 
 static void pl011_clock_update(void *opaque, ClockEvent event)
-- 
2.38.1

Re: [PATCH 06/12] hw/char/pl011: Replace magic values by register field definitions

[Alex Bennée]

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

> 0x400 is Data Register Break Error (DR_BE),
> 0x10 is Line Control Register Fifo Enabled (LCR_FEN)
> and 0x1 is Send Break (LCR_BRK).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 07/12] 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>
---
 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 98c5268388..f0b305e5d7 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;
@@ -504,7 +512,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.38.1

Re: [PATCH 07/12] hw/char/pl011: Split RX/TX path of pl011_reset_fifo()

[Alex Bennée]

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

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 08/12] hw/char/pl011: Extract pl011_write_tx() 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_tx() function.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f0b305e5d7..c55ef41fbf 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_tx(PL011State *s, const uint8_t *buf, int length)
+{
+    /* ??? 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, buf, 1);
+    s->int_level |= INT_TX;
+    pl011_update(s);
+}
+
 static uint64_t pl011_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
@@ -262,19 +273,12 @@ 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);
+        pl011_write_tx(s, (uint8_t *) &value, 1);
         break;
     case 1: /* UARTRSR/UARTECR */
         s->rsr = 0;
-- 
2.38.1

Re: [PATCH 08/12] hw/char/pl011: Extract pl011_write_tx() from pl011_write()

[Alex Bennée]

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

> When implementing FIFO, this code will become more complex.
> Start by factoring it out to a new pl011_write_tx() function.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Philippe Mathieu-Daudé]

Do not transmit characters when UART or transmitter are disabled.

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c55ef41fbf..30bedeac15 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,9 @@ static inline void pl011_reset_tx_fifo(PL011State *s)
 
 static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length)
 {
-    /* ??? Check if transmitter is enabled.  */
+    if (!(s->cr & (CR_UARTEN | CR_TXE))) {
+        return;
+    }
 
     /* XXX this blocks entire thread. Rewrite to use
      * qemu_chr_fe_write and background I/O callbacks */
-- 
2.38.1

Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Alex Bennée]

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

> Do not transmit characters when UART or transmitter are disabled.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Peter Maydell]

On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Do not transmit characters when UART or transmitter are disabled.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Last time somebody tried to add checks on the tx/rx enable bits
for the PL011 it broke 'make check' because the hand-rolled
UART code in boot-serial-test and migration-test doesn't
set up the UART control register strictly correctly:

https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/

Given that imposing these checks doesn't help anything
much and might break naive bare-metal tested-only-on-QEMU
code, is it worthwhile ?

>  static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length)
>  {
> -    /* ??? Check if transmitter is enabled.  */
> +    if (!(s->cr & (CR_UARTEN | CR_TXE))) {

This will allow TX if either UARTEN or TXE is set, which
probably isn't what you meant.

> +        return;
> +    }
>
>      /* XXX this blocks entire thread. Rewrite to use
>       * qemu_chr_fe_write and background I/O callbacks */

thanks
-- PMM

Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Alex Bennée]

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

> On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Do not transmit characters when UART or transmitter are disabled.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Last time somebody tried to add checks on the tx/rx enable bits
> for the PL011 it broke 'make check' because the hand-rolled
> UART code in boot-serial-test and migration-test doesn't
> set up the UART control register strictly correctly:
>
> https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/
>
> Given that imposing these checks doesn't help anything
> much and might break naive bare-metal tested-only-on-QEMU
> code, is it worthwhile ?

Surely we aim to be a correct model so the fix should be in our naive
and incorrect code?

>
>>  static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length)
>>  {
>> -    /* ??? Check if transmitter is enabled.  */
>> +    if (!(s->cr & (CR_UARTEN | CR_TXE))) {
>
> This will allow TX if either UARTEN or TXE is set, which
> probably isn't what you meant.
>
>> +        return;
>> +    }
>>
>>      /* XXX this blocks entire thread. Rewrite to use
>>       * qemu_chr_fe_write and background I/O callbacks */
>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Peter Maydell]

On Thu, 25 May 2023 at 13:52, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Do not transmit characters when UART or transmitter are disabled.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> > Last time somebody tried to add checks on the tx/rx enable bits
> > for the PL011 it broke 'make check' because the hand-rolled
> > UART code in boot-serial-test and migration-test doesn't
> > set up the UART control register strictly correctly:
> >
> > https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/
> >
> > Given that imposing these checks doesn't help anything
> > much and might break naive bare-metal tested-only-on-QEMU
> > code, is it worthwhile ?
>
> Surely we aim to be a correct model so the fix should be in our naive
> and incorrect code?

In our own test suites, sure -- we should probably fix that
even if we don't change the PL011 model to require it.
But if we let this kind of thing get past us in our own testsuite,
it suggests there's probably a lot of similar naive code out
there in the world -- these Arm boards with PL011s are pretty
commonly used for "my first bare metal assembly program" stuff
and there's a lot of cargo-culting of how to do things like
serial output, and programs that were never tested on any
real hardware...

thanks
-- PMM

Re: [PATCH 09/12] hw/char/pl011: Check if transmitter is enabled

[Philippe Mathieu-Daudé]

On 25/5/23 14:55, Peter Maydell wrote:
> On Thu, 25 May 2023 at 13:52, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Mon, 22 May 2023 at 16:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> Do not transmit characters when UART or transmitter are disabled.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> Last time somebody tried to add checks on the tx/rx enable bits
>>> for the PL011 it broke 'make check' because the hand-rolled
>>> UART code in boot-serial-test and migration-test doesn't
>>> set up the UART control register strictly correctly:
>>>
>>> https://lore.kernel.org/qemu-devel/CAFEAcA8ZDmjP7G0eVpxcB1jiSGarZAbqPV0xr5WquR213mBUBg@mail.gmail.com/

Hmm I ran 'make check-qtest' my build directory only targets aarch64,
I probably missed the arm (32-bit) tests :/

>>> Given that imposing these checks doesn't help anything
>>> much and might break naive bare-metal tested-only-on-QEMU
>>> code, is it worthwhile ?
>>
>> Surely we aim to be a correct model so the fix should be in our naive
>> and incorrect code?
> 
> In our own test suites, sure -- we should probably fix that
> even if we don't change the PL011 model to require it.
> But if we let this kind of thing get past us in our own testsuite,
> it suggests there's probably a lot of similar naive code out
> there in the world -- these Arm boards with PL011s are pretty
> commonly used for "my first bare metal assembly program" stuff
> and there's a lot of cargo-culting of how to do things like
> serial output, and programs that were never tested on any
> real hardware...

OK. I'll add a comment, keep the current behavior when TX is
disabled, but add a GUEST_ERROR log message.

[PATCH 10/12] 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>
---
 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 30bedeac15..1ec102d8de 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)
 
@@ -348,9 +349,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 | CR_RXE)) {
+        r = s->read_count < pl011_get_fifo_depth(s);
+    }
     trace_pl011_can_receive(s->lcr, s->read_count, r);
     return r;
 }
-- 
2.38.1

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

[Alex Bennée]

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

> Do not receive characters when UART or receiver are disabled.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 11/12] 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>
---
 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 1ec102d8de..03c006199e 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -358,7 +358,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;
@@ -369,9 +369,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) {
@@ -382,13 +382,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 babf4d35ea..9fd40e3aae 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.38.1

Re: [PATCH 11/12] hw/char/pl011: Rename RX FIFO methods

[Alex Bennée]

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

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 12/12] 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).

Due to the addition of the TX FIFO in the instance state, increase
the migration stream version.

Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org>
Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC because I'm pretty sure I got the migration code wrong.

After writing this I noticed the hw/char/cmsdk-apb-uart.c model
is much more complete. Instead of copy/pasting its code, I'd rather
try to extract some generic/bstract "FIFO based chardev" QOM class;
but this is beyond the scope of this series.
---
 include/hw/char/pl011.h |   2 +
 hw/char/pl011.c         | 105 +++++++++++++++++++++++++++++++++++++---
 hw/char/trace-events    |   4 ++
 3 files changed, 105 insertions(+), 6 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 03c006199e..a957138405 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)
@@ -152,19 +155,94 @@ static inline 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 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 FALSE;
+}
+
+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;
+
+    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) {
+        return FALSE;
+    }
+    if  (!(s->cr & CR_UARTEN)) {
+        /* Allow completing the current FIFO character before stopping. */
+        count = 1;
+    }
+
+    /* 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 ((s->cr & CR_UARTEN) && !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) {
+            return pl011_drain_tx(s);
+        }
+    }
+
+    pl011_update(s);
+
+    return FALSE;
 }
 
 static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length)
 {
     if (!(s->cr & (CR_UARTEN | CR_TXE))) {
+        if (!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);
+        }
         return;
     }
 
-    /* XXX this blocks entire thread. Rewrite to use
-     * qemu_chr_fe_write and background I/O callbacks */
-    qemu_chr_fe_write_all(&s->chr, buf, 1);
-    s->int_level |= INT_TX;
-    pl011_update(s);
+    if (length > fifo8_num_free(&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(length);
+    fifo8_push_all(&s->xmit_fifo, buf, length);
+
+    pl011_xmit(NULL, G_IO_OUT, s);
 }
 
 static uint64_t pl011_read(void *opaque, hwaddr offset,
@@ -444,12 +522,17 @@ static int pl011_post_load(void *opaque, int version_id)
         s->read_pos = 0;
     }
 
+    if (version_id >= 3 && !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;
 }
 
 static const VMStateDescription vmstate_pl011 = {
     .name = "pl011",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
     .post_load = pl011_post_load,
     .fields = (VMStateField[]) {
@@ -462,6 +545,7 @@ static const VMStateDescription vmstate_pl011 = {
         VMSTATE_UINT32(int_enabled, PL011State),
         VMSTATE_UINT32(int_level, PL011State),
         VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
+        VMSTATE_FIFO8(xmit_fifo, PL011State),
         VMSTATE_UINT32(ilpr, PL011State),
         VMSTATE_UINT32(ibrd, PL011State),
         VMSTATE_UINT32(fbrd, PL011State),
@@ -505,10 +589,18 @@ static void pl011_realize(DeviceState *dev, Error **errp)
 {
     PL011State *s = PL011(dev);
 
+    fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH);
     qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
                              pl011_event, NULL, s, NULL, true);
 }
 
+static void pl011_unrealize(DeviceState *dev)
+{
+    PL011State *s = PL011(dev);
+
+    fifo8_destroy(&s->xmit_fifo);
+}
+
 static void pl011_reset(DeviceState *dev)
 {
     PL011State *s = PL011(dev);
@@ -534,6 +626,7 @@ static void pl011_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = pl011_realize;
+    dc->unrealize = pl011_unrealize;
     dc->reset = pl011_reset;
     dc->vmsd = &vmstate_pl011;
     device_class_set_props(dc, pl011_properties);
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 9fd40e3aae..4c25564066 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(int count) "TX FIFO push %d"
+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.38.1

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

[Alex Bennée]

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

> 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).
>
> Due to the addition of the TX FIFO in the instance state, increase
> the migration stream version.
>
> Reported-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC because I'm pretty sure I got the migration code wrong.
>
> After writing this I noticed the hw/char/cmsdk-apb-uart.c model
> is much more complete. Instead of copy/pasting its code, I'd rather
> try to extract some generic/bstract "FIFO based chardev" QOM class;
> but this is beyond the scope of this series.
> ---
<snip>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 03c006199e..a957138405 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)
> @@ -152,19 +155,94 @@ static inline 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 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 FALSE;

WHY ARE YOU SHOUTING?

> +}
> +

worth a comment the return signals something to the chardev - I guess
FEWatchFunc could do with a comment?

> +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;
> +
> +    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) {
> +        return FALSE;
> +    }
> +    if  (!(s->cr & CR_UARTEN)) {
> +        /* Allow completing the current FIFO character before stopping. */
> +        count = 1;
> +    }

maybe:

  bool tx_enabled = s->cr & CR_UARTEN;
  ...
  count = tx_enabled ? fifo8_num_used(&s->xmit_fifo) : 1; /* current only */

  if (count) {

> +
> +    /* 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 ((s->cr & CR_UARTEN) && !fifo8_is_empty(&s->xmit_fifo)) {

 tx_enabled && ...

> +        /* 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) {
> +            return pl011_drain_tx(s);
> +        }
> +    }
> +
> +    pl011_update(s);

}

> +
> +    return FALSE;
>  }
>  
>  static void pl011_write_tx(PL011State *s, const uint8_t *buf, int length)
>  {
>      if (!(s->cr & (CR_UARTEN | CR_TXE))) {
> +        if (!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);
> +        }
>          return;
>      }
>  
> -    /* XXX this blocks entire thread. Rewrite to use
> -     * qemu_chr_fe_write and background I/O callbacks */
> -    qemu_chr_fe_write_all(&s->chr, buf, 1);
> -    s->int_level |= INT_TX;
> -    pl011_update(s);
> +    if (length > fifo8_num_free(&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(length);
> +    fifo8_push_all(&s->xmit_fifo, buf, length);
> +
> +    pl011_xmit(NULL, G_IO_OUT, s);
>  }
>  
>  static uint64_t pl011_read(void *opaque, hwaddr offset,
> @@ -444,12 +522,17 @@ static int pl011_post_load(void *opaque, int version_id)
>          s->read_pos = 0;
>      }
>  
> +    if (version_id >= 3 && !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;
>  }
>  
>  static const VMStateDescription vmstate_pl011 = {
>      .name = "pl011",
> -    .version_id = 2,
> +    .version_id = 3,
>      .minimum_version_id = 2,
>      .post_load = pl011_post_load,
>      .fields = (VMStateField[]) {
> @@ -462,6 +545,7 @@ static const VMStateDescription vmstate_pl011 = {
>          VMSTATE_UINT32(int_enabled, PL011State),
>          VMSTATE_UINT32(int_level, PL011State),
>          VMSTATE_UINT32_ARRAY(read_fifo, PL011State, PL011_FIFO_DEPTH),
> +        VMSTATE_FIFO8(xmit_fifo, PL011State),

I think you want something like:

   VMSTATE_FIFO8_TEST(xmit_fifo, PL011State, pl011_is_version_3_or_better),

>          VMSTATE_UINT32(ilpr, PL011State),
>          VMSTATE_UINT32(ibrd, PL011State),
>          VMSTATE_UINT32(fbrd, PL011State),
> @@ -505,10 +589,18 @@ static void pl011_realize(DeviceState *dev, Error **errp)
>  {
>      PL011State *s = PL011(dev);
>  
> +    fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH);
>      qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
>                               pl011_event, NULL, s, NULL, true);
>  }
>  
> +static void pl011_unrealize(DeviceState *dev)
> +{
> +    PL011State *s = PL011(dev);
> +
> +    fifo8_destroy(&s->xmit_fifo);
> +}
> +
>  static void pl011_reset(DeviceState *dev)
>  {
>      PL011State *s = PL011(dev);
> @@ -534,6 +626,7 @@ static void pl011_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      dc->realize = pl011_realize;
> +    dc->unrealize = pl011_unrealize;
>      dc->reset = pl011_reset;
>      dc->vmsd = &vmstate_pl011;
>      device_class_set_props(dc, pl011_properties);
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index 9fd40e3aae..4c25564066 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(int count) "TX FIFO push %d"
> +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


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro