[PATCH 0/3] mps3-an524: support memory remapping

[PATCH 0/3] mps3-an524: support memory remapping

[Peter Maydell]

The AN524 FPGA image supports two memory maps, which differ
in where the QSPI and BRAM are. In the default map, the BRAM
is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
map, they are the other way around.

In hardware, the initial mapping can be selected by the user
by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
in the board configuration file. The guest can also dynamically
change the mapping via the SCC CFG_REG0 register.

This patchset adds support for the feature to QEMU's model;
the user-sets-the-initial-mapping part is a new machine property
which can be set with "-M remap=QSPI".

This is needed for some guest images -- for instance the
Arm TF-M binaries -- which assume they have the QSPI layout.

Based-on: 20210409150527.15053-1-peter.maydell@linaro.org
("mps3-an524: Fix MPC setting for SRAM block")
though any conflict/dependency would be minor and purely textual.

thanks
-- PMM

Peter Maydell (3):
  hw/misc/mps2-scc: Add "QEMU interface" comment
  hw/misc/mps2-scc: Support using CFG0 bit 0 for remapping
  hw/arm/mps2-tz: Implement AN524 memory remapping via machine property

 docs/system/arm/mps2.rst   |  10 ++++
 include/hw/misc/mps2-scc.h |  21 ++++++++
 hw/arm/mps2-tz.c           | 106 ++++++++++++++++++++++++++++++++++++-
 hw/misc/mps2-scc.c         |  13 +++--
 4 files changed, 146 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH 1/3] hw/misc/mps2-scc: Add "QEMU interface" comment

[Peter Maydell]

The MPS2 SCC device doesn't have any documentation of its properties;
add a "QEMU interface" format comment describing them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/misc/mps2-scc.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
index 49d070616aa..ea261ea30d6 100644
--- a/include/hw/misc/mps2-scc.h
+++ b/include/hw/misc/mps2-scc.h
@@ -9,6 +9,18 @@
  *  (at your option) any later version.
  */
 
+/*
+ * This is a model of the Serial Communication Controller (SCC)
+ * block found in most MPS FPGA images.
+ *
+ * QEMU interface:
+ *  + sysbus MMIO region 0: the register bank
+ *  + QOM property "scc-cfg4": value of the read-only CFG4 register
+ *  + QOM property "scc-aid": value of the read-only SCC_AID register
+ *  + QOM property "scc-id": value of the read-only SCC_ID register
+ *  + QOM property array "oscclk": reset values of the OSCCLK registers
+ *    (which are accessed via the SYS_CFG channel provided by this device)
+ */
 #ifndef MPS2_SCC_H
 #define MPS2_SCC_H
 
-- 
2.20.1

[PATCH 2/3] hw/misc/mps2-scc: Support using CFG0 bit 0 for remapping

[Peter Maydell]

On some boards, SCC config register CFG0 bit 0 controls whether
parts of the board memory map are remapped. Support this with:
 * a device property scc-cfg0 so the board can specify the
   initial value of the CFG0 register
 * an outbound GPIO line which tracks bit 0 and which the board
   can wire up to provide the remapping

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/misc/mps2-scc.h |  9 +++++++++
 hw/misc/mps2-scc.c         | 13 ++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/hw/misc/mps2-scc.h b/include/hw/misc/mps2-scc.h
index ea261ea30d6..3b2d13ac9c3 100644
--- a/include/hw/misc/mps2-scc.h
+++ b/include/hw/misc/mps2-scc.h
@@ -18,8 +18,14 @@
  *  + QOM property "scc-cfg4": value of the read-only CFG4 register
  *  + QOM property "scc-aid": value of the read-only SCC_AID register
  *  + QOM property "scc-id": value of the read-only SCC_ID register
+ *  + QOM property "scc-cfg0": reset value of the CFG0 register
  *  + QOM property array "oscclk": reset values of the OSCCLK registers
  *    (which are accessed via the SYS_CFG channel provided by this device)
+ *  + named GPIO output "remap": this tracks the value of CFG0 register
+ *    bit 0. Boards where this bit controls memory remapping should
+ *    connect this GPIO line to a function performing that mapping.
+ *    Boards where bit 0 has no special function should leave the GPIO
+ *    output disconnected.
  */
 #ifndef MPS2_SCC_H
 #define MPS2_SCC_H
@@ -55,6 +61,9 @@ struct MPS2SCC {
     uint32_t num_oscclk;
     uint32_t *oscclk;
     uint32_t *oscclk_reset;
+    uint32_t cfg0_reset;
+
+    qemu_irq remap;
 };
 
 #endif
diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index c56aca86ad5..b3b42a792cd 100644
--- a/hw/misc/mps2-scc.c
+++ b/hw/misc/mps2-scc.c
@@ -23,6 +23,7 @@
 #include "qemu/bitops.h"
 #include "trace.h"
 #include "hw/sysbus.h"
+#include "hw/irq.h"
 #include "migration/vmstate.h"
 #include "hw/registerfields.h"
 #include "hw/misc/mps2-scc.h"
@@ -186,10 +187,13 @@ static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
     switch (offset) {
     case A_CFG0:
         /*
-         * TODO on some boards bit 0 controls RAM remapping;
-         * on others bit 1 is CPU_WAIT.
+         * On some boards bit 0 controls board-specific remapping;
+         * we always reflect bit 0 in the 'remap' GPIO output line,
+         * and let the board wire it up or not as it chooses.
+         * TODO on some boards bit 1 is CPU_WAIT.
          */
         s->cfg0 = value;
+        qemu_set_irq(s->remap, s->cfg0 & 1);
         break;
     case A_CFG1:
         s->cfg1 = value;
@@ -283,7 +287,7 @@ static void mps2_scc_reset(DeviceState *dev)
     int i;
 
     trace_mps2_scc_reset();
-    s->cfg0 = 0;
+    s->cfg0 = s->cfg0_reset;
     s->cfg1 = 0;
     s->cfg2 = 0;
     s->cfg5 = 0;
@@ -308,6 +312,7 @@ static void mps2_scc_init(Object *obj)
 
     memory_region_init_io(&s->iomem, obj, &mps2_scc_ops, s, "mps2-scc", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->remap, "remap", 1);
 }
 
 static void mps2_scc_realize(DeviceState *dev, Error **errp)
@@ -353,6 +358,8 @@ static Property mps2_scc_properties[] = {
     DEFINE_PROP_UINT32("scc-cfg4", MPS2SCC, cfg4, 0),
     DEFINE_PROP_UINT32("scc-aid", MPS2SCC, aid, 0),
     DEFINE_PROP_UINT32("scc-id", MPS2SCC, id, 0),
+    /* Reset value for CFG0 register */
+    DEFINE_PROP_UINT32("scc-cfg0", MPS2SCC, cfg0_reset, 0),
     /*
      * These are the initial settings for the source clocks on the board.
      * In hardware they can be configured via a config file read by the
-- 
2.20.1

[PATCH 3/3] hw/arm/mps2-tz: Implement AN524 memory remapping via machine property

[Peter Maydell]

The AN524 FPGA image supports two memory maps, which differ
in where the QSPI and BRAM are. In the default map, the BRAM
is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
map, they are the other way around.

In hardware, the initial mapping can be selected by the user
by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
in the board configuration file. The guest can also dynamically
change the mapping via the SCC CFG_REG0 register.

Implement this functionality for QEMU, using a machine property
"remap" with valid values "BRAM" and "QSPI" to allow the user to set
the initial mapping, in the same way they can on the FPGA, and
wiring up the bit from the SCC register to also switch the mapping.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/system/arm/mps2.rst |  10 ++++
 hw/arm/mps2-tz.c         | 106 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/mps2.rst b/docs/system/arm/mps2.rst
index f83b1517871..8a75beb3a08 100644
--- a/docs/system/arm/mps2.rst
+++ b/docs/system/arm/mps2.rst
@@ -45,3 +45,13 @@ Differences between QEMU and real hardware:
   flash, but only as simple ROM, so attempting to rewrite the flash
   from the guest will fail
 - QEMU does not model the USB controller in MPS3 boards
+
+Machine-specific options
+""""""""""""""""""""""""
+
+The following machine-specific options are supported:
+
+remap
+  Supported for ``mps3-an524`` only.
+  Set ``BRAM``/``QSPI`` to select the initial memory mapping. The
+  default is ``BRAM``.
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 25016e464d9..e9806779326 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -55,6 +55,7 @@
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/reset.h"
 #include "hw/misc/unimp.h"
 #include "hw/char/cmsdk-apb-uart.h"
 #include "hw/timer/cmsdk-apb-timer.h"
@@ -72,6 +73,7 @@
 #include "hw/core/split-irq.h"
 #include "hw/qdev-clock.h"
 #include "qom/object.h"
+#include "hw/irq.h"
 
 #define MPS2TZ_NUMIRQ_MAX 96
 #define MPS2TZ_RAM_MAX 5
@@ -153,6 +155,9 @@ struct MPS2TZMachineState {
     SplitIRQ cpu_irq_splitter[MPS2TZ_NUMIRQ_MAX];
     Clock *sysclk;
     Clock *s32kclk;
+
+    int remap;
+    qemu_irq remap_irq;
 };
 
 #define TYPE_MPS2TZ_MACHINE "mps2tz"
@@ -228,6 +233,10 @@ static const RAMInfo an505_raminfo[] = { {
     },
 };
 
+/*
+ * Note that the addresses and MPC numbering here should match up
+ * with those used in remap_memory(), which can swap the BRAM and QSPI.
+ */
 static const RAMInfo an524_raminfo[] = { {
         .name = "bram",
         .base = 0x00000000,
@@ -457,6 +466,7 @@ static MemoryRegion *make_scc(MPS2TZMachineState *mms, void *opaque,
 
     object_initialize_child(OBJECT(mms), "scc", scc, TYPE_MPS2_SCC);
     sccdev = DEVICE(scc);
+    qdev_prop_set_uint32(sccdev, "scc-cfg0", mms->remap);
     qdev_prop_set_uint32(sccdev, "scc-cfg4", 0x2);
     qdev_prop_set_uint32(sccdev, "scc-aid", 0x00200008);
     qdev_prop_set_uint32(sccdev, "scc-id", mmc->scc_id);
@@ -573,6 +583,50 @@ static MemoryRegion *make_mpc(MPS2TZMachineState *mms, void *opaque,
     return sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 0);
 }
 
+static hwaddr boot_mem_base(MPS2TZMachineState *mms)
+{
+    /*
+     * Return the canonical address of the block which will be mapped
+     * at address 0x0 (i.e. where the vector table is).
+     * This is usually 0, but if the AN524 alternate memory map is
+     * enabled it will be the base address of the QSPI block.
+     */
+    return mms->remap ? 0x28000000 : 0;
+}
+
+static void remap_memory(MPS2TZMachineState *mms, int map)
+{
+    /*
+     * Remap the memory for the AN524. 'map' is the value of
+     * SCC CFG_REG0 bit 0, i.e. 0 for the default map and 1
+     * for the "option 1" mapping where QSPI is at address 0.
+     *
+     * Effectively we need to swap around the "upstream" ends of
+     * MPC 0 and MPC 1.
+     */
+    MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
+    int i;
+
+    if (mmc->fpga_type != FPGA_AN524) {
+        return;
+    }
+
+    for (i = 0; i < 2; i++) {
+        TZMPC *mpc = &mms->mpc[i];
+        MemoryRegion *upstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 1);
+        hwaddr addr = (i ^ map) ? 0x28000000 : 0;
+
+        memory_region_set_address(upstream, addr);
+    }
+}
+
+static void remap_irq_fn(void *opaque, int n, int level)
+{
+    MPS2TZMachineState *mms = opaque;
+
+    remap_memory(mms, level);
+}
+
 static MemoryRegion *make_dma(MPS2TZMachineState *mms, void *opaque,
                               const char *name, hwaddr size,
                               const int *irqs)
@@ -711,7 +765,7 @@ static uint32_t boot_ram_size(MPS2TZMachineState *mms)
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
 
     for (p = mmc->raminfo; p->name; p++) {
-        if (p->base == 0) {
+        if (p->base == boot_mem_base(mms)) {
             return p->size;
         }
     }
@@ -1095,6 +1149,16 @@ static void mps2tz_common_init(MachineState *machine)
 
     create_non_mpc_ram(mms);
 
+    if (mmc->fpga_type == FPGA_AN524) {
+        /*
+         * Connect the line from the SCC so that we can remap when the
+         * guest updates that register.
+         */
+        mms->remap_irq = qemu_allocate_irq(remap_irq_fn, mms, 0);
+        qdev_connect_gpio_out_named(DEVICE(&mms->scc), "remap", 0,
+                                    mms->remap_irq);
+    }
+
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
                        boot_ram_size(mms));
 }
@@ -1117,12 +1181,47 @@ static void mps2_tz_idau_check(IDAUInterface *ii, uint32_t address,
     *iregion = region;
 }
 
+static char *mps2_get_remap(Object *obj, Error **errp)
+{
+    MPS2TZMachineState *mms = MPS2TZ_MACHINE(obj);
+    const char *val = mms->remap ? "QSPI" : "BRAM";
+    return g_strdup(val);
+}
+
+static void mps2_set_remap(Object *obj, const char *value, Error **errp)
+{
+    MPS2TZMachineState *mms = MPS2TZ_MACHINE(obj);
+
+    if (!strcmp(value, "BRAM")) {
+        mms->remap = 0;
+    } else if (!strcmp(value, "QSPI")) {
+        mms->remap = 1;
+    } else {
+        error_setg(errp, "Invalid remap value");
+        error_append_hint(errp, "Valid values are BRAM and QSPI.\n");
+    }
+}
+
+static void mps2_machine_reset(MachineState *machine)
+{
+    MPS2TZMachineState *mms = MPS2TZ_MACHINE(machine);
+
+    /*
+     * Set the initial memory mapping before triggering the reset of
+     * the rest of the system, so that the guest image loader and CPU
+     * reset see the correct mapping.
+     */
+    remap_memory(mms, mms->remap);
+    qemu_devices_reset();
+}
+
 static void mps2tz_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     IDAUInterfaceClass *iic = IDAU_INTERFACE_CLASS(oc);
 
     mc->init = mps2tz_common_init;
+    mc->reset = mps2_machine_reset;
     iic->check = mps2_tz_idau_check;
 }
 
@@ -1225,6 +1324,11 @@ static void mps3tz_an524_class_init(ObjectClass *oc, void *data)
     mmc->raminfo = an524_raminfo;
     mmc->armsse_type = TYPE_SSE200;
     mps2tz_set_default_ram_info(mmc);
+
+    object_class_property_add_str(oc, "remap", mps2_get_remap, mps2_set_remap);
+    object_class_property_set_description(oc, "remap",
+                                          "Set memory mapping. Valid values "
+                                          "are BRAM (default) and QSPI.");
 }
 
 static void mps3tz_an547_class_init(ObjectClass *oc, void *data)
-- 
2.20.1

Re: [PATCH 0/3] mps3-an524: support memory remapping

[Philippe Mathieu-Daudé]

Hi Peter,

On 4/12/21 3:43 PM, Peter Maydell wrote:
> The AN524 FPGA image supports two memory maps, which differ
> in where the QSPI and BRAM are. In the default map, the BRAM
> is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
> map, they are the other way around.
> 
> In hardware, the initial mapping can be selected by the user
> by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
> in the board configuration file. The guest can also dynamically
> change the mapping via the SCC CFG_REG0 register.
> 
> This patchset adds support for the feature to QEMU's model;
> the user-sets-the-initial-mapping part is a new machine property
> which can be set with "-M remap=QSPI".
> 
> This is needed for some guest images -- for instance the
> Arm TF-M binaries -- which assume they have the QSPI layout.

I tend to see machine property set on the command line similar
to hardware wire jumpers, externally set (by an operator having
access to the hardware, not guest code).

Here the remap behavior you described is triggered by the guest.
Usually this is done by a bootloader code before running the
guest code.
Couldn't we have the same result using a booloader (like -bios
cmd line option) rather than modifying internal peripheral state?

I'm worried anyone wants to add its own machine property to simplify
device modelling, but maybe this is the correct way to go...

Regards,

Phil.

Re: [PATCH 0/3] mps3-an524: support memory remapping

[Peter Maydell]

On Mon, 12 Apr 2021 at 15:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 4/12/21 3:43 PM, Peter Maydell wrote:
> > The AN524 FPGA image supports two memory maps, which differ
> > in where the QSPI and BRAM are. In the default map, the BRAM
> > is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
> > map, they are the other way around.
> >
> > In hardware, the initial mapping can be selected by the user
> > by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
> > in the board configuration file. The guest can also dynamically
> > change the mapping via the SCC CFG_REG0 register.
> >
> > This patchset adds support for the feature to QEMU's model;
> > the user-sets-the-initial-mapping part is a new machine property
> > which can be set with "-M remap=QSPI".
> >
> > This is needed for some guest images -- for instance the
> > Arm TF-M binaries -- which assume they have the QSPI layout.
>
> I tend to see machine property set on the command line similar
> to hardware wire jumpers, externally set (by an operator having
> access to the hardware, not guest code).
>
> Here the remap behavior you described is triggered by the guest.
> Usually this is done by a bootloader code before running the
> guest code.
> Couldn't we have the same result using a booloader (like -bios
> cmd line option) rather than modifying internal peripheral state?

In the real hardware, the handling of the board configuration
file is done by the "Motherboard Configuration Controller", which
is an entirely separate microcontroller on the dev board but outside
the FPGA, and which is responsible for things like loading image
files off the SD card and writing them to memory, setting a bunch
of initial configuration including the remap setting but also
things like setting the oscillators to the values that this
particular FPGA image needs. It's also what makes the board
appear to a connected computer as a USB mass storage device so
you can update the SD card files via USB cable rather than doing
lots of plugging and unplugging, and it is what loads the FPGA
image off SD card and into the FPGA in the first place.

QEMU is never going to implement the MCC as a real emulated
guest CPU; instead our models hard-code some of the things it
does. I think that a machine property (a thing set externally
to the guest CPU and valid before any guest CPU code executes)
is a reasonable way to implement the remap setting, which from
the point of view of the CPU inside the FPGA is a thing set
externally and valid before any guest CPU code executes.

thanks
-- PMM

Re: [PATCH 0/3] mps3-an524: support memory remapping

[Richard Henderson]

On 4/12/21 6:43 AM, Peter Maydell wrote:
> Peter Maydell (3):
>    hw/misc/mps2-scc: Add "QEMU interface" comment
>    hw/misc/mps2-scc: Support using CFG0 bit 0 for remapping
>    hw/arm/mps2-tz: Implement AN524 memory remapping via machine property

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

r~

Re: [PATCH 0/3] mps3-an524: support memory remapping

[Philippe Mathieu-Daudé]

Hi Peter,

On 4/12/21 4:48 PM, Peter Maydell wrote:
> On Mon, 12 Apr 2021 at 15:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/12/21 3:43 PM, Peter Maydell wrote:
>>> The AN524 FPGA image supports two memory maps, which differ
>>> in where the QSPI and BRAM are. In the default map, the BRAM
>>> is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
>>> map, they are the other way around.
>>>
>>> In hardware, the initial mapping can be selected by the user
>>> by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
>>> in the board configuration file. The guest can also dynamically
>>> change the mapping via the SCC CFG_REG0 register.
>>>
>>> This patchset adds support for the feature to QEMU's model;
>>> the user-sets-the-initial-mapping part is a new machine property
>>> which can be set with "-M remap=QSPI".
>>>
>>> This is needed for some guest images -- for instance the
>>> Arm TF-M binaries -- which assume they have the QSPI layout.
>>
>> I tend to see machine property set on the command line similar
>> to hardware wire jumpers, externally set (by an operator having
>> access to the hardware, not guest code).
>>
>> Here the remap behavior you described is triggered by the guest.
>> Usually this is done by a bootloader code before running the
>> guest code.
>> Couldn't we have the same result using a booloader (like -bios
>> cmd line option) rather than modifying internal peripheral state?
> 

(

> In the real hardware, the handling of the board configuration
> file is done by the "Motherboard Configuration Controller", which
> is an entirely separate microcontroller on the dev board but outside
> the FPGA, and which is responsible for things like loading image
> files off the SD card and writing them to memory, setting a bunch
> of initial configuration including the remap setting but also
> things like setting the oscillators to the values that this
> particular FPGA image needs. It's also what makes the board
> appear to a connected computer as a USB mass storage device so
> you can update the SD card files via USB cable rather than doing
> lots of plugging and unplugging, and it is what loads the FPGA
> image off SD card and into the FPGA in the first place.

) [*]

> QEMU is never going to implement the MCC as a real emulated
> guest CPU; instead our models hard-code some of the things it
> does. I think that a machine property (a thing set externally
> to the guest CPU and valid before any guest CPU code executes)
> is a reasonable way to implement the remap setting, which from
> the point of view of the CPU inside the FPGA is a thing set
> externally and valid before any guest CPU code executes.

OK now I understand the picture, the MCC is external. In that case
the machine property is a clean way to address that.

Could you add the first paragraph of your answer ([*]) in patch 3
description (before the current comment) to make it clearer?

Thanks for the clarification,

Phil.

Re: [PATCH 2/3] hw/misc/mps2-scc: Support using CFG0 bit 0 for remapping

[Philippe Mathieu-Daudé]

On 4/12/21 3:43 PM, Peter Maydell wrote:
> On some boards, SCC config register CFG0 bit 0 controls whether
> parts of the board memory map are remapped. Support this with:
>  * a device property scc-cfg0 so the board can specify the
>    initial value of the CFG0 register
>  * an outbound GPIO line which tracks bit 0 and which the board
>    can wire up to provide the remapping
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/misc/mps2-scc.h |  9 +++++++++
>  hw/misc/mps2-scc.c         | 13 ++++++++++---
>  2 files changed, 19 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 0/3] mps3-an524: support memory remapping

[Philippe Mathieu-Daudé]

On 4/13/21 6:29 PM, Philippe Mathieu-Daudé wrote:> On 4/12/21 4:48 PM,
Peter Maydell wrote:
>> On Mon, 12 Apr 2021 at 15:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> On 4/12/21 3:43 PM, Peter Maydell wrote:
>>>> The AN524 FPGA image supports two memory maps, which differ
>>>> in where the QSPI and BRAM are. In the default map, the BRAM
>>>> is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
>>>> map, they are the other way around.
>>>>
>>>> In hardware, the initial mapping can be selected by the user
>>>> by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
>>>> in the board configuration file. The guest can also dynamically
>>>> change the mapping via the SCC CFG_REG0 register.
>>>>
>>>> This patchset adds support for the feature to QEMU's model;
>>>> the user-sets-the-initial-mapping part is a new machine property
>>>> which can be set with "-M remap=QSPI".
>>>>
>>>> This is needed for some guest images -- for instance the
>>>> Arm TF-M binaries -- which assume they have the QSPI layout.
>>>
>>> I tend to see machine property set on the command line similar
>>> to hardware wire jumpers, externally set (by an operator having
>>> access to the hardware, not guest code).
>>>
>>> Here the remap behavior you described is triggered by the guest.
>>> Usually this is done by a bootloader code before running the
>>> guest code.
>>> Couldn't we have the same result using a booloader (like -bios
>>> cmd line option) rather than modifying internal peripheral state?
>>
> 
> (
> 
>> In the real hardware, the handling of the board configuration
>> file is done by the "Motherboard Configuration Controller", which
>> is an entirely separate microcontroller on the dev board but outside
>> the FPGA, and which is responsible for things like loading image
>> files off the SD card and writing them to memory, setting a bunch
>> of initial configuration including the remap setting but also
>> things like setting the oscillators to the values that this
>> particular FPGA image needs. It's also what makes the board
>> appear to a connected computer as a USB mass storage device so
>> you can update the SD card files via USB cable rather than doing
>> lots of plugging and unplugging, and it is what loads the FPGA
>> image off SD card and into the FPGA in the first place.
> 
> ) [*]
> 
>> QEMU is never going to implement the MCC as a real emulated
>> guest CPU; instead our models hard-code some of the things it
>> does. I think that a machine property (a thing set externally
>> to the guest CPU and valid before any guest CPU code executes)
>> is a reasonable way to implement the remap setting, which from
>> the point of view of the CPU inside the FPGA is a thing set
>> externally and valid before any guest CPU code executes.
> 
> OK now I understand the picture, the MCC is external. In that case
> the machine property is a clean way to address that.
> 
> Could you add the first paragraph of your answer ([*]) in patch 3
> description (before the current comment) to make it clearer?

(In case you agree, no need to respin).

Re: [PATCH 3/3] hw/arm/mps2-tz: Implement AN524 memory remapping via machine property

[Philippe Mathieu-Daudé]

On 4/12/21 3:43 PM, Peter Maydell wrote:
> The AN524 FPGA image supports two memory maps, which differ
> in where the QSPI and BRAM are. In the default map, the BRAM
> is at 0x0000_0000, and the QSPI at 0x2800_0000. In the second
> map, they are the other way around.
> 
> In hardware, the initial mapping can be selected by the user
> by writing either "REMAP: BRAM" (the default) or "REMAP: QSPI"
> in the board configuration file. The guest can also dynamically
> change the mapping via the SCC CFG_REG0 register.
> 
> Implement this functionality for QEMU, using a machine property
> "remap" with valid values "BRAM" and "QSPI" to allow the user to set
> the initial mapping, in the same way they can on the FPGA, and
> wiring up the bit from the SCC register to also switch the mapping.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/system/arm/mps2.rst |  10 ++++
>  hw/arm/mps2-tz.c         | 106 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/mps2.rst b/docs/system/arm/mps2.rst
> index f83b1517871..8a75beb3a08 100644
> --- a/docs/system/arm/mps2.rst
> +++ b/docs/system/arm/mps2.rst
> @@ -45,3 +45,13 @@ Differences between QEMU and real hardware:
>    flash, but only as simple ROM, so attempting to rewrite the flash
>    from the guest will fail
>  - QEMU does not model the USB controller in MPS3 boards
> +
> +Machine-specific options
> +""""""""""""""""""""""""
> +
> +The following machine-specific options are supported:
> +
> +remap
> +  Supported for ``mps3-an524`` only.
> +  Set ``BRAM``/``QSPI`` to select the initial memory mapping. The
> +  default is ``BRAM``.
> diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
> index 25016e464d9..e9806779326 100644
> --- a/hw/arm/mps2-tz.c
> +++ b/hw/arm/mps2-tz.c
> @@ -55,6 +55,7 @@
>  #include "hw/boards.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/reset.h"
>  #include "hw/misc/unimp.h"
>  #include "hw/char/cmsdk-apb-uart.h"
>  #include "hw/timer/cmsdk-apb-timer.h"
> @@ -72,6 +73,7 @@
>  #include "hw/core/split-irq.h"
>  #include "hw/qdev-clock.h"
>  #include "qom/object.h"
> +#include "hw/irq.h"
>  
>  #define MPS2TZ_NUMIRQ_MAX 96
>  #define MPS2TZ_RAM_MAX 5
> @@ -153,6 +155,9 @@ struct MPS2TZMachineState {
>      SplitIRQ cpu_irq_splitter[MPS2TZ_NUMIRQ_MAX];
>      Clock *sysclk;
>      Clock *s32kclk;
> +
> +    int remap;

Maybe bool, ...

> +    qemu_irq remap_irq;
>  };
>  
>  #define TYPE_MPS2TZ_MACHINE "mps2tz"
> @@ -228,6 +233,10 @@ static const RAMInfo an505_raminfo[] = { {
>      },
>  };
>  
> +/*
> + * Note that the addresses and MPC numbering here should match up
> + * with those used in remap_memory(), which can swap the BRAM and QSPI.
> + */
>  static const RAMInfo an524_raminfo[] = { {
>          .name = "bram",
>          .base = 0x00000000,
> @@ -457,6 +466,7 @@ static MemoryRegion *make_scc(MPS2TZMachineState *mms, void *opaque,
>  
>      object_initialize_child(OBJECT(mms), "scc", scc, TYPE_MPS2_SCC);
>      sccdev = DEVICE(scc);
> +    qdev_prop_set_uint32(sccdev, "scc-cfg0", mms->remap);

... and here:

       qdev_prop_set_uint32(sccdev, "scc-cfg0", mms->remap ? 1 : 0);

as remap is a bit and scc-cfg0 could have other bits set in the future.

>      qdev_prop_set_uint32(sccdev, "scc-cfg4", 0x2);
>      qdev_prop_set_uint32(sccdev, "scc-aid", 0x00200008);
>      qdev_prop_set_uint32(sccdev, "scc-id", mmc->scc_id);
> @@ -573,6 +583,50 @@ static MemoryRegion *make_mpc(MPS2TZMachineState *mms, void *opaque,
>      return sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 0);
>  }
>  
> +static hwaddr boot_mem_base(MPS2TZMachineState *mms)
> +{
> +    /*
> +     * Return the canonical address of the block which will be mapped
> +     * at address 0x0 (i.e. where the vector table is).
> +     * This is usually 0, but if the AN524 alternate memory map is
> +     * enabled it will be the base address of the QSPI block.
> +     */
> +    return mms->remap ? 0x28000000 : 0;
> +}
> +
> +static void remap_memory(MPS2TZMachineState *mms, int map)
> +{
> +    /*
> +     * Remap the memory for the AN524. 'map' is the value of
> +     * SCC CFG_REG0 bit 0, i.e. 0 for the default map and 1
> +     * for the "option 1" mapping where QSPI is at address 0.
> +     *
> +     * Effectively we need to swap around the "upstream" ends of
> +     * MPC 0 and MPC 1.
> +     */
> +    MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
> +    int i;
> +
> +    if (mmc->fpga_type != FPGA_AN524) {
> +        return;
> +    }
> +

This is done in the machine_reset() handler and during QDev realization,
so probably not important, but since it is reachable by an IRQ handler,
maybe it is better (thinking about code copy/pasting) to wrap this with:

       memory_region_transaction_begin();

> +    for (i = 0; i < 2; i++) {
> +        TZMPC *mpc = &mms->mpc[i];
> +        MemoryRegion *upstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 1);
> +        hwaddr addr = (i ^ map) ? 0x28000000 : 0;
> +
> +        memory_region_set_address(upstream, addr);
> +    }

       memory_region_transaction_commit();

> +}
> +
> +static void remap_irq_fn(void *opaque, int n, int level)
> +{
> +    MPS2TZMachineState *mms = opaque;
> +
> +    remap_memory(mms, level);
> +}

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>