[PATCH 0/4] Fix SiFive UART character drop issue and minor refactors

[PATCH 0/4] Fix SiFive UART character drop issue and minor refactors

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

This patch set fixes the SiFive UART character drop issue introduced
after commit [1], which changed character printing from synchronous to
asynchronous.

Since UART now transmits characters asynchronously, it is possible for
the Tx FIFO to become full, causing new characters to be ignored and
dropped when running Linux. This happens because:

  1. The Linux SiFive UART driver sets the transmit watermark level to 1
     [2], meaning a transmit watermark interrupt is raised whenever a
     character is enqueued into the Tx FIFO.
  2. Upon receiving a transmit watermark interrupt, the Linux driver
     transfers up to a full Tx FIFO's worth of characters from the Linux
     serial transmit buffer [3], without checking the txdata.full flag
     before transferring multiple characters [4].

This patch set updates QEMU to honor the Tx/Rx watermark thresholds and
raise interrupts only when the Tx threshold is exceeded or the Rx
threshold is undercut.

The remaining patches contain minor refactors, including removing an
outdated comment about the Tx FIFO.

[1] 53c1557b230986ab6320a58e1b2c26216ecd86d5
[2] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L1039
[3] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L538
[4] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L291

Frank Chang (4):
  hw/char: sifive_uart: Raise IRQ according to the Tx/Rx watermark
    thresholds
  hw/char: sifive_uart: Avoid pushing Tx FIFO when size is zero
  hw/char: sifive_uart: Remove outdated comment about Tx FIFO
  hw/char: sifive_uart: Add newline to error message

 hw/char/sifive_uart.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

--
2.49.0

[PATCH 1/4] hw/char: sifive_uart: Raise IRQ according to the Tx/Rx watermark thresholds

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Currently, the SiFive UART raises an IRQ whenever:

  1. ie.txwm is enabled.
  2. ie.rxwm is enabled and the Rx FIFO is not empty.

It does not check the watermark thresholds set by software. However,
since commit [1] changed the SiFive UART character printing from
synchronous to asynchronous, Tx overflows may occur, causing characters
to be dropped when running Linux because:

  1. The Linux SiFive UART driver sets the transmit watermark level to 1
     [2], meaning a transmit watermark interrupt is raised whenever a
     character is enqueued into the Tx FIFO.
  2. Upon receiving a transmit watermark interrupt, the Linux driver
     transfers up to a full Tx FIFO's worth of characters from the Linux
     serial transmit buffer [3], without checking the txdata.full flag
     before transferring multiple characters [4].

To fix this issue, we must honor the Tx/Rx watermark thresholds and
raise interrupts only when the Tx threshold is exceeded or the Rx
threshold is undercut.

[1] 53c1557b230986ab6320a58e1b2c26216ecd86d5
[2] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L1039
[3] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L538
[4] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L291

Signed-off-by: Frank Chang <frank.chang@sifive.com>
Signed-off-by: Emmanuel Blot <emmanuel.blot@sifive.com>
---
 hw/char/sifive_uart.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 9bc697a67b5..138c31fcabf 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -35,16 +35,17 @@
  */
 
 /* Returns the state of the IP (interrupt pending) register */
-static uint64_t sifive_uart_ip(SiFiveUARTState *s)
+static uint32_t sifive_uart_ip(SiFiveUARTState *s)
 {
-    uint64_t ret = 0;
+    uint32_t ret = 0;
 
-    uint64_t txcnt = SIFIVE_UART_GET_TXCNT(s->txctrl);
-    uint64_t rxcnt = SIFIVE_UART_GET_RXCNT(s->rxctrl);
+    uint32_t txcnt = SIFIVE_UART_GET_TXCNT(s->txctrl);
+    uint32_t rxcnt = SIFIVE_UART_GET_RXCNT(s->rxctrl);
 
-    if (txcnt != 0) {
+    if (fifo8_num_used(&s->tx_fifo) < txcnt) {
         ret |= SIFIVE_UART_IP_TXWM;
     }
+
     if (s->rx_fifo_len > rxcnt) {
         ret |= SIFIVE_UART_IP_RXWM;
     }
@@ -55,15 +56,14 @@ static uint64_t sifive_uart_ip(SiFiveUARTState *s)
 static void sifive_uart_update_irq(SiFiveUARTState *s)
 {
     int cond = 0;
-    if ((s->ie & SIFIVE_UART_IE_TXWM) ||
-        ((s->ie & SIFIVE_UART_IE_RXWM) && s->rx_fifo_len)) {
+    uint32_t ip = sifive_uart_ip(s);
+
+    if (((ip & SIFIVE_UART_IP_TXWM) && (s->ie & SIFIVE_UART_IE_TXWM)) ||
+        ((ip & SIFIVE_UART_IP_RXWM) && (s->ie & SIFIVE_UART_IE_RXWM))) {
         cond = 1;
     }
-    if (cond) {
-        qemu_irq_raise(s->irq);
-    } else {
-        qemu_irq_lower(s->irq);
-    }
+
+    qemu_set_irq(s->irq, cond);
 }
 
 static gboolean sifive_uart_xmit(void *do_not_use, GIOCondition cond,
-- 
2.49.0

[PATCH 2/4] hw/char: sifive_uart: Avoid pushing Tx FIFO when size is zero

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

There's no need to call fifo8_push_all() when size is zero.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 hw/char/sifive_uart.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 138c31fcabf..401f869680d 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -122,7 +122,9 @@ static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf,
         qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow");
     }
 
-    fifo8_push_all(&s->tx_fifo, buf, size);
+    if (size > 0) {
+        fifo8_push_all(&s->tx_fifo, buf, size);
+    }
 
     if (fifo8_is_full(&s->tx_fifo)) {
         s->txfifo |= SIFIVE_UART_TXFIFO_FULL;
-- 
2.49.0

[PATCH 3/4] hw/char: sifive_uart: Remove outdated comment about Tx FIFO

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Since Tx FIFO is now implemented using "qemu/fifo8.h", remove the comment
that no longer reflects the current implementation.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 hw/char/sifive_uart.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 401f869680d..baef0bd9c28 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -28,12 +28,6 @@
 
 #define TX_INTERRUPT_TRIGGER_DELAY_NS 100
 
-/*
- * Not yet implemented:
- *
- * Transmit FIFO using "qemu/fifo8.h"
- */
-
 /* Returns the state of the IP (interrupt pending) register */
 static uint32_t sifive_uart_ip(SiFiveUARTState *s)
 {
-- 
2.49.0

[PATCH 4/4] hw/char: sifive_uart: Add newline to error message

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Adds a missing newline character to the error message.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 hw/char/sifive_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index baef0bd9c28..e7357d585a1 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -113,7 +113,7 @@ static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf,
 
     if (size > fifo8_num_free(&s->tx_fifo)) {
         size = fifo8_num_free(&s->tx_fifo);
-        qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow");
+        qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow.\n");
     }
 
     if (size > 0) {
-- 
2.49.0

Re: [PATCH 1/4] hw/char: sifive_uart: Raise IRQ according to the Tx/Rx watermark thresholds

[Alistair Francis]

On Fri, Sep 12, 2025 at 2:08 AM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Currently, the SiFive UART raises an IRQ whenever:
>
>   1. ie.txwm is enabled.
>   2. ie.rxwm is enabled and the Rx FIFO is not empty.
>
> It does not check the watermark thresholds set by software. However,
> since commit [1] changed the SiFive UART character printing from
> synchronous to asynchronous, Tx overflows may occur, causing characters
> to be dropped when running Linux because:
>
>   1. The Linux SiFive UART driver sets the transmit watermark level to 1
>      [2], meaning a transmit watermark interrupt is raised whenever a
>      character is enqueued into the Tx FIFO.
>   2. Upon receiving a transmit watermark interrupt, the Linux driver
>      transfers up to a full Tx FIFO's worth of characters from the Linux
>      serial transmit buffer [3], without checking the txdata.full flag
>      before transferring multiple characters [4].
>
> To fix this issue, we must honor the Tx/Rx watermark thresholds and
> raise interrupts only when the Tx threshold is exceeded or the Rx
> threshold is undercut.
>
> [1] 53c1557b230986ab6320a58e1b2c26216ecd86d5
> [2] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L1039
> [3] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L538
> [4] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L291
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> Signed-off-by: Emmanuel Blot <emmanuel.blot@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/sifive_uart.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index 9bc697a67b5..138c31fcabf 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -35,16 +35,17 @@
>   */
>
>  /* Returns the state of the IP (interrupt pending) register */
> -static uint64_t sifive_uart_ip(SiFiveUARTState *s)
> +static uint32_t sifive_uart_ip(SiFiveUARTState *s)
>  {
> -    uint64_t ret = 0;
> +    uint32_t ret = 0;
>
> -    uint64_t txcnt = SIFIVE_UART_GET_TXCNT(s->txctrl);
> -    uint64_t rxcnt = SIFIVE_UART_GET_RXCNT(s->rxctrl);
> +    uint32_t txcnt = SIFIVE_UART_GET_TXCNT(s->txctrl);
> +    uint32_t rxcnt = SIFIVE_UART_GET_RXCNT(s->rxctrl);
>
> -    if (txcnt != 0) {
> +    if (fifo8_num_used(&s->tx_fifo) < txcnt) {
>          ret |= SIFIVE_UART_IP_TXWM;
>      }
> +
>      if (s->rx_fifo_len > rxcnt) {
>          ret |= SIFIVE_UART_IP_RXWM;
>      }
> @@ -55,15 +56,14 @@ static uint64_t sifive_uart_ip(SiFiveUARTState *s)
>  static void sifive_uart_update_irq(SiFiveUARTState *s)
>  {
>      int cond = 0;
> -    if ((s->ie & SIFIVE_UART_IE_TXWM) ||
> -        ((s->ie & SIFIVE_UART_IE_RXWM) && s->rx_fifo_len)) {
> +    uint32_t ip = sifive_uart_ip(s);
> +
> +    if (((ip & SIFIVE_UART_IP_TXWM) && (s->ie & SIFIVE_UART_IE_TXWM)) ||
> +        ((ip & SIFIVE_UART_IP_RXWM) && (s->ie & SIFIVE_UART_IE_RXWM))) {
>          cond = 1;
>      }
> -    if (cond) {
> -        qemu_irq_raise(s->irq);
> -    } else {
> -        qemu_irq_lower(s->irq);
> -    }
> +
> +    qemu_set_irq(s->irq, cond);
>  }
>
>  static gboolean sifive_uart_xmit(void *do_not_use, GIOCondition cond,
> --
> 2.49.0
>
>

Re: [PATCH 2/4] hw/char: sifive_uart: Avoid pushing Tx FIFO when size is zero

[Alistair Francis]

On Fri, Sep 12, 2025 at 2:08 AM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> There's no need to call fifo8_push_all() when size is zero.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/sifive_uart.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index 138c31fcabf..401f869680d 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -122,7 +122,9 @@ static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf,
>          qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow");
>      }
>
> -    fifo8_push_all(&s->tx_fifo, buf, size);
> +    if (size > 0) {
> +        fifo8_push_all(&s->tx_fifo, buf, size);
> +    }
>
>      if (fifo8_is_full(&s->tx_fifo)) {
>          s->txfifo |= SIFIVE_UART_TXFIFO_FULL;
> --
> 2.49.0
>
>

Re: [PATCH 3/4] hw/char: sifive_uart: Remove outdated comment about Tx FIFO

[Alistair Francis]

On Fri, Sep 12, 2025 at 2:08 AM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Since Tx FIFO is now implemented using "qemu/fifo8.h", remove the comment
> that no longer reflects the current implementation.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/sifive_uart.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index 401f869680d..baef0bd9c28 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -28,12 +28,6 @@
>
>  #define TX_INTERRUPT_TRIGGER_DELAY_NS 100
>
> -/*
> - * Not yet implemented:
> - *
> - * Transmit FIFO using "qemu/fifo8.h"
> - */
> -
>  /* Returns the state of the IP (interrupt pending) register */
>  static uint32_t sifive_uart_ip(SiFiveUARTState *s)
>  {
> --
> 2.49.0
>
>

Re: [PATCH 4/4] hw/char: sifive_uart: Add newline to error message

[Alistair Francis]

On Fri, Sep 12, 2025 at 2:08 AM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Adds a missing newline character to the error message.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/sifive_uart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
> index baef0bd9c28..e7357d585a1 100644
> --- a/hw/char/sifive_uart.c
> +++ b/hw/char/sifive_uart.c
> @@ -113,7 +113,7 @@ static void sifive_uart_write_tx_fifo(SiFiveUARTState *s, const uint8_t *buf,
>
>      if (size > fifo8_num_free(&s->tx_fifo)) {
>          size = fifo8_num_free(&s->tx_fifo);
> -        qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow");
> +        qemu_log_mask(LOG_GUEST_ERROR, "sifive_uart: TX FIFO overflow.\n");
>      }
>
>      if (size > 0) {
> --
> 2.49.0
>
>

Re: [PATCH 0/4] Fix SiFive UART character drop issue and minor refactors

[Alistair Francis]

On Fri, Sep 12, 2025 at 2:08 AM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> This patch set fixes the SiFive UART character drop issue introduced
> after commit [1], which changed character printing from synchronous to
> asynchronous.
>
> Since UART now transmits characters asynchronously, it is possible for
> the Tx FIFO to become full, causing new characters to be ignored and
> dropped when running Linux. This happens because:
>
>   1. The Linux SiFive UART driver sets the transmit watermark level to 1
>      [2], meaning a transmit watermark interrupt is raised whenever a
>      character is enqueued into the Tx FIFO.
>   2. Upon receiving a transmit watermark interrupt, the Linux driver
>      transfers up to a full Tx FIFO's worth of characters from the Linux
>      serial transmit buffer [3], without checking the txdata.full flag
>      before transferring multiple characters [4].
>
> This patch set updates QEMU to honor the Tx/Rx watermark thresholds and
> raise interrupts only when the Tx threshold is exceeded or the Rx
> threshold is undercut.
>
> The remaining patches contain minor refactors, including removing an
> outdated comment about the Tx FIFO.
>
> [1] 53c1557b230986ab6320a58e1b2c26216ecd86d5
> [2] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L1039
> [3] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L538
> [4] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L291
>
> Frank Chang (4):
>   hw/char: sifive_uart: Raise IRQ according to the Tx/Rx watermark
>     thresholds
>   hw/char: sifive_uart: Avoid pushing Tx FIFO when size is zero
>   hw/char: sifive_uart: Remove outdated comment about Tx FIFO
>   hw/char: sifive_uart: Add newline to error message

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  hw/char/sifive_uart.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> --
> 2.49.0
>
>

Re: [PATCH 0/4] Fix SiFive UART character drop issue and minor refactors

[Michael Tokarev]

On 9/11/25 19:06, frank.chang@sifive.com wrote:
> From: Frank Chang <frank.chang@sifive.com>
> 
> This patch set fixes the SiFive UART character drop issue introduced
> after commit [1], which changed character printing from synchronous to
> asynchronous.
> 
> Since UART now transmits characters asynchronously, it is possible for
> the Tx FIFO to become full, causing new characters to be ignored and
> dropped when running Linux. This happens because:
> 
>    1. The Linux SiFive UART driver sets the transmit watermark level to 1
>       [2], meaning a transmit watermark interrupt is raised whenever a
>       character is enqueued into the Tx FIFO.
>    2. Upon receiving a transmit watermark interrupt, the Linux driver
>       transfers up to a full Tx FIFO's worth of characters from the Linux
>       serial transmit buffer [3], without checking the txdata.full flag
>       before transferring multiple characters [4].
> 
> This patch set updates QEMU to honor the Tx/Rx watermark thresholds and
> raise interrupts only when the Tx threshold is exceeded or the Rx
> threshold is undercut.

This change seems to be worth picking up for qemu-stable series
(10.0 & 10.1).  Please let me know if it is not.

> The remaining patches contain minor refactors, including removing an
> outdated comment about the Tx FIFO.

> [1] 53c1557b230986ab6320a58e1b2c26216ecd86d5
> [2] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L1039
> [3] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L538
> [4] https://github.com/torvalds/linux/blob/master/drivers/tty/serial/sifive.c#L291
> 
> Frank Chang (4):
>    hw/char: sifive_uart: Raise IRQ according to the Tx/Rx watermark
>      thresholds
>    hw/char: sifive_uart: Avoid pushing Tx FIFO when size is zero
>    hw/char: sifive_uart: Remove outdated comment about Tx FIFO
>    hw/char: sifive_uart: Add newline to error message
> 
>   hw/char/sifive_uart.c | 36 ++++++++++++++++--------------------
>   1 file changed, 16 insertions(+), 20 deletions(-)

Thanks,

/mjt