[PATCH v1 0/3] ZynqMP / Versal: various fixes

[PATCH v1 0/3] ZynqMP / Versal: various fixes

[Frederic Konrad]

Hi,

Those are various simple fixes for ZynqMP:
  * 1: fixes a possible out of bound access in the SPI model.
  * 2: is a trivial fix for documentation url.
  * 3: is a log guest error fix for the CSU DMA.

Best Regards,
Fred

Frederic Konrad (3):
  hw/ssi/xilinx_spips: fix an out of bound access
  fix some url for amd / xilinx models
  hw/dma/xlnx_csu_dma: don't throw guest errors when stopping the SRC
    DMA

 hw/dma/xlnx_csu_dma.c                      | 14 +++++++++-----
 hw/ssi/xilinx_spips.c                      |  7 ++++++-
 include/hw/misc/xlnx-versal-cframe-reg.h   |  2 +-
 include/hw/misc/xlnx-versal-cfu.h          |  2 +-
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h |  2 +-
 include/hw/ssi/xilinx_spips.h              |  3 +++
 include/hw/ssi/xlnx-versal-ospi.h          |  2 +-
 7 files changed, 22 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access

[Frederic Konrad]

The spips, qspips, and zynqmp-qspips share the same realize function
(xilinx_spips_realize) and initialize their io memory region with different
mmio_ops passed through the class.  The size of the memory region is set to
the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
out of s->regs[addr] in xilinx_spips_write for spips and qspips.

This fixes that wrong behavior.

Reviewed-by: Luc Michel <luc.michel@amd.com>
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/ssi/xilinx_spips.c         | 7 ++++++-
 include/hw/ssi/xilinx_spips.h | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index a3955c6c50..0bdfad7e2e 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
 
     DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
     addr >>= 2;
+    assert(addr < XLNX_SPIPS_R_MAX);
+
     switch (addr) {
     case R_CONFIG:
         mask = ~(R_CONFIG_RSVD | MAN_START_COM);
@@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
     }
 
     memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
-                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
+                          "spi", xsc->reg_size);
     sysbus_init_mmio(sbd, &s->iomem);
 
     s->irqline = -1;
@@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
 
     dc->realize = xilinx_qspips_realize;
     xsc->reg_ops = &qspips_ops;
+    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
 }
@@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_xilinx_spips;
 
     xsc->reg_ops = &spips_ops;
+    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A;
     xsc->tx_fifo_size = TXFF_A;
 }
@@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
     dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
     device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
     xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
+    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
 }
diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
index 1386d5ac8f..7a754bf67a 100644
--- a/include/hw/ssi/xilinx_spips.h
+++ b/include/hw/ssi/xilinx_spips.h
@@ -33,7 +33,9 @@
 
 typedef struct XilinxSPIPS XilinxSPIPS;
 
+/* For SPIPS, QSPIPS.  */
 #define XLNX_SPIPS_R_MAX        (0x100 / 4)
+/* For ZYNQMP_QSPIPS.  */
 #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
 
 /* Bite off 4k chunks at a time */
@@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
     SysBusDeviceClass parent_class;
 
     const MemoryRegionOps *reg_ops;
+    uint64_t reg_size;
 
     uint32_t rx_fifo_size;
     uint32_t tx_fifo_size;
-- 
2.25.1

[PATCH v1 2/3] fix some url for amd / xilinx models

[Frederic Konrad]

It seems that the url changed a bit, and it triggers an error.  Fix the URLs so
the documentation can be reached again.

Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/dma/xlnx_csu_dma.c                      | 2 +-
 include/hw/misc/xlnx-versal-cframe-reg.h   | 2 +-
 include/hw/misc/xlnx-versal-cfu.h          | 2 +-
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h | 2 +-
 include/hw/ssi/xlnx-versal-ospi.h          | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index e89089821a..531013f35a 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -33,7 +33,7 @@
 
 /*
  * Ref: UG1087 (v1.7) February 8, 2019
- * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
+ * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers
  * CSUDMA Module section
  */
 REG32(ADDR, 0x0)
diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h
index a14fbd7fe4..0091505246 100644
--- a/include/hw/misc/xlnx-versal-cframe-reg.h
+++ b/include/hw/misc/xlnx-versal-cframe-reg.h
@@ -12,7 +12,7 @@
  *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
  *
  * [2] Versal ACAP Register Reference,
- *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
+ *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFRAME_REG-Module
  */
 #ifndef HW_MISC_XLNX_VERSAL_CFRAME_REG_H
 #define HW_MISC_XLNX_VERSAL_CFRAME_REG_H
diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h
index 86fb841053..be62bab8c8 100644
--- a/include/hw/misc/xlnx-versal-cfu.h
+++ b/include/hw/misc/xlnx-versal-cfu.h
@@ -12,7 +12,7 @@
  *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
  *
  * [2] Versal ACAP Register Reference,
- *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
+ *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFU_CSR-Module
  */
 #ifndef HW_MISC_XLNX_VERSAL_CFU_APB_H
 #define HW_MISC_XLNX_VERSAL_CFU_APB_H
diff --git a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
index f7d24c93c4..0c4a4fd66d 100644
--- a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
+++ b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
@@ -34,7 +34,7 @@
  *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
  *
  * [2] Versal ACAP Register Reference,
- *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___pmc_iop_slcr.html
+ *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/PMC_IOP_SLCR-Module
  *
  * QEMU interface:
  * + sysbus MMIO region 0: MemoryRegion for the device's registers
diff --git a/include/hw/ssi/xlnx-versal-ospi.h b/include/hw/ssi/xlnx-versal-ospi.h
index 5d131d351d..4ac975aa2f 100644
--- a/include/hw/ssi/xlnx-versal-ospi.h
+++ b/include/hw/ssi/xlnx-versal-ospi.h
@@ -34,7 +34,7 @@
  *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
  *
  * [2] Versal ACAP Register Reference,
- *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___ospi.html
+ *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/OSPI-Module
  *
  *
  * QEMU interface:
-- 
2.25.1

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

[Frederic Konrad]

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 1/3] hw/ssi/xilinx_spips: fix an out of bound access

[Francisco Iglesias]

On 2023-11-24 15:35, Frederic Konrad wrote:
> The spips, qspips, and zynqmp-qspips share the same realize function
> (xilinx_spips_realize) and initialize their io memory region with different
> mmio_ops passed through the class.  The size of the memory region is set to
> the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
> out of s->regs[addr] in xilinx_spips_write for spips and qspips.
> 
> This fixes that wrong behavior.
> 
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

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


> ---
>   hw/ssi/xilinx_spips.c         | 7 ++++++-
>   include/hw/ssi/xilinx_spips.h | 3 +++
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index a3955c6c50..0bdfad7e2e 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>   
>       DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
>       addr >>= 2;
> +    assert(addr < XLNX_SPIPS_R_MAX);
> +
>       switch (addr) {
>       case R_CONFIG:
>           mask = ~(R_CONFIG_RSVD | MAN_START_COM);
> @@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>       }
>   
>       memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
> -                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
> +                          "spi", xsc->reg_size);
>       sysbus_init_mmio(sbd, &s->iomem);
>   
>       s->irqline = -1;
> @@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>   
>       dc->realize = xilinx_qspips_realize;
>       xsc->reg_ops = &qspips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A_Q;
>       xsc->tx_fifo_size = TXFF_A_Q;
>   }
> @@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_xilinx_spips;
>   
>       xsc->reg_ops = &spips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A;
>       xsc->tx_fifo_size = TXFF_A;
>   }
> @@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
>       dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
>       device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
>       xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
> +    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
>       xsc->rx_fifo_size = RXFF_A_Q;
>       xsc->tx_fifo_size = TXFF_A_Q;
>   }
> diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
> index 1386d5ac8f..7a754bf67a 100644
> --- a/include/hw/ssi/xilinx_spips.h
> +++ b/include/hw/ssi/xilinx_spips.h
> @@ -33,7 +33,9 @@
>   
>   typedef struct XilinxSPIPS XilinxSPIPS;
>   
> +/* For SPIPS, QSPIPS.  */
>   #define XLNX_SPIPS_R_MAX        (0x100 / 4)
> +/* For ZYNQMP_QSPIPS.  */
>   #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
>   
>   /* Bite off 4k chunks at a time */
> @@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
>       SysBusDeviceClass parent_class;
>   
>       const MemoryRegionOps *reg_ops;
> +    uint64_t reg_size;
>   
>       uint32_t rx_fifo_size;
>       uint32_t tx_fifo_size;

Re: [PATCH v1 2/3] fix some url for amd / xilinx models

[Francisco Iglesias]

On 2023-11-24 15:35, Frederic Konrad wrote:
> It seems that the url changed a bit, and it triggers an error.  Fix the URLs so
> the documentation can be reached again.
> 
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> ---
>   hw/dma/xlnx_csu_dma.c                      | 2 +-
>   include/hw/misc/xlnx-versal-cframe-reg.h   | 2 +-
>   include/hw/misc/xlnx-versal-cfu.h          | 2 +-
>   include/hw/misc/xlnx-versal-pmc-iou-slcr.h | 2 +-
>   include/hw/ssi/xlnx-versal-ospi.h          | 2 +-
>   5 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
> index e89089821a..531013f35a 100644
> --- a/hw/dma/xlnx_csu_dma.c
> +++ b/hw/dma/xlnx_csu_dma.c
> @@ -33,7 +33,7 @@
>   
>   /*
>    * Ref: UG1087 (v1.7) February 8, 2019
> - * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> + * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers
>    * CSUDMA Module section
>    */
>   REG32(ADDR, 0x0)
> diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h
> index a14fbd7fe4..0091505246 100644
> --- a/include/hw/misc/xlnx-versal-cframe-reg.h
> +++ b/include/hw/misc/xlnx-versal-cframe-reg.h
> @@ -12,7 +12,7 @@
>    *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>    *
>    * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFRAME_REG-Module
>    */
>   #ifndef HW_MISC_XLNX_VERSAL_CFRAME_REG_H
>   #define HW_MISC_XLNX_VERSAL_CFRAME_REG_H
> diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h
> index 86fb841053..be62bab8c8 100644
> --- a/include/hw/misc/xlnx-versal-cfu.h
> +++ b/include/hw/misc/xlnx-versal-cfu.h
> @@ -12,7 +12,7 @@
>    *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>    *
>    * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFU_CSR-Module
>    */
>   #ifndef HW_MISC_XLNX_VERSAL_CFU_APB_H
>   #define HW_MISC_XLNX_VERSAL_CFU_APB_H
> diff --git a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> index f7d24c93c4..0c4a4fd66d 100644
> --- a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> +++ b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> @@ -34,7 +34,7 @@
>    *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>    *
>    * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___pmc_iop_slcr.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/PMC_IOP_SLCR-Module
>    *
>    * QEMU interface:
>    * + sysbus MMIO region 0: MemoryRegion for the device's registers
> diff --git a/include/hw/ssi/xlnx-versal-ospi.h b/include/hw/ssi/xlnx-versal-ospi.h
> index 5d131d351d..4ac975aa2f 100644
> --- a/include/hw/ssi/xlnx-versal-ospi.h
> +++ b/include/hw/ssi/xlnx-versal-ospi.h
> @@ -34,7 +34,7 @@
>    *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>    *
>    * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___ospi.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/OSPI-Module
>    *
>    *
>    * QEMU interface:

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

[Francisco Iglesias]

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)

Re: [PATCH v1 0/3] ZynqMP / Versal: various fixes

[Peter Maydell]

On Fri, 24 Nov 2023 at 14:35, Frederic Konrad <fkonrad@amd.com> wrote:
>
> Hi,
>
> Those are various simple fixes for ZynqMP:
>   * 1: fixes a possible out of bound access in the SPI model.
>   * 2: is a trivial fix for documentation url.
>   * 3: is a log guest error fix for the CSU DMA.
>



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v1 1/3] hw/ssi/xilinx_spips: fix an out of bound access

[Alistair Francis]

On Sat, Nov 25, 2023 at 12:38 AM Frederic Konrad <fkonrad@amd.com> wrote:
>
> The spips, qspips, and zynqmp-qspips share the same realize function
> (xilinx_spips_realize) and initialize their io memory region with different
> mmio_ops passed through the class.  The size of the memory region is set to
> the largest area (0x200 bytes for zynqmp-qspips) thus it is possible to write
> out of s->regs[addr] in xilinx_spips_write for spips and qspips.
>
> This fixes that wrong behavior.
>
> Reviewed-by: Luc Michel <luc.michel@amd.com>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

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

Alistair

> ---
>  hw/ssi/xilinx_spips.c         | 7 ++++++-
>  include/hw/ssi/xilinx_spips.h | 3 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index a3955c6c50..0bdfad7e2e 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -973,6 +973,8 @@ static void xilinx_spips_write(void *opaque, hwaddr addr,
>
>      DB_PRINT_L(0, "addr=" HWADDR_FMT_plx " = %x\n", addr, (unsigned)value);
>      addr >>= 2;
> +    assert(addr < XLNX_SPIPS_R_MAX);
> +
>      switch (addr) {
>      case R_CONFIG:
>          mask = ~(R_CONFIG_RSVD | MAN_START_COM);
> @@ -1299,7 +1301,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>      }
>
>      memory_region_init_io(&s->iomem, OBJECT(s), xsc->reg_ops, s,
> -                          "spi", XLNX_ZYNQMP_SPIPS_R_MAX * 4);
> +                          "spi", xsc->reg_size);
>      sysbus_init_mmio(sbd, &s->iomem);
>
>      s->irqline = -1;
> @@ -1435,6 +1437,7 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>
>      dc->realize = xilinx_qspips_realize;
>      xsc->reg_ops = &qspips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
>  }
> @@ -1450,6 +1453,7 @@ static void xilinx_spips_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vmstate_xilinx_spips;
>
>      xsc->reg_ops = &spips_ops;
> +    xsc->reg_size = XLNX_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A;
>      xsc->tx_fifo_size = TXFF_A;
>  }
> @@ -1464,6 +1468,7 @@ static void xlnx_zynqmp_qspips_class_init(ObjectClass *klass, void * data)
>      dc->vmsd = &vmstate_xlnx_zynqmp_qspips;
>      device_class_set_props(dc, xilinx_zynqmp_qspips_properties);
>      xsc->reg_ops = &xlnx_zynqmp_qspips_ops;
> +    xsc->reg_size = XLNX_ZYNQMP_SPIPS_R_MAX * 4;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
>  }
> diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
> index 1386d5ac8f..7a754bf67a 100644
> --- a/include/hw/ssi/xilinx_spips.h
> +++ b/include/hw/ssi/xilinx_spips.h
> @@ -33,7 +33,9 @@
>
>  typedef struct XilinxSPIPS XilinxSPIPS;
>
> +/* For SPIPS, QSPIPS.  */
>  #define XLNX_SPIPS_R_MAX        (0x100 / 4)
> +/* For ZYNQMP_QSPIPS.  */
>  #define XLNX_ZYNQMP_SPIPS_R_MAX (0x200 / 4)
>
>  /* Bite off 4k chunks at a time */
> @@ -125,6 +127,7 @@ struct XilinxSPIPSClass {
>      SysBusDeviceClass parent_class;
>
>      const MemoryRegionOps *reg_ops;
> +    uint64_t reg_size;
>
>      uint32_t rx_fifo_size;
>      uint32_t tx_fifo_size;
> --
> 2.25.1
>
>

Re: [PATCH v1 2/3] fix some url for amd / xilinx models

[Alistair Francis]

On Sat, Nov 25, 2023 at 12:37 AM Frederic Konrad <fkonrad@amd.com> wrote:
>
> It seems that the url changed a bit, and it triggers an error.  Fix the URLs so
> the documentation can be reached again.
>
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>

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

Alistair

> ---
>  hw/dma/xlnx_csu_dma.c                      | 2 +-
>  include/hw/misc/xlnx-versal-cframe-reg.h   | 2 +-
>  include/hw/misc/xlnx-versal-cfu.h          | 2 +-
>  include/hw/misc/xlnx-versal-pmc-iou-slcr.h | 2 +-
>  include/hw/ssi/xlnx-versal-ospi.h          | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
> index e89089821a..531013f35a 100644
> --- a/hw/dma/xlnx_csu_dma.c
> +++ b/hw/dma/xlnx_csu_dma.c
> @@ -33,7 +33,7 @@
>
>  /*
>   * Ref: UG1087 (v1.7) February 8, 2019
> - * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> + * https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers
>   * CSUDMA Module section
>   */
>  REG32(ADDR, 0x0)
> diff --git a/include/hw/misc/xlnx-versal-cframe-reg.h b/include/hw/misc/xlnx-versal-cframe-reg.h
> index a14fbd7fe4..0091505246 100644
> --- a/include/hw/misc/xlnx-versal-cframe-reg.h
> +++ b/include/hw/misc/xlnx-versal-cframe-reg.h
> @@ -12,7 +12,7 @@
>   *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>   *
>   * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFRAME_REG-Module
>   */
>  #ifndef HW_MISC_XLNX_VERSAL_CFRAME_REG_H
>  #define HW_MISC_XLNX_VERSAL_CFRAME_REG_H
> diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h
> index 86fb841053..be62bab8c8 100644
> --- a/include/hw/misc/xlnx-versal-cfu.h
> +++ b/include/hw/misc/xlnx-versal-cfu.h
> @@ -12,7 +12,7 @@
>   *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>   *
>   * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/htmldocs/registers/am012/am012-versal-register-reference.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/CFU_CSR-Module
>   */
>  #ifndef HW_MISC_XLNX_VERSAL_CFU_APB_H
>  #define HW_MISC_XLNX_VERSAL_CFU_APB_H
> diff --git a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> index f7d24c93c4..0c4a4fd66d 100644
> --- a/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> +++ b/include/hw/misc/xlnx-versal-pmc-iou-slcr.h
> @@ -34,7 +34,7 @@
>   *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>   *
>   * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___pmc_iop_slcr.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/PMC_IOP_SLCR-Module
>   *
>   * QEMU interface:
>   * + sysbus MMIO region 0: MemoryRegion for the device's registers
> diff --git a/include/hw/ssi/xlnx-versal-ospi.h b/include/hw/ssi/xlnx-versal-ospi.h
> index 5d131d351d..4ac975aa2f 100644
> --- a/include/hw/ssi/xlnx-versal-ospi.h
> +++ b/include/hw/ssi/xlnx-versal-ospi.h
> @@ -34,7 +34,7 @@
>   *     https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
>   *
>   * [2] Versal ACAP Register Reference,
> - *     https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___ospi.html
> + *     https://docs.xilinx.com/r/en-US/am012-versal-register-reference/OSPI-Module
>   *
>   *
>   * QEMU interface:
> --
> 2.25.1
>
>

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

[Alistair Francis]

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