[PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[Peter Maydell]

This patchset fixes some bugs in how we were modelling the
TCMs and the SRAM in the SSE-300 which were preventing
Arm TF-M from booting on our AN547 model; there are also
some fixes to things I noticed while I was in the code.

The specific bugs preventing boot were:
 * SRAM_ADDR_WIDTH for the AN547 is 21, not 15, so the SRAM
   area was too small
 * we were putting the SRAMs at the wrong address (0x2100_0000
   for SSE-300, not 0x2000_0000 as for SSE-200)

The other stuff I've fixed is:
 * we were modelling the SRAM in the AN524 both in the SSE
   and in the board model (harmlessly, as the board-model
   memory was just always shadowed in the memory map and
   unreachable)
 * we were modelling the TCMs in the AN547 board model,
   which is conceptually wrong because in hardware they're
   part of the SSE-300. No guest-visible change, but it will
   avoid problems if/when we add another SSE-300 board model

thanks
-- PMM

Peter Maydell (6):
  hw/arm/mps2-tz: Don't duplicate modelling of SRAM in AN524
  hw/arm/mps2-tz: Make SRAM_ADDR_WIDTH board-specific
  hw/arm/armsse.c: Correct modelling of SSE-300 internal SRAMs
  hw/arm/armsse: Convert armsse_realize() to use ERRP_GUARD
  hw/arm/mps2-tz: Allow board to specify a boot RAM size
  hw/arm: Model TCMs in the SSE-300, not the AN547

 include/hw/arm/armsse.h |  2 ++
 hw/arm/armsse.c         | 35 +++++++++++++++++++++++++++++------
 hw/arm/mps2-tz.c        | 39 ++++++++++++++++++++-------------------
 3 files changed, 51 insertions(+), 25 deletions(-)

-- 
2.20.1

[PATCH 1/6] hw/arm/mps2-tz: Don't duplicate modelling of SRAM in AN524

[Peter Maydell]

The SRAM at 0x2000_0000 is part of the SSE-200 itself, and we model
it that way in hw/arm/armsse.c (along with the associated MPCs).  We
incorrectly also added an entry to the RAMInfo array for the AN524 in
hw/arm/mps2-tz.c, which was pointless because the CPU would never see
it.  Delete it.

The bug had no guest-visible effect because devices in the SSE-200
take priority over those in the board model (armsse.c maps
s->board_memory at priority -2).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 70aa31a7f6c..77ff83acb06 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -243,19 +243,13 @@ static const RAMInfo an524_raminfo[] = { {
         .size = 512 * KiB,
         .mpc = 0,
         .mrindex = 0,
-    }, {
-        .name = "sram",
-        .base = 0x20000000,
-        .size = 32 * 4 * KiB,
-        .mpc = -1,
-        .mrindex = 1,
     }, {
         /* We don't model QSPI flash yet; for now expose it as simple ROM */
         .name = "QSPI",
         .base = 0x28000000,
         .size = 8 * MiB,
         .mpc = 1,
-        .mrindex = 2,
+        .mrindex = 1,
         .flags = IS_ROM,
     }, {
         .name = "DDR",
-- 
2.20.1

Re: [PATCH 1/6] hw/arm/mps2-tz: Don't duplicate modelling of SRAM in AN524

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> The SRAM at 0x2000_0000 is part of the SSE-200 itself, and we model
> it that way in hw/arm/armsse.c (along with the associated MPCs).  We
> incorrectly also added an entry to the RAMInfo array for the AN524 in
> hw/arm/mps2-tz.c, which was pointless because the CPU would never see
> it.  Delete it.
> 
> The bug had no guest-visible effect because devices in the SSE-200
> take priority over those in the board model (armsse.c maps
> s->board_memory at priority -2).
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/6] hw/arm/mps2-tz: Make SRAM_ADDR_WIDTH board-specific

[Peter Maydell]

The AN547 sets the SRAM_ADDR_WIDTH for the SSE-300 to 21;
since this is not the default value for the SSE-300, model this
in mps2-tz.c as a per-board value.

Reported-by: Devaraj Ranganna <devaraj.ranganna@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 77ff83acb06..f2595b1c7f7 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -123,6 +123,7 @@ struct MPS2TZMachineClass {
     int numirq; /* Number of external interrupts */
     int uart_overflow_irq; /* number of the combined UART overflow IRQ */
     uint32_t init_svtor; /* init-svtor setting for SSE */
+    uint32_t sram_addr_width; /* SRAM_ADDR_WIDTH setting for SSE */
     const RAMInfo *raminfo;
     const char *armsse_type;
 };
@@ -806,6 +807,7 @@ static void mps2tz_common_init(MachineState *machine)
                              OBJECT(system_memory), &error_abort);
     qdev_prop_set_uint32(iotkitdev, "EXP_NUMIRQ", mmc->numirq);
     qdev_prop_set_uint32(iotkitdev, "init-svtor", mmc->init_svtor);
+    qdev_prop_set_uint32(iotkitdev, "SRAM_ADDR_WIDTH", mmc->sram_addr_width);
     qdev_connect_clock_in(iotkitdev, "MAINCLK", mms->sysclk);
     qdev_connect_clock_in(iotkitdev, "S32KCLK", mms->s32kclk);
     sysbus_realize(SYS_BUS_DEVICE(&mms->iotkit), &error_fatal);
@@ -1263,6 +1265,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
     mmc->numirq = 92;
     mmc->uart_overflow_irq = 47;
     mmc->init_svtor = 0x10000000;
+    mmc->sram_addr_width = 15;
     mmc->raminfo = an505_raminfo;
     mmc->armsse_type = TYPE_IOTKIT;
     mps2tz_set_default_ram_info(mmc);
@@ -1290,6 +1293,7 @@ static void mps2tz_an521_class_init(ObjectClass *oc, void *data)
     mmc->numirq = 92;
     mmc->uart_overflow_irq = 47;
     mmc->init_svtor = 0x10000000;
+    mmc->sram_addr_width = 15;
     mmc->raminfo = an505_raminfo; /* AN521 is the same as AN505 here */
     mmc->armsse_type = TYPE_SSE200;
     mps2tz_set_default_ram_info(mmc);
@@ -1317,6 +1321,7 @@ static void mps3tz_an524_class_init(ObjectClass *oc, void *data)
     mmc->numirq = 95;
     mmc->uart_overflow_irq = 47;
     mmc->init_svtor = 0x10000000;
+    mmc->sram_addr_width = 15;
     mmc->raminfo = an524_raminfo;
     mmc->armsse_type = TYPE_SSE200;
     mps2tz_set_default_ram_info(mmc);
@@ -1349,6 +1354,7 @@ static void mps3tz_an547_class_init(ObjectClass *oc, void *data)
     mmc->numirq = 96;
     mmc->uart_overflow_irq = 48;
     mmc->init_svtor = 0x00000000;
+    mmc->sram_addr_width = 21;
     mmc->raminfo = an547_raminfo;
     mmc->armsse_type = TYPE_SSE300;
     mps2tz_set_default_ram_info(mmc);
-- 
2.20.1

Re: [PATCH 2/6] hw/arm/mps2-tz: Make SRAM_ADDR_WIDTH board-specific

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> The AN547 sets the SRAM_ADDR_WIDTH for the SSE-300 to 21;
> since this is not the default value for the SSE-300, model this
> in mps2-tz.c as a per-board value.
> 
> Reported-by: Devaraj Ranganna<devaraj.ranganna@linaro.org>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/mps2-tz.c | 6 ++++++
>   1 file changed, 6 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/6] hw/arm/armsse.c: Correct modelling of SSE-300 internal SRAMs

[Peter Maydell]

The SSE-300 was not correctly modelling its internal SRAMs:
 * the SRAM address width default is 18
 * the SRAM is mapped at 0x2100_0000, not 0x2000_0000 like
   the SSE-200 and IoTKit

The default address width is no longer guest-visible since
our only SSE-300 board sets it explicitly to a non-default
value, but following the hardware's default will help for
any future boards we need to model.

Reported-by: Devaraj Ranganna <devaraj.ranganna@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/armsse.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 2e5d0679e7b..1729f09c7cb 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -59,6 +59,7 @@ struct ARMSSEInfo {
     const char *cpu_type;
     uint32_t sse_version;
     int sram_banks;
+    uint32_t sram_bank_base;
     int num_cpus;
     uint32_t sys_version;
     uint32_t iidr;
@@ -102,7 +103,7 @@ static Property sse300_properties[] = {
     DEFINE_PROP_LINK("memory", ARMSSE, board_memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
     DEFINE_PROP_UINT32("EXP_NUMIRQ", ARMSSE, exp_numirq, 64),
-    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 15),
+    DEFINE_PROP_UINT32("SRAM_ADDR_WIDTH", ARMSSE, sram_addr_width, 18),
     DEFINE_PROP_UINT32("init-svtor", ARMSSE, init_svtor, 0x10000000),
     DEFINE_PROP_BOOL("CPU0_FPU", ARMSSE, cpu_fpu[0], true),
     DEFINE_PROP_BOOL("CPU0_DSP", ARMSSE, cpu_dsp[0], true),
@@ -504,6 +505,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .sse_version = ARMSSE_IOTKIT,
         .cpu_type = ARM_CPU_TYPE_NAME("cortex-m33"),
         .sram_banks = 1,
+        .sram_bank_base = 0x20000000,
         .num_cpus = 1,
         .sys_version = 0x41743,
         .iidr = 0,
@@ -523,6 +525,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .sse_version = ARMSSE_SSE200,
         .cpu_type = ARM_CPU_TYPE_NAME("cortex-m33"),
         .sram_banks = 4,
+        .sram_bank_base = 0x20000000,
         .num_cpus = 2,
         .sys_version = 0x22041743,
         .iidr = 0,
@@ -542,6 +545,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .sse_version = ARMSSE_SSE300,
         .cpu_type = ARM_CPU_TYPE_NAME("cortex-m55"),
         .sram_banks = 2,
+        .sram_bank_base = 0x21000000,
         .num_cpus = 1,
         .sys_version = 0x7e00043b,
         .iidr = 0x74a0043b,
@@ -1161,7 +1165,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
         /* Map the upstream end of the MPC into the right place... */
         sbd_mpc = SYS_BUS_DEVICE(&s->mpc[i]);
         memory_region_add_subregion(&s->container,
-                                    0x20000000 + i * sram_bank_size,
+                                    info->sram_bank_base + i * sram_bank_size,
                                     sysbus_mmio_get_region(sbd_mpc, 1));
         /* ...and its register interface */
         memory_region_add_subregion(&s->container, 0x50083000 + i * 0x1000,
-- 
2.20.1

Re: [PATCH 3/6] hw/arm/armsse.c: Correct modelling of SSE-300 internal SRAMs

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> The SSE-300 was not correctly modelling its internal SRAMs:
>   * the SRAM address width default is 18
>   * the SRAM is mapped at 0x2100_0000, not 0x2000_0000 like
>     the SSE-200 and IoTKit
> 
> The default address width is no longer guest-visible since
> our only SSE-300 board sets it explicitly to a non-default
> value, but following the hardware's default will help for
> any future boards we need to model.
> 
> Reported-by: Devaraj Ranganna<devaraj.ranganna@linaro.org>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/armsse.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/6] hw/arm/armsse: Convert armsse_realize() to use ERRP_GUARD

[Peter Maydell]

Convert armsse_realize() to use ERRP_GUARD(), following
the rules in include/qapi/error.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We'll be adding a new error check in the next patch, so
do this first to avoid adding more uses of legacy
error_propagate().
---
 hw/arm/armsse.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 1729f09c7cb..be5aa1f113a 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -913,7 +913,6 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     const ARMSSEDeviceInfo *devinfo;
     int i;
     MemoryRegion *mr;
-    Error *err = NULL;
     SysBusDevice *sbd_apb_ppc0;
     SysBusDevice *sbd_secctl;
     DeviceState *dev_apb_ppc0;
@@ -922,6 +921,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     DeviceState *dev_splitter;
     uint32_t addr_width_max;
 
+    ERRP_GUARD();
+
     if (!s->board_memory) {
         error_setg(errp, "memory property was not set");
         return;
@@ -1151,10 +1152,9 @@ static void armsse_realize(DeviceState *dev, Error **errp)
         uint32_t sram_bank_size = 1 << s->sram_addr_width;
 
         memory_region_init_ram(&s->sram[i], NULL, ramname,
-                               sram_bank_size, &err);
+                               sram_bank_size, errp);
         g_free(ramname);
-        if (err) {
-            error_propagate(errp, err);
+        if (*errp) {
             return;
         }
         object_property_set_link(OBJECT(&s->mpc[i]), "downstream",
-- 
2.20.1

Re: [PATCH 4/6] hw/arm/armsse: Convert armsse_realize() to use ERRP_GUARD

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> Convert armsse_realize() to use ERRP_GUARD(), following
> the rules in include/qapi/error.h.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> We'll be adding a new error check in the next patch, so
> do this first to avoid adding more uses of legacy
> error_propagate().
> ---
>   hw/arm/armsse.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 5/6] hw/arm/mps2-tz: Allow board to specify a boot RAM size

[Peter Maydell]

Currently we model the ITCM in the AN547's RAMInfo list. This is incorrect
because this RAM is really a part of the SSE-300. We can't just delete
it from the RAMInfo list, though, because this would make boot_ram_size()
assert because it wouldn't be able to find an entry in the list covering
guest address 0.

Allow a board to specify a boot RAM size manually if it doesn't have
any RAM itself at address 0 and is relying on the SSE for that, and
set the correct value for the AN547. The other boards can continue
to use the "look it up from the RAMInfo list" logic.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/mps2-tz.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index f2595b1c7f7..8d921afec14 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -126,6 +126,7 @@ struct MPS2TZMachineClass {
     uint32_t sram_addr_width; /* SRAM_ADDR_WIDTH setting for SSE */
     const RAMInfo *raminfo;
     const char *armsse_type;
+    uint32_t boot_ram_size; /* size of ram at address 0; 0 == find in raminfo */
 };
 
 struct MPS2TZMachineState {
@@ -761,6 +762,14 @@ static uint32_t boot_ram_size(MPS2TZMachineState *mms)
     const RAMInfo *p;
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
 
+    /*
+     * Use a per-board specification (for when the boot RAM is in
+     * the SSE and so doesn't have a RAMInfo list entry)
+     */
+    if (mmc->boot_ram_size) {
+        return mmc->boot_ram_size;
+    }
+
     for (p = mmc->raminfo; p->name; p++) {
         if (p->base == boot_mem_base(mms)) {
             return p->size;
@@ -1268,6 +1277,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
     mmc->sram_addr_width = 15;
     mmc->raminfo = an505_raminfo;
     mmc->armsse_type = TYPE_IOTKIT;
+    mmc->boot_ram_size = 0;
     mps2tz_set_default_ram_info(mmc);
 }
 
@@ -1296,6 +1306,7 @@ static void mps2tz_an521_class_init(ObjectClass *oc, void *data)
     mmc->sram_addr_width = 15;
     mmc->raminfo = an505_raminfo; /* AN521 is the same as AN505 here */
     mmc->armsse_type = TYPE_SSE200;
+    mmc->boot_ram_size = 0;
     mps2tz_set_default_ram_info(mmc);
 }
 
@@ -1324,6 +1335,7 @@ static void mps3tz_an524_class_init(ObjectClass *oc, void *data)
     mmc->sram_addr_width = 15;
     mmc->raminfo = an524_raminfo;
     mmc->armsse_type = TYPE_SSE200;
+    mmc->boot_ram_size = 0;
     mps2tz_set_default_ram_info(mmc);
 
     object_class_property_add_str(oc, "remap", mps2_get_remap, mps2_set_remap);
@@ -1357,6 +1369,7 @@ static void mps3tz_an547_class_init(ObjectClass *oc, void *data)
     mmc->sram_addr_width = 21;
     mmc->raminfo = an547_raminfo;
     mmc->armsse_type = TYPE_SSE300;
+    mmc->boot_ram_size = 512 * KiB;
     mps2tz_set_default_ram_info(mmc);
 }
 
-- 
2.20.1

Re: [PATCH 5/6] hw/arm/mps2-tz: Allow board to specify a boot RAM size

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> Currently we model the ITCM in the AN547's RAMInfo list. This is incorrect
> because this RAM is really a part of the SSE-300. We can't just delete
> it from the RAMInfo list, though, because this would make boot_ram_size()
> assert because it wouldn't be able to find an entry in the list covering
> guest address 0.
> 
> Allow a board to specify a boot RAM size manually if it doesn't have
> any RAM itself at address 0 and is relying on the SSE for that, and
> set the correct value for the AN547. The other boards can continue
> to use the "look it up from the RAMInfo list" logic.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/mps2-tz.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 6/6] hw/arm: Model TCMs in the SSE-300, not the AN547

[Peter Maydell]

The SSE-300 has an ITCM at 0x0000_0000 and a DTCM at 0x2000_0000.
Currently we model these in the AN547 board, but this is conceptually
wrong, because they are a part of the SSE-300 itself. Move the
modelling of the TCMs out of mps2-tz.c into sse300.c.

This has no guest-visible effects.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/armsse.h |  2 ++
 hw/arm/armsse.c         | 19 +++++++++++++++++++
 hw/arm/mps2-tz.c        | 12 ------------
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/hw/arm/armsse.h b/include/hw/arm/armsse.h
index 36592be62c5..9648e7a4193 100644
--- a/include/hw/arm/armsse.h
+++ b/include/hw/arm/armsse.h
@@ -198,6 +198,8 @@ struct ARMSSE {
     MemoryRegion alias2;
     MemoryRegion alias3[SSE_MAX_CPUS];
     MemoryRegion sram[MAX_SRAM_BANKS];
+    MemoryRegion itcm;
+    MemoryRegion dtcm;
 
     qemu_irq *exp_irqs[SSE_MAX_CPUS];
     qemu_irq ppc0_irq;
diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index be5aa1f113a..a1456cb0f42 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -13,6 +13,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/bitops.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "trace.h"
 #include "hw/sysbus.h"
@@ -70,6 +71,7 @@ struct ARMSSEInfo {
     bool has_cpuid;
     bool has_cpu_pwrctrl;
     bool has_sse_counter;
+    bool has_tcms;
     Property *props;
     const ARMSSEDeviceInfo *devinfo;
     const bool *irq_is_common;
@@ -516,6 +518,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .has_cpuid = false,
         .has_cpu_pwrctrl = false,
         .has_sse_counter = false,
+        .has_tcms = false,
         .props = iotkit_properties,
         .devinfo = iotkit_devices,
         .irq_is_common = sse200_irq_is_common,
@@ -536,6 +539,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .has_cpuid = true,
         .has_cpu_pwrctrl = false,
         .has_sse_counter = false,
+        .has_tcms = false,
         .props = sse200_properties,
         .devinfo = sse200_devices,
         .irq_is_common = sse200_irq_is_common,
@@ -556,6 +560,7 @@ static const ARMSSEInfo armsse_variants[] = {
         .has_cpuid = true,
         .has_cpu_pwrctrl = true,
         .has_sse_counter = true,
+        .has_tcms = true,
         .props = sse300_properties,
         .devinfo = sse300_devices,
         .irq_is_common = sse300_irq_is_common,
@@ -1214,6 +1219,20 @@ static void armsse_realize(DeviceState *dev, Error **errp)
                                     sysbus_mmio_get_region(sbd, 1));
     }
 
+    if (info->has_tcms) {
+        /* The SSE-300 has an ITCM at 0x0000_0000 and a DTCM at 0x2000_0000 */
+        memory_region_init_ram(&s->itcm, NULL, "sse300-itcm", 512 * KiB, errp);
+        if (*errp) {
+            return;
+        }
+        memory_region_init_ram(&s->dtcm, NULL, "sse300-dtcm", 512 * KiB, errp);
+        if (*errp) {
+            return;
+        }
+        memory_region_add_subregion(&s->container, 0x00000000, &s->itcm);
+        memory_region_add_subregion(&s->container, 0x20000000, &s->dtcm);
+    }
+
     /* Devices behind APB PPC0:
      *   0x40000000: timer0
      *   0x40001000: timer1
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 8d921afec14..e23830f4b7d 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -265,23 +265,11 @@ static const RAMInfo an524_raminfo[] = { {
 };
 
 static const RAMInfo an547_raminfo[] = { {
-        .name = "itcm",
-        .base = 0x00000000,
-        .size = 512 * KiB,
-        .mpc = -1,
-        .mrindex = 0,
-    }, {
         .name = "sram",
         .base = 0x01000000,
         .size = 2 * MiB,
         .mpc = 0,
         .mrindex = 1,
-    }, {
-        .name = "dtcm",
-        .base = 0x20000000,
-        .size = 4 * 128 * KiB,
-        .mpc = -1,
-        .mrindex = 2,
     }, {
         .name = "sram 2",
         .base = 0x21000000,
-- 
2.20.1

Re: [PATCH 6/6] hw/arm: Model TCMs in the SSE-300, not the AN547

[Richard Henderson]

On 5/10/21 12:08 PM, Peter Maydell wrote:
> The SSE-300 has an ITCM at 0x0000_0000 and a DTCM at 0x2000_0000.
> Currently we model these in the AN547 board, but this is conceptually
> wrong, because they are a part of the SSE-300 itself. Move the
> modelling of the TCMs out of mps2-tz.c into sse300.c.
> 
> This has no guest-visible effects.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/arm/armsse.h |  2 ++
>   hw/arm/armsse.c         | 19 +++++++++++++++++++
>   hw/arm/mps2-tz.c        | 12 ------------
>   3 files changed, 21 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[Philippe Mathieu-Daudé]

Hi Peter,

On 5/10/21 9:08 PM, Peter Maydell wrote:
> This patchset fixes some bugs in how we were modelling the
> TCMs and the SRAM in the SSE-300 which were preventing
> Arm TF-M from booting on our AN547 model; there are also
> some fixes to things I noticed while I was in the code.
> 
> The specific bugs preventing boot were:
>  * SRAM_ADDR_WIDTH for the AN547 is 21, not 15, so the SRAM
>    area was too small
>  * we were putting the SRAMs at the wrong address (0x2100_0000
>    for SSE-300, not 0x2000_0000 as for SSE-200)

How can we test it?

> The other stuff I've fixed is:
>  * we were modelling the SRAM in the AN524 both in the SSE
>    and in the board model (harmlessly, as the board-model
>    memory was just always shadowed in the memory map and
>    unreachable)
>  * we were modelling the TCMs in the AN547 board model,
>    which is conceptually wrong because in hardware they're
>    part of the SSE-300. No guest-visible change, but it will
>    avoid problems if/when we add another SSE-300 board model
> 
> thanks
> -- PMM
> 
> Peter Maydell (6):
>   hw/arm/mps2-tz: Don't duplicate modelling of SRAM in AN524
>   hw/arm/mps2-tz: Make SRAM_ADDR_WIDTH board-specific
>   hw/arm/armsse.c: Correct modelling of SSE-300 internal SRAMs
>   hw/arm/armsse: Convert armsse_realize() to use ERRP_GUARD
>   hw/arm/mps2-tz: Allow board to specify a boot RAM size
>   hw/arm: Model TCMs in the SSE-300, not the AN547
> 
>  include/hw/arm/armsse.h |  2 ++
>  hw/arm/armsse.c         | 35 +++++++++++++++++++++++++++++------
>  hw/arm/mps2-tz.c        | 39 ++++++++++++++++++++-------------------
>  3 files changed, 51 insertions(+), 25 deletions(-)
> 

Re: [PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[Peter Maydell]

On Mon, 10 May 2021 at 20:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 5/10/21 9:08 PM, Peter Maydell wrote:
> > This patchset fixes some bugs in how we were modelling the
> > TCMs and the SRAM in the SSE-300 which were preventing
> > Arm TF-M from booting on our AN547 model; there are also
> > some fixes to things I noticed while I was in the code.
> >
> > The specific bugs preventing boot were:
> >  * SRAM_ADDR_WIDTH for the AN547 is 21, not 15, so the SRAM
> >    area was too small
> >  * we were putting the SRAMs at the wrong address (0x2100_0000
> >    for SSE-300, not 0x2000_0000 as for SSE-200)
>
> How can we test it?

I tested using a binary that Devaraj provided me. As usual,
I don't know if there's anything that would be a sufficiently
"publicly hosted, with associated source for licensing purposes"
binary that we could put into tests/acceptance.

-- PMM

Re: [PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[Philippe Mathieu-Daudé]

On 5/10/21 9:22 PM, Peter Maydell wrote:
> On Mon, 10 May 2021 at 20:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Peter,
>>
>> On 5/10/21 9:08 PM, Peter Maydell wrote:
>>> This patchset fixes some bugs in how we were modelling the
>>> TCMs and the SRAM in the SSE-300 which were preventing
>>> Arm TF-M from booting on our AN547 model; there are also
>>> some fixes to things I noticed while I was in the code.
>>>
>>> The specific bugs preventing boot were:
>>>  * SRAM_ADDR_WIDTH for the AN547 is 21, not 15, so the SRAM
>>>    area was too small
>>>  * we were putting the SRAMs at the wrong address (0x2100_0000
>>>    for SSE-300, not 0x2000_0000 as for SSE-200)
>>
>> How can we test it?
> 
> I tested using a binary that Devaraj provided me. As usual,
> I don't know if there's anything that would be a sufficiently
> "publicly hosted, with associated source for licensing purposes"
> binary that we could put into tests/acceptance.

OK, thank you.

Phil.

Re: [PATCH 0/6] hw/arm: Fix modelling of SSE-300 TCMs and SRAM

[Peter Maydell]

On Mon, 10 May 2021 at 20:08, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes some bugs in how we were modelling the
> TCMs and the SRAM in the SSE-300 which were preventing
> Arm TF-M from booting on our AN547 model; there are also
> some fixes to things I noticed while I was in the code.
>
> The specific bugs preventing boot were:
>  * SRAM_ADDR_WIDTH for the AN547 is 21, not 15, so the SRAM
>    area was too small
>  * we were putting the SRAMs at the wrong address (0x2100_0000
>    for SSE-300, not 0x2000_0000 as for SSE-200)
>
> The other stuff I've fixed is:
>  * we were modelling the SRAM in the AN524 both in the SSE
>    and in the board model (harmlessly, as the board-model
>    memory was just always shadowed in the memory map and
>    unreachable)
>  * we were modelling the TCMs in the AN547 board model,
>    which is conceptually wrong because in hardware they're
>    part of the SSE-300. No guest-visible change, but it will
>    avoid problems if/when we add another SSE-300 board model
>
> thanks
> -- PMM

Ping for code review, please?

thanks
-- PMM