[Qemu-devel] [PATCH v3 0/2] nvic: Handle ARMv6-M SCS reserved registers

[Qemu-devel] [PATCH v3 0/2] nvic: Handle ARMv6-M SCS reserved registers

[Julia Suvorova]

v3:
    * Fix indents
v2:
    * Use ARM_FEATURE_M_MAIN instead of ARM_FEATURE_V7 in most cases
    * Remove CPUID registers check
    * Use bad_offset instead of return
    * Misc style fixes

Julia Suvorova (2):
  nvic: Handle ARMv6-M SCS reserved registers
  tests: Add ARMv6-M reserved register test

 hw/intc/armv7m_nvic.c             | 51 ++++++++++++++++++++++++--
 tests/Makefile.include            |  2 ++
 tests/tcg/arm/test-reserved-reg.c | 60 +++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/arm/test-reserved-reg.c

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/2] nvic: Handle ARMv6-M SCS reserved registers

[Julia Suvorova]

Handle SCS reserved registers listed in ARMv6-M ARM D3.6.1.
All reserved registers are RAZ/WI. ARM_FEATURE_M_MAIN is used for the
checks, because these registers are reserved in ARMv8-M Baseline too.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/intc/armv7m_nvic.c | 51 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index aba4510c70..7f71b336bd 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -865,6 +865,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         }
         return val;
     case 0xd10: /* System Control.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            goto bad_offset;
+        }
         return cpu->env.v7m.scr[attrs.secure];
     case 0xd14: /* Configuration Control.  */
         /* The BFHFNMIGN bit is the only non-banked bit; we
@@ -986,12 +989,21 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         }
         return val;
     case 0xd2c: /* Hard Fault Status.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         return cpu->env.v7m.hfsr;
     case 0xd30: /* Debug Fault Status.  */
         return cpu->env.v7m.dfsr;
     case 0xd34: /* MMFAR MemManage Fault Address */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         return cpu->env.v7m.mmfar[attrs.secure];
     case 0xd38: /* Bus Fault Address.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         return cpu->env.v7m.bfar;
     case 0xd3c: /* Aux Fault Status.  */
         /* TODO: Implement fault status registers.  */
@@ -1287,6 +1299,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         }
         break;
     case 0xd10: /* System Control.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            goto bad_offset;
+        }
         /* We don't implement deep-sleep so these bits are RAZ/WI.
          * The other bits in the register are banked.
          * QEMU's implementation ignores SEVONPEND and SLEEPONEXIT, which
@@ -1388,15 +1403,24 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         nvic_irq_update(s);
         break;
     case 0xd2c: /* Hard Fault Status.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         cpu->env.v7m.hfsr &= ~value; /* W1C */
         break;
     case 0xd30: /* Debug Fault Status.  */
         cpu->env.v7m.dfsr &= ~value; /* W1C */
         break;
     case 0xd34: /* Mem Manage Address.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         cpu->env.v7m.mmfar[attrs.secure] = value;
         return;
     case 0xd38: /* Bus Fault Address.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
         cpu->env.v7m.bfar = value;
         return;
     case 0xd3c: /* Aux Fault Status.  */
@@ -1626,6 +1650,11 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
     case 0xf00: /* Software Triggered Interrupt Register */
     {
         int excnum = (value & 0x1ff) + NVIC_FIRST_IRQ;
+
+        if (!arm_feature(&cpu->env, ARM_FEATURE_M_MAIN)) {
+            goto bad_offset;
+        }
+
         if (excnum < s->num_irq) {
             armv7m_nvic_set_pending(s, excnum, false);
         }
@@ -1775,7 +1804,13 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
             }
         }
         break;
-    case 0xd18 ... 0xd23: /* System Handler Priority (SHPR1, SHPR2, SHPR3) */
+    case 0xd18: /* System Handler Priority (SHPR1) */
+        if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) {
+            val = 0;
+            break;
+        }
+        /* fall through */
+    case 0xd1c ... 0xd23: /* System Handler Priority (SHPR2, SHPR3) */
         val = 0;
         for (i = 0; i < size; i++) {
             unsigned hdlidx = (offset - 0xd14) + i;
@@ -1788,6 +1823,10 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
         }
         break;
     case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
+        if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) {
+            val = 0;
+            break;
+        };
         /* The BFSR bits [15:8] are shared between security states
          * and we store them in the NS copy
          */
@@ -1882,7 +1921,12 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
         }
         nvic_irq_update(s);
         return MEMTX_OK;
-    case 0xd18 ... 0xd23: /* System Handler Priority (SHPR1, SHPR2, SHPR3) */
+    case 0xd18: /* System Handler Priority (SHPR1) */
+        if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) {
+            return MEMTX_OK;
+        }
+        /* fall through */
+    case 0xd1c ... 0xd23: /* System Handler Priority (SHPR2, SHPR3) */
         for (i = 0; i < size; i++) {
             unsigned hdlidx = (offset - 0xd14) + i;
             int newprio = extract32(value, i * 8, 8);
@@ -1896,6 +1940,9 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
         nvic_irq_update(s);
         return MEMTX_OK;
     case 0xd28 ... 0xd2b: /* Configurable Fault Status (CFSR) */
+        if (!arm_feature(&s->cpu->env, ARM_FEATURE_M_MAIN)) {
+            return MEMTX_OK;
+        }
         /* All bits are W1C, so construct 32 bit value with 0s in
          * the parts not written by the access size
          */
-- 
2.17.1

[Qemu-devel] [RFC v3 2/2] tests: Add ARMv6-M reserved register test

[Julia Suvorova]

Check that reserved SCS registers return 0 at read,
and writes are ignored.

Based-on: <20180627143815.1829-1-joel@jms.id.au>
Based-on: <20180630091343.14391-1-stefanha@redhat.com>

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
Test will work if Joel's patches will use ARMv6-M.

 tests/Makefile.include            |  2 ++
 tests/tcg/arm/test-reserved-reg.c | 60 +++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 tests/tcg/arm/test-reserved-reg.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index d323c42682..8ab0b0d15f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -387,6 +387,7 @@ gcov-files-arm-y += hw/timer/arm_mptimer.c
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-y += tests/tcg/arm/test-reserved-reg$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
@@ -771,6 +772,7 @@ tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/test-reserved-reg$(EXESUF): tests/tcg/arm/test-reserved-reg.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
diff --git a/tests/tcg/arm/test-reserved-reg.c b/tests/tcg/arm/test-reserved-reg.c
new file mode 100644
index 0000000000..97273ff24d
--- /dev/null
+++ b/tests/tcg/arm/test-reserved-reg.c
@@ -0,0 +1,60 @@
+/*
+ * Test ARMv6-M SCS reserved registers
+ *
+ * Copyright (c) 2018 Julia Suvorova <jusual@mail.ru>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2
+ * or later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+static void test_reserved_reg(void)
+{
+    QTestState *s;
+    int i;
+    static const uint64_t reserved_reg[] = { 0xe000ed10, /* SCR */
+                                             0xe000ed18, /* SHPR1 */
+                                             0xe000ed28, /* CFSR */
+                                             0xe000ed2c, /* HFSR */
+                                             0xe000ed34, /* MMFAR */
+                                             0xe000ed38, /* BFAR */
+                                             0xe000ed3c, /* AFSR */
+                                             0xe000ed40, /* CPUID */
+                                             0xe000ed88, /* CPACR */
+                                             0xe000ef00  /* STIR */ };
+    static const uint8_t mini_kernel[] = { 0x00, 0x00, 0x00, 0x00,
+                                           0x09, 0x00, 0x00, 0x00 };
+    ssize_t wlen, kernel_size = sizeof(mini_kernel);
+    int code_fd;
+    char codetmp[] = "/tmp/reserved-reg-test-XXXXXX";
+
+    code_fd = mkstemp(codetmp);
+    wlen = write(code_fd, mini_kernel, sizeof(mini_kernel));
+    g_assert(wlen == kernel_size);
+    close(code_fd);
+
+    s = qtest_startf("-kernel %s -M microbit -nographic", codetmp);
+
+    for (i = 0; i < ARRAY_SIZE(reserved_reg); i++) {
+        int res;
+        qtest_writel(s, reserved_reg[i], 1);
+        res = qtest_readl(s, reserved_reg[i]);
+        g_assert(!res);
+    }
+
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("tcg/arm/test-reserved-reg", test_reserved_reg);
+    ret = g_test_run();
+
+    return ret;
+}
-- 
2.17.1

Re: [Qemu-devel] [RFC v3 2/2] tests: Add ARMv6-M reserved register test

[Peter Maydell]

On 5 July 2018 at 23:21, Julia Suvorova <jusual@mail.ru> wrote:
> Check that reserved SCS registers return 0 at read,
> and writes are ignored.
>
> Based-on: <20180627143815.1829-1-joel@jms.id.au>
> Based-on: <20180630091343.14391-1-stefanha@redhat.com>
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
> Test will work if Joel's patches will use ARMv6-M.
>
>  tests/Makefile.include            |  2 ++
>  tests/tcg/arm/test-reserved-reg.c | 60 +++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 tests/tcg/arm/test-reserved-reg.c

Is this in the wrong place? It doesn't look like a
tests/tcg test -- those directories are for test
programs which need to be compiled with a cross compiler
for the guest CPU and then run on it.

I tried running 'make check' and I got this error:

  CC      tests/tcg/arm/test-reserved-reg.o
/home/petmay01/linaro/qemu-from-laptop/qemu/tests/tcg/arm/test-reserved-reg.c:60:1:
fatal error: opening dependency file
tests/tcg/arm/test-reserved-reg.d: No such file or directory
 }
 ^

which is a bit weird.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/2] nvic: Handle ARMv6-M SCS reserved registers

[Peter Maydell]

On 5 July 2018 at 23:21, Julia Suvorova <jusual@mail.ru> wrote:
> Handle SCS reserved registers listed in ARMv6-M ARM D3.6.1.
> All reserved registers are RAZ/WI. ARM_FEATURE_M_MAIN is used for the
> checks, because these registers are reserved in ARMv8-M Baseline too.
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/intc/armv7m_nvic.c | 51 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)

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

I've applied this patch (but not the 2nd one in this series)
to target-arm.for-3.1.

thanks
-- PMM

Re: [Qemu-devel] [RFC v3 2/2] tests: Add ARMv6-M reserved register test

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

On Fri, Jul 06, 2018 at 03:55:14PM +0100, Peter Maydell wrote:
> On 5 July 2018 at 23:21, Julia Suvorova <jusual@mail.ru> wrote:
> > Check that reserved SCS registers return 0 at read,
> > and writes are ignored.
> >
> > Based-on: <20180627143815.1829-1-joel@jms.id.au>
> > Based-on: <20180630091343.14391-1-stefanha@redhat.com>
> >
> > Signed-off-by: Julia Suvorova <jusual@mail.ru>
> > ---
> > Test will work if Joel's patches will use ARMv6-M.
> >
> >  tests/Makefile.include            |  2 ++
> >  tests/tcg/arm/test-reserved-reg.c | 60 +++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >  create mode 100644 tests/tcg/arm/test-reserved-reg.c
> 
> Is this in the wrong place? It doesn't look like a
> tests/tcg test -- those directories are for test
> programs which need to be compiled with a cross compiler
> for the guest CPU and then run on it.

Yes, this test should go in tests/ along with the other qtests.

> 
> I tried running 'make check' and I got this error:
> 
>   CC      tests/tcg/arm/test-reserved-reg.o
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/tcg/arm/test-reserved-reg.c:60:1:
> fatal error: opening dependency file
> tests/tcg/arm/test-reserved-reg.d: No such file or directory
>  }
>  ^
> 
> which is a bit weird.
> 
> thanks
> -- PMM
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]