[PATCH for-6.0 0/2] mps3-an524: Fix MPC setting for SRAM block

[PATCH for-6.0 0/2] mps3-an524: Fix MPC setting for SRAM block

[Peter Maydell]

The AN524 FPGA image has three MPCs: one for the BRAM, one for
the QSPI flash, and one for the DDR. In the an524_raminfo[] array
that defines the various RAM blocks on the board, we incorrectly
set the .mpc field for the SRAM to 1 as well as for the QSPI flash.
The effect of this was to cause the QSPI flash not to be mapped
at all (because when we mapped the 'upstream' end of each MPC,
we found the incorrectly marked SRAM entry before the QSPI one
when scanning through the raminfo array, and so put the upstream
end of MPC1 at the SRAM address).

Patch 1 fixes the SRAM block to use '.mpc = -1' indicating that
there is no associated MPC. Patch 2 adds an assert() that would
have caught this programming error (which is quite easy to make
if you're constructing the raminfo array for a new board by
copying and modifying entries from existing boards).

I think this makes sense to put into 6.0, it's a pretty safe change.

Peter Maydell (2):
  hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block
  hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC

 hw/arm/mps2-tz.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH for-6.0 1/2] hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block

[Peter Maydell]

The AN524 has three MPCs: one for the BRAM, one for the QSPI flash,
and one for the DDR.  We incorrectly set the .mpc field in the
RAMInfo struct for the SRAM block to 1, giving it the same MPC we are
using for the QSPI.  The effect of this was that the QSPI didn't get
mapped into the system address space at all, via an MPC or otherwise,
and guest programs which tried to read from the QSPI would get a bus
error.  Correct the SRAM RAMInfo to indicate that it does not have an
associated MPC.

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

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 3fbe3d29f95..5ebd671bf83 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -238,7 +238,7 @@ static const RAMInfo an524_raminfo[] = { {
         .name = "sram",
         .base = 0x20000000,
         .size = 32 * 4 * KiB,
-        .mpc = 1,
+        .mpc = -1,
         .mrindex = 1,
     }, {
         /* We don't model QSPI flash yet; for now expose it as simple ROM */
-- 
2.20.1

Re: [PATCH for-6.0 1/2] hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block

[Philippe Mathieu-Daudé]

On 4/9/21 5:05 PM, Peter Maydell wrote:
> The AN524 has three MPCs: one for the BRAM, one for the QSPI flash,
> and one for the DDR.  We incorrectly set the .mpc field in the
> RAMInfo struct for the SRAM block to 1, giving it the same MPC we are
> using for the QSPI.  The effect of this was that the QSPI didn't get
> mapped into the system address space at all, via an MPC or otherwise,
> and guest programs which tried to read from the QSPI would get a bus
> error.  Correct the SRAM RAMInfo to indicate that it does not have an
> associated MPC.
> 

Fixes: 25ff112a8cc ("hw/arm/mps2-tz: Add new mps3-an524 board")

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/arm/mps2-tz.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH for-6.0 2/2] hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC

[Peter Maydell]

Each board in mps2-tz.c specifies a RAMInfo[] array providing
information about each RAM in the board.  The .mpc field of the
RAMInfo struct specifies which MPC, if any, the RAM is attached to.
We already assert if the array doesn't have any entry for an MPC, but
we don't diagnose the error of using the same MPC number twice (which
is quite easy to do by accident if copy-and-pasting structure
entries).

Enhance find_raminfo_for_mpc() so that it detects multiple entries
for the MPC as well as missing entries.

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

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 5ebd671bf83..25016e464d9 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -306,14 +306,18 @@ static const RAMInfo *find_raminfo_for_mpc(MPS2TZMachineState *mms, int mpc)
 {
     MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
     const RAMInfo *p;
+    const RAMInfo *found = NULL;
 
     for (p = mmc->raminfo; p->name; p++) {
         if (p->mpc == mpc && !(p->flags & IS_ALIAS)) {
-            return p;
+            /* There should only be one entry in the array for this MPC */
+            g_assert(!found);
+            found = p;
         }
     }
     /* if raminfo array doesn't have an entry for each MPC this is a bug */
-    g_assert_not_reached();
+    assert(found);
+    return found;
 }
 
 static MemoryRegion *mr_for_raminfo(MPS2TZMachineState *mms,
-- 
2.20.1

Re: [PATCH for-6.0 0/2] mps3-an524: Fix MPC setting for SRAM block

[Richard Henderson]

On 4/9/21 8:05 AM, Peter Maydell wrote:
> Peter Maydell (2):
>    hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block
>    hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC

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

r~