[PATCH v4 0/3] Drop ignore_memory_transaction_failures for xilink_zynq

[PATCH v4 0/3] Drop ignore_memory_transaction_failures for xilink_zynq

[Chao Liu]

Hi, maintainer,

Following the reference from the chip manualug585-Zynq-7000-TRM manual
B.3 (Module Summary), placeholders have been added for all unimplemented
devices, including the AXI and AMBA bus controllers that interact with
the FPGA.

We can check against the manual by printing the address space of the
zynq board with the following qemu command:
    ${QEMU_PATH}/qemu-system-aarch64 \
    -M xilinx-zynq-a9 \
    -display none \
    -monitor stdio -s
    (qemu) info mtree -f

The testing methodology previously discussed in earlier email exchanges
will not be repeated here.

Chao Liu (3):
  xilink_zynq: Add various missing unimplemented devices
  xilink-zynq-devcfg: Fix up for memory address range size not set
    correctly
  xilink-zynq-devcfg: Avoid disabling devcfg memory region during
    initialization

 hw/arm/xilinx_zynq.c              | 71 ++++++++++++++++++++++++++++++-
 hw/dma/xlnx-zynq-devcfg.c         |  9 +++-
 include/hw/dma/xlnx-zynq-devcfg.h |  2 +-
 3 files changed, 78 insertions(+), 4 deletions(-)

-- 
2.46.1

[PATCH v4 1/3] xilink_zynq: Add various missing unimplemented devices

[Chao Liu]

Add xilinx zynq board memory mapping is implemented in the device.

Remove a ignore_memory_transaction_failures concurrently.

See: ug585-Zynq-7000-TRM manual B.3 (Module Summary)
Signed-off-by: Chao Liu <chao.liu@yeah.net>
---
 hw/arm/xilinx_zynq.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 37c234f5ab..ca21b313b7 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -34,6 +34,7 @@
 #include "hw/net/cadence_gem.h"
 #include "hw/cpu/a9mpcore.h"
 #include "hw/qdev-clock.h"
+#include "hw/misc/unimp.h"
 #include "sysemu/reset.h"
 #include "qom/object.h"
 #include "exec/tswap.h"
@@ -373,6 +374,75 @@ static void zynq_init(MachineState *machine)
     sysbus_connect_irq(busdev, 0, pic[40 - IRQ_OFFSET]);
     sysbus_mmio_map(busdev, 0, 0xF8007000);
 
+    /*
+     * Refer to the ug585-Zynq-7000-TRM manual B.3 (Module Summary) and
+     * the zynq-7000.dtsi. Add placeholders for unimplemented devices.
+     */
+    create_unimplemented_device("zynq.i2c0", 0xE0004000, 4 * KiB);
+    create_unimplemented_device("zynq.i2c1", 0xE0005000, 4 * KiB);
+    create_unimplemented_device("zynq.can0", 0xE0008000, 4 * KiB);
+    create_unimplemented_device("zynq.can1", 0xE0009000, 4 * KiB);
+    create_unimplemented_device("zynq.gpio", 0xE000A000, 4 * KiB);
+    create_unimplemented_device("zynq.smcc", 0xE000E000, 4 * KiB);
+
+    /* Direct Memory Access Controller, PL330, Non-Secure Mode */
+    create_unimplemented_device("zynq.dma_ns", 0xF8004000, 4 * KiB);
+
+    /* System Watchdog Timer Registers */
+    create_unimplemented_device("zynq.swdt", 0xF8005000, 4 * KiB);
+
+    /* DDR memory controller */
+    create_unimplemented_device("zynq.ddrc", 0xF8006000, 4 * KiB);
+
+    /* AXI_HP Interface (AFI) */
+    create_unimplemented_device("zynq.axi_hp0", 0xF8008000, 0x28);
+    create_unimplemented_device("zynq.axi_hp1", 0xF8009000, 0x28);
+    create_unimplemented_device("zynq.axi_hp2", 0xF800A000, 0x28);
+    create_unimplemented_device("zynq.axi_hp3", 0xF800B000, 0x28);
+
+    create_unimplemented_device("zynq.efuse", 0xF800d000, 0x20);
+
+    /* Embedded Trace Buffer */
+    create_unimplemented_device("zynq.etb", 0xF8801000, 4 * KiB);
+
+    /* Cross Trigger Interface, ETB and TPIU */
+    create_unimplemented_device("zynq.cti_etb_tpiu", 0xF8802000, 4 * KiB);
+
+    /* Trace Port Interface Unit */
+    create_unimplemented_device("zynq.tpiu", 0xF8803000, 4 * KiB);
+
+    /* CoreSight Trace Funnel */
+    create_unimplemented_device("zynq.funnel", 0xF8804000, 4 * KiB);
+
+    /* Instrumentation Trace Macrocell */
+    create_unimplemented_device("zynq.itm", 0xF8805000, 4 * KiB);
+
+    /* Cross Trigger Interface, FTM */
+    create_unimplemented_device("zynq.cti_ftm", 0xF8809000, 4 * KiB);
+
+    /* Fabric Trace Macrocell */
+    create_unimplemented_device("zynq.ftm", 0xF880B000, 4 * KiB);
+
+    /* Cortex A9 Performance Monitoring Unit, CPU */
+    create_unimplemented_device("cortex-a9.pmu0", 0xF8891000, 4 * KiB);
+    create_unimplemented_device("cortex-a9.pmu1", 0xF8893000, 4 * KiB);
+
+    /* Cross Trigger Interface, CPU */
+    create_unimplemented_device("zynq.cpu_cti0", 0xF8898000, 4 * KiB);
+    create_unimplemented_device("zynq.cpu_cti1", 0xF8899000, 4 * KiB);
+
+    /* CoreSight PTM-A9, CPU */
+    create_unimplemented_device("cortex-a9.ptm0", 0xF889c000, 4 * KiB);
+    create_unimplemented_device("cortex-a9.ptm1", 0xF889d000, 4 * KiB);
+
+    /* AMBA NIC301 TrustZone */
+    create_unimplemented_device("zynq.trustZone", 0xF8900000, 0x20);
+
+    /* AMBA Network Interconnect Advanced Quality of Service (QoS-301) */
+    create_unimplemented_device("zynq.qos301_cpu", 0xF8946000, 0x130);
+    create_unimplemented_device("zynq.qos301_dmac", 0xF8947000, 0x130);
+    create_unimplemented_device("zynq.qos301_iou", 0xF8948000, 0x130);
+
     zynq_binfo.ram_size = machine->ram_size;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
@@ -394,7 +464,6 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
     mc->init = zynq_init;
     mc->max_cpus = ZYNQ_MAX_CPUS;
     mc->no_sdcard = 1;
-    mc->ignore_memory_transaction_failures = true;
     mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_id = "zynq.ext_ram";
     prop = object_class_property_add_str(oc, "boot-mode", NULL,
-- 
2.46.1

Re: [PATCH v4 1/3] xilink_zynq: Add various missing unimplemented devices

[Peter Maydell]

On Mon, 7 Oct 2024 at 12:25, Chao Liu <chao.liu@yeah.net> wrote:
>
> Add xilinx zynq board memory mapping is implemented in the device.
>
> Remove a ignore_memory_transaction_failures concurrently.
>
> See: ug585-Zynq-7000-TRM manual B.3 (Module Summary)
> Signed-off-by: Chao Liu <chao.liu@yeah.net>

The list of new unimplemented devices looks good. However
we do not want to remove ignore_memory_transaction_failures
in this patch:
 (1) patch 3 says it fixes a breakage if we stop ignoring
     memory transaction failures, so we mustn't turn off
     the flag until after that point, or we break bisection
 (2) if we discover a problem with turning off the flag,
     it's easier to detect and revert if that is done in
     its own patch, not together with the addition of all
     the unimplemented-device devices
 (3) turning off the flag needs to come with a description
     of all the testing we've done that makes us confident
     we can do it

thanks
-- PMM

[PATCH v4 2/3] xilink-zynq-devcfg: Fix up for memory address range size not set correctly

[Chao Liu]

Signed-off-by: Chao Liu <chao.liu@yeah.net>
---
 hw/dma/xlnx-zynq-devcfg.c         | 2 +-
 include/hw/dma/xlnx-zynq-devcfg.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
index b8544d0731..e5eff9abc0 100644
--- a/hw/dma/xlnx-zynq-devcfg.c
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -365,7 +365,7 @@ static void xlnx_zynq_devcfg_init(Object *obj)
 
     sysbus_init_irq(sbd, &s->irq);
 
-    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
+    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX);
     reg_array =
         register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
                               ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
index e4cf085d70..fc26132069 100644
--- a/include/hw/dma/xlnx-zynq-devcfg.h
+++ b/include/hw/dma/xlnx-zynq-devcfg.h
@@ -35,7 +35,7 @@
 
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG)
 
-#define XLNX_ZYNQ_DEVCFG_R_MAX (0x100 / 4)
+#define XLNX_ZYNQ_DEVCFG_R_MAX 0x100
 
 #define XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN 10
 
-- 
2.46.1

Re: [PATCH v4 2/3] xilink-zynq-devcfg: Fix up for memory address range size not set correctly

[Peter Maydell]

On Mon, 7 Oct 2024 at 12:25, Chao Liu <chao.liu@yeah.net> wrote:
>
> Signed-off-by: Chao Liu <chao.liu@yeah.net>
> ---
>  hw/dma/xlnx-zynq-devcfg.c         | 2 +-
>  include/hw/dma/xlnx-zynq-devcfg.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
> index b8544d0731..e5eff9abc0 100644
> --- a/hw/dma/xlnx-zynq-devcfg.c
> +++ b/hw/dma/xlnx-zynq-devcfg.c
> @@ -365,7 +365,7 @@ static void xlnx_zynq_devcfg_init(Object *obj)
>
>      sysbus_init_irq(sbd, &s->irq);
>
> -    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX * 4);
> +    memory_region_init(&s->iomem, obj, "devcfg", XLNX_ZYNQ_DEVCFG_R_MAX);
>      reg_array =
>          register_init_block32(DEVICE(obj), xlnx_zynq_devcfg_regs_info,
>                                ARRAY_SIZE(xlnx_zynq_devcfg_regs_info),
> diff --git a/include/hw/dma/xlnx-zynq-devcfg.h b/include/hw/dma/xlnx-zynq-devcfg.h
> index e4cf085d70..fc26132069 100644
> --- a/include/hw/dma/xlnx-zynq-devcfg.h
> +++ b/include/hw/dma/xlnx-zynq-devcfg.h
> @@ -35,7 +35,7 @@
>
>  OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG)
>
> -#define XLNX_ZYNQ_DEVCFG_R_MAX (0x100 / 4)
> +#define XLNX_ZYNQ_DEVCFG_R_MAX 0x100

This doesn't look right. The device tree in Linux says this
device is 0x100 bytes long. In QEMU the _R_MAX type constant
generally is the number of (32-bit) registers, hence the
division by 4 here to go from bytes to words, and the multiply
by 4 to get the memory_region_init() argument which is bytes
again.

What is this patch trying to fix?

("What is this change fixing" is the kind of thing you
should explain in the commit message.)

thanks
-- PMM

[PATCH v4 3/3] xilink-zynq-devcfg: Avoid disabling devcfg memory region during initialization

[Chao Liu]

During the initialization phase, we've encountered an issue where the
UNLOCK register is inadvertently cleared. This results in devcfg MR being
disabled, which in turn leads to unexpected memory access exceptions when
attempting subsequent accesses to the devcfg register. This behavior is not
consistent with the hardware specifications.

This bug was not found earlier because the ignore_memory_transaction_failures
flag was enabled, which ignored exceptions from devcfg devices
when access was disabled.

Signed-off-by: Chao Liu <chao.liu@yeah.net>
---
 hw/dma/xlnx-zynq-devcfg.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
index e5eff9abc0..af8cc72471 100644
--- a/hw/dma/xlnx-zynq-devcfg.c
+++ b/hw/dma/xlnx-zynq-devcfg.c
@@ -144,7 +144,12 @@ static void xlnx_zynq_devcfg_reset(DeviceState *dev)
     int i;
 
     for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
-        register_reset(&s->regs_info[i]);
+        if (s->regs_info[i].access) {
+            if (s->regs_info[i].access->addr == A_UNLOCK) {
+                continue;
+            }
+            register_reset(&s->regs_info[i]);
+        }
     }
 }
 
-- 
2.46.1

Re: [PATCH v4 3/3] xilink-zynq-devcfg: Avoid disabling devcfg memory region during initialization

[Peter Maydell]

On Mon, 7 Oct 2024 at 12:25, Chao Liu <chao.liu@yeah.net> wrote:
>
> During the initialization phase, we've encountered an issue where the
> UNLOCK register is inadvertently cleared. This results in devcfg MR being
> disabled, which in turn leads to unexpected memory access exceptions when
> attempting subsequent accesses to the devcfg register. This behavior is not
> consistent with the hardware specifications.
>
> This bug was not found earlier because the ignore_memory_transaction_failures
> flag was enabled, which ignored exceptions from devcfg devices
> when access was disabled.
>
> Signed-off-by: Chao Liu <chao.liu@yeah.net>
> ---
>  hw/dma/xlnx-zynq-devcfg.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/dma/xlnx-zynq-devcfg.c b/hw/dma/xlnx-zynq-devcfg.c
> index e5eff9abc0..af8cc72471 100644
> --- a/hw/dma/xlnx-zynq-devcfg.c
> +++ b/hw/dma/xlnx-zynq-devcfg.c
> @@ -144,7 +144,12 @@ static void xlnx_zynq_devcfg_reset(DeviceState *dev)
>      int i;
>
>      for (i = 0; i < XLNX_ZYNQ_DEVCFG_R_MAX; ++i) {
> -        register_reset(&s->regs_info[i]);
> +        if (s->regs_info[i].access) {
> +            if (s->regs_info[i].access->addr == A_UNLOCK) {
> +                continue;
> +            }
> +            register_reset(&s->regs_info[i]);
> +        }
>      }

This looks strange. Reset should reset things. If the
UNLOCK register's reset behaviour is wrong, then shouldn't
we fix it, rather than explicitly skipping resetting it ?

thanks
-- PMM

Re: [PATCH v4 0/3] Drop ignore_memory_transaction_failures for xilink_zynq

[Peter Maydell]

On Mon, 7 Oct 2024 at 12:25, Chao Liu <chao.liu@yeah.net> wrote:
>
> Hi, maintainer,
>
> Following the reference from the chip manualug585-Zynq-7000-TRM manual
> B.3 (Module Summary), placeholders have been added for all unimplemented
> devices, including the AXI and AMBA bus controllers that interact with
> the FPGA.
>
> We can check against the manual by printing the address space of the
> zynq board with the following qemu command:
>     ${QEMU_PATH}/qemu-system-aarch64 \
>     -M xilinx-zynq-a9 \
>     -display none \
>     -monitor stdio -s
>     (qemu) info mtree -f
>
> The testing methodology previously discussed in earlier email exchanges
> will not be repeated here.
>
> Chao Liu (3):
>   xilink_zynq: Add various missing unimplemented devices
>   xilink-zynq-devcfg: Fix up for memory address range size not set
>     correctly
>   xilink-zynq-devcfg: Avoid disabling devcfg memory region during
>     initialization

I've left comments for patches 2 and 3. I have taken patch 1
into target-arm.next, with the ignore_memory_transaction_failures
line reinstated.

I'm all in favour of our being able to get rid of that
legacy flag setting for this board, but as I've said on
previous versions of this patchset, we need to have
confidence that it's not going to break existing guest
code, which means the patch removing it needs to come
with a description of the testing that's been done
(which should be more than "Linux still boots").

thanks
-- PMM