[PATCH 1/2] hw/display/xlnx_dp.c: Don't abort on AUX FIFO overrun/underrun

Peter Maydell posted 2 patches 1 week, 1 day ago
Maintainers: Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Peter Maydell <peter.maydell@linaro.org>
[PATCH 1/2] hw/display/xlnx_dp.c: Don't abort on AUX FIFO overrun/underrun
Posted by Peter Maydell 1 week, 1 day ago
The documentation of the Xilinx DisplayPort subsystem at
https://www.xilinx.com/support/documents/ip_documentation/v_dp_txss1/v3_1/pg299-v-dp-txss1.pdf
doesn't say what happens if a guest tries to issue an AUX write
command with a length greater than the amount of data in the AUX
write FIFO, or tries to write more data to the write FIFO than it can
hold, or issues multiple commands that put data into the AUX read
FIFO without reading it such that it overflows.

Currently QEMU will abort() in these guest-error situations, either
in xlnx_dp.c itself or in the fifo8 code.  Make these cases all be
logged as guest errors instead.  We choose to ignore the new data on
overflow, and return 0 on underflow. This is in line with how we handled
the "read from empty RX FIFO" case in commit a09ef5040477.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1418
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1419
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1424
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/xlnx_dp.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 96cbb1b3a7d..c2bf692e7b1 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -435,7 +435,18 @@ static void xlnx_dp_aux_clear_rx_fifo(XlnxDPState *s)
 
 static void xlnx_dp_aux_push_rx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
 {
+    size_t avail = fifo8_num_free(&s->rx_fifo);
     DPRINTF("Push %u data in rx_fifo\n", (unsigned)len);
+    if (len > avail) {
+        /*
+         * Data sheet doesn't specify behaviour here: we choose to ignore
+         * the excess data.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ignoring %zu bytes pushed to full RX_FIFO\n",
+                      __func__, len - avail);
+        len = avail;
+    }
     fifo8_push_all(&s->rx_fifo, buf, len);
 }
 
@@ -466,7 +477,18 @@ static void xlnx_dp_aux_clear_tx_fifo(XlnxDPState *s)
 
 static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
 {
+    size_t avail = fifo8_num_free(&s->tx_fifo);
     DPRINTF("Push %u data in tx_fifo\n", (unsigned)len);
+    if (len > avail) {
+        /*
+         * Data sheet doesn't specify behaviour here: we choose to ignore
+         * the excess data.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ignoring %zu bytes pushed to full TX_FIFO\n",
+                      __func__, len - avail);
+        len = avail;
+    }
     fifo8_push_all(&s->tx_fifo, buf, len);
 }
 
@@ -475,8 +497,10 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
     uint8_t ret;
 
     if (fifo8_is_empty(&s->tx_fifo)) {
-        error_report("%s: TX_FIFO underflow", __func__);
-        abort();
+        /* Data sheet doesn't specify behaviour here: we choose to return 0 */
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: attempt to read empty TX_FIFO\n",
+                      __func__);
+        return 0;
     }
     ret = fifo8_pop(&s->tx_fifo);
     DPRINTF("pop 0x%2.2X from tx_fifo.\n", ret);
-- 
2.43.0
Re: [PATCH 1/2] hw/display/xlnx_dp.c: Don't abort on AUX FIFO overrun/underrun
Posted by Edgar E. Iglesias 1 week ago
On Thu, Nov 06, 2025 at 02:52:08PM +0000, Peter Maydell wrote:
> The documentation of the Xilinx DisplayPort subsystem at
> https://www.xilinx.com/support/documents/ip_documentation/v_dp_txss1/v3_1/pg299-v-dp-txss1.pdf
> doesn't say what happens if a guest tries to issue an AUX write
> command with a length greater than the amount of data in the AUX
> write FIFO, or tries to write more data to the write FIFO than it can
> hold, or issues multiple commands that put data into the AUX read
> FIFO without reading it such that it overflows.
> 
> Currently QEMU will abort() in these guest-error situations, either
> in xlnx_dp.c itself or in the fifo8 code.  Make these cases all be
> logged as guest errors instead.  We choose to ignore the new data on
> overflow, and return 0 on underflow. This is in line with how we handled
> the "read from empty RX FIFO" case in commit a09ef5040477.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1418
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1419
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1424
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



> ---
>  hw/display/xlnx_dp.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 96cbb1b3a7d..c2bf692e7b1 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -435,7 +435,18 @@ static void xlnx_dp_aux_clear_rx_fifo(XlnxDPState *s)
>  
>  static void xlnx_dp_aux_push_rx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
>  {
> +    size_t avail = fifo8_num_free(&s->rx_fifo);
>      DPRINTF("Push %u data in rx_fifo\n", (unsigned)len);
> +    if (len > avail) {
> +        /*
> +         * Data sheet doesn't specify behaviour here: we choose to ignore
> +         * the excess data.
> +         */
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: ignoring %zu bytes pushed to full RX_FIFO\n",
> +                      __func__, len - avail);
> +        len = avail;
> +    }
>      fifo8_push_all(&s->rx_fifo, buf, len);
>  }
>  
> @@ -466,7 +477,18 @@ static void xlnx_dp_aux_clear_tx_fifo(XlnxDPState *s)
>  
>  static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
>  {
> +    size_t avail = fifo8_num_free(&s->tx_fifo);
>      DPRINTF("Push %u data in tx_fifo\n", (unsigned)len);
> +    if (len > avail) {
> +        /*
> +         * Data sheet doesn't specify behaviour here: we choose to ignore
> +         * the excess data.
> +         */
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: ignoring %zu bytes pushed to full TX_FIFO\n",
> +                      __func__, len - avail);
> +        len = avail;
> +    }
>      fifo8_push_all(&s->tx_fifo, buf, len);
>  }
>  
> @@ -475,8 +497,10 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
>      uint8_t ret;
>  
>      if (fifo8_is_empty(&s->tx_fifo)) {
> -        error_report("%s: TX_FIFO underflow", __func__);
> -        abort();
> +        /* Data sheet doesn't specify behaviour here: we choose to return 0 */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: attempt to read empty TX_FIFO\n",
> +                      __func__);
> +        return 0;
>      }
>      ret = fifo8_pop(&s->tx_fifo);
>      DPRINTF("pop 0x%2.2X from tx_fifo.\n", ret);
> -- 
> 2.43.0
>
Re: [PATCH 1/2] hw/display/xlnx_dp.c: Don't abort on AUX FIFO overrun/underrun
Posted by Philippe Mathieu-Daudé 1 week ago
On 6/11/25 15:52, Peter Maydell wrote:
> The documentation of the Xilinx DisplayPort subsystem at
> https://www.xilinx.com/support/documents/ip_documentation/v_dp_txss1/v3_1/pg299-v-dp-txss1.pdf
> doesn't say what happens if a guest tries to issue an AUX write
> command with a length greater than the amount of data in the AUX
> write FIFO, or tries to write more data to the write FIFO than it can
> hold, or issues multiple commands that put data into the AUX read
> FIFO without reading it such that it overflows.
> 
> Currently QEMU will abort() in these guest-error situations, either
> in xlnx_dp.c itself or in the fifo8 code.  Make these cases all be
> logged as guest errors instead.  We choose to ignore the new data on
> overflow, and return 0 on underflow. This is in line with how we handled
> the "read from empty RX FIFO" case in commit a09ef5040477.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1418
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1419
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1424
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/display/xlnx_dp.c | 28 ++++++++++++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)

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