ZynqMP / Versal: various fixes

[PATCH v1 3/3] hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC DMA

Posted by Frederic Konrad 2 years, 2 months ago

UG1087 states for the source channel that: if SIZE is programmed to 0, and the
DMA is started, the interrupts DONE and MEM_DONE will be asserted.

This implies that it is allowed for the guest to stop the source DMA by writing
a size of 0 to the SIZE register, so remove the LOG_GUEST_ERROR in that case.

While at it remove the comment marking the SIZE register as write-only.

See: https://docs.xilinx.com/r/en-US/ug1087-zynq-ultrascale-registers/CSUDMA_SRC_SIZE-CSUDMA-Register

Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/dma/xlnx_csu_dma.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 531013f35a..bc1505aade 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -39,7 +39,7 @@
 REG32(ADDR, 0x0)
     FIELD(ADDR, ADDR, 2, 30) /* wo */
 REG32(SIZE, 0x4)
-    FIELD(SIZE, SIZE, 2, 27) /* wo */
+    FIELD(SIZE, SIZE, 2, 27)
     FIELD(SIZE, LAST_WORD, 0, 1) /* rw, only exists in SRC */
 REG32(STATUS, 0x8)
     FIELD(STATUS, DONE_CNT, 13, 3) /* wtc */
@@ -335,10 +335,14 @@ static uint64_t addr_pre_write(RegisterInfo *reg, uint64_t val)
 static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
 {
     XlnxCSUDMA *s = XLNX_CSU_DMA(reg->opaque);
+    uint64_t size = val & R_SIZE_SIZE_MASK;
 
     if (s->regs[R_SIZE] != 0) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Starting DMA while already running.\n", __func__);
+        if (size || s->is_dst) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Starting DMA while already running.\n",
+                          __func__);
+        }
     }
 
     if (!s->is_dst) {
@@ -346,7 +350,7 @@ static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
     }
 
     /* Size is word aligned */
-    return val & R_SIZE_SIZE_MASK;
+    return size;
 }
 
 static uint64_t size_post_read(RegisterInfo *reg, uint64_t val)
-- 
2.25.1

Re: [PATCH v1 3/3] hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC DMA

Posted by Alistair Francis 2 years, 2 months ago

On Sat, Nov 25, 2023 at 1:36 AM Frederic Konrad <fkonrad@amd.com> wrote:
>
> UG1087 states for the source channel that: if SIZE is programmed to 0, and the
> DMA is started, the interrupts DONE and MEM_DONE will be asserted.
>
> This implies that it is allowed for the guest to stop the source DMA by writing
> a size of 0 to the SIZE register, so remove the LOG_GUEST_ERROR in that case.
>
> While at it remove the comment marking the SIZE register as write-only.
>
> See: https://docs.xilinx.com/r/en-US/ug1087-zynq-ultrascale-registers/CSUDMA_SRC_SIZE-CSUDMA-Register
>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

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

Alistair

> ---
>  hw/dma/xlnx_csu_dma.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
> index 531013f35a..bc1505aade 100644
> --- a/hw/dma/xlnx_csu_dma.c
> +++ b/hw/dma/xlnx_csu_dma.c
> @@ -39,7 +39,7 @@
>  REG32(ADDR, 0x0)
>      FIELD(ADDR, ADDR, 2, 30) /* wo */
>  REG32(SIZE, 0x4)
> -    FIELD(SIZE, SIZE, 2, 27) /* wo */
> +    FIELD(SIZE, SIZE, 2, 27)
>      FIELD(SIZE, LAST_WORD, 0, 1) /* rw, only exists in SRC */
>  REG32(STATUS, 0x8)
>      FIELD(STATUS, DONE_CNT, 13, 3) /* wtc */
> @@ -335,10 +335,14 @@ static uint64_t addr_pre_write(RegisterInfo *reg, uint64_t val)
>  static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
>  {
>      XlnxCSUDMA *s = XLNX_CSU_DMA(reg->opaque);
> +    uint64_t size = val & R_SIZE_SIZE_MASK;
>
>      if (s->regs[R_SIZE] != 0) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: Starting DMA while already running.\n", __func__);
> +        if (size || s->is_dst) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Starting DMA while already running.\n",
> +                          __func__);
> +        }
>      }
>
>      if (!s->is_dst) {
> @@ -346,7 +350,7 @@ static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
>      }
>
>      /* Size is word aligned */
> -    return val & R_SIZE_SIZE_MASK;
> +    return size;
>  }
>
>  static uint64_t size_post_read(RegisterInfo *reg, uint64_t val)
> --
> 2.25.1
>
>

Re: [PATCH v1 3/3] hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC DMA

Posted by Francisco Iglesias 2 years, 2 months ago


On 2023-11-24 15:35, Frederic Konrad wrote:
> UG1087 states for the source channel that: if SIZE is programmed to 0, and the
> DMA is started, the interrupts DONE and MEM_DONE will be asserted.
> 
> This implies that it is allowed for the guest to stop the source DMA by writing
> a size of 0 to the SIZE register, so remove the LOG_GUEST_ERROR in that case.
> 
> While at it remove the comment marking the SIZE register as write-only.
> 
> See: https://docs.xilinx.com/r/en-US/ug1087-zynq-ultrascale-registers/CSUDMA_SRC_SIZE-CSUDMA-Register
> 
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>

> ---
>   hw/dma/xlnx_csu_dma.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
> index 531013f35a..bc1505aade 100644
> --- a/hw/dma/xlnx_csu_dma.c
> +++ b/hw/dma/xlnx_csu_dma.c
> @@ -39,7 +39,7 @@
>   REG32(ADDR, 0x0)
>       FIELD(ADDR, ADDR, 2, 30) /* wo */
>   REG32(SIZE, 0x4)
> -    FIELD(SIZE, SIZE, 2, 27) /* wo */
> +    FIELD(SIZE, SIZE, 2, 27)
>       FIELD(SIZE, LAST_WORD, 0, 1) /* rw, only exists in SRC */
>   REG32(STATUS, 0x8)
>       FIELD(STATUS, DONE_CNT, 13, 3) /* wtc */
> @@ -335,10 +335,14 @@ static uint64_t addr_pre_write(RegisterInfo *reg, uint64_t val)
>   static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
>   {
>       XlnxCSUDMA *s = XLNX_CSU_DMA(reg->opaque);
> +    uint64_t size = val & R_SIZE_SIZE_MASK;
>   
>       if (s->regs[R_SIZE] != 0) {
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: Starting DMA while already running.\n", __func__);
> +        if (size || s->is_dst) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Starting DMA while already running.\n",
> +                          __func__);
> +        }
>       }
>   
>       if (!s->is_dst) {
> @@ -346,7 +350,7 @@ static uint64_t size_pre_write(RegisterInfo *reg, uint64_t val)
>       }
>   
>       /* Size is word aligned */
> -    return val & R_SIZE_SIZE_MASK;
> +    return size;
>   }
>   
>   static uint64_t size_post_read(RegisterInfo *reg, uint64_t val)