[Qemu-devel] [PATCH 0/8] v8m: minor missing regs and bugfixes

[Qemu-devel] [PATCH 0/8] v8m: minor missing regs and bugfixes

[Peter Maydell]

This patchset is the result of comparing the registers
listed in the v8M Arm ARM against what QEMU was implementing.
It adds a collection of generally unexciting missing registers
(most of which we can simply NOP or make reads-as-written).
There are also a couple of bug fixes in there, of which the
worst is the byte-to-interrupt-number conversions being
completely wrong in several places. I think we didn't notice
that before because if you only deal with the bottom 32
interrupts then they're all in the first register in the
enable/pend/etc banks...

These patches sit on top of the derived-exceptions patchset,
but any conflicts should be purely textual.

Based-on: <1517324542-6607-1-git-send-email-peter.maydell@linaro.org>

thanks
-- PMM

Peter Maydell (8):
  hw/intc/armv7m_nvic: Don't hardcode M profile ID registers in NVIC
  hw/intc/armv7m_nvic: Fix ICSR PENDNMISET/CLR handling
  hw/intc/armv7m_nvic: Implement M profile cache maintenance ops
  hw/intc/armv7m_nvic: Implement v8M CPPWR register
  hw/intc/armv7m_nvic: Implement cache ID registers
  hw/intc/armv7m_nvic: Implement SCR
  target/arm: Implement writing to CONTROL_NS for v8M
  hw/intc/armv7m_nvic: Fix byte-to-interrupt number conversions

 target/arm/cpu.h      | 16 +++++++++
 hw/intc/armv7m_nvic.c | 95 +++++++++++++++++++++++++++++++++++++--------------
 target/arm/cpu.c      | 28 +++++++++++++++
 target/arm/helper.c   | 10 ++++++
 target/arm/machine.c  | 48 ++++++++++++++++++++++++++
 5 files changed, 172 insertions(+), 25 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH 1/8] hw/intc/armv7m_nvic: Don't hardcode M profile ID registers in NVIC

[Peter Maydell]

Instead of hardcoding the values of M profile ID registers in the
NVIC, use the fields in the CPU struct. This will allow us to
give different M profile CPU types different ID register values.

This commit includes the addition of the missing ID_ISAR5,
which exists as RES0 in both v7M and v8M.

(The values of the ID registers might be wrong for the M4 --
this commit leaves the behaviour there unchanged.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 30 ++++++++++++++++--------------
 target/arm/cpu.c      | 28 ++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 360889d30b..63da0fee34 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -990,31 +990,33 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
                       "Aux Fault status registers unimplemented\n");
         return 0;
     case 0xd40: /* PFR0.  */
-        return 0x00000030;
-    case 0xd44: /* PRF1.  */
-        return 0x00000200;
+        return cpu->id_pfr0;
+    case 0xd44: /* PFR1.  */
+        return cpu->id_pfr1;
     case 0xd48: /* DFR0.  */
-        return 0x00100000;
+        return cpu->id_dfr0;
     case 0xd4c: /* AFR0.  */
-        return 0x00000000;
+        return cpu->id_afr0;
     case 0xd50: /* MMFR0.  */
-        return 0x00000030;
+        return cpu->id_mmfr0;
     case 0xd54: /* MMFR1.  */
-        return 0x00000000;
+        return cpu->id_mmfr1;
     case 0xd58: /* MMFR2.  */
-        return 0x00000000;
+        return cpu->id_mmfr2;
     case 0xd5c: /* MMFR3.  */
-        return 0x00000000;
+        return cpu->id_mmfr3;
     case 0xd60: /* ISAR0.  */
-        return 0x01141110;
+        return cpu->id_isar0;
     case 0xd64: /* ISAR1.  */
-        return 0x02111000;
+        return cpu->id_isar1;
     case 0xd68: /* ISAR2.  */
-        return 0x21112231;
+        return cpu->id_isar2;
     case 0xd6c: /* ISAR3.  */
-        return 0x01111110;
+        return cpu->id_isar3;
     case 0xd70: /* ISAR4.  */
-        return 0x01310102;
+        return cpu->id_isar4;
+    case 0xd74: /* ISAR5.  */
+        return cpu->id_isar5;
     /* TODO: Implement debug registers.  */
     case 0xd90: /* MPU_TYPE */
         /* Unified MPU; if the MPU is not present this value is zero */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9da6ea505c..223361fb50 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1146,6 +1146,20 @@ static void cortex_m3_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_M);
     cpu->midr = 0x410fc231;
     cpu->pmsav7_dregion = 8;
+    cpu->id_pfr0 = 0x00000030;
+    cpu->id_pfr1 = 0x00000200;
+    cpu->id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x00000030;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x00000000;
+    cpu->id_mmfr3 = 0x00000000;
+    cpu->id_isar0 = 0x01141110;
+    cpu->id_isar1 = 0x02111000;
+    cpu->id_isar2 = 0x21112231;
+    cpu->id_isar3 = 0x01111110;
+    cpu->id_isar4 = 0x01310102;
+    cpu->id_isar5 = 0x00000000;
 }
 
 static void cortex_m4_initfn(Object *obj)
@@ -1157,6 +1171,20 @@ static void cortex_m4_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
     cpu->midr = 0x410fc240; /* r0p0 */
     cpu->pmsav7_dregion = 8;
+    cpu->id_pfr0 = 0x00000030;
+    cpu->id_pfr1 = 0x00000200;
+    cpu->id_dfr0 = 0x00100000;
+    cpu->id_afr0 = 0x00000000;
+    cpu->id_mmfr0 = 0x00000030;
+    cpu->id_mmfr1 = 0x00000000;
+    cpu->id_mmfr2 = 0x00000000;
+    cpu->id_mmfr3 = 0x00000000;
+    cpu->id_isar0 = 0x01141110;
+    cpu->id_isar1 = 0x02111000;
+    cpu->id_isar2 = 0x21112231;
+    cpu->id_isar3 = 0x01111110;
+    cpu->id_isar4 = 0x01310102;
+    cpu->id_isar5 = 0x00000000;
 }
 
 static void arm_v7m_class_init(ObjectClass *oc, void *data)
-- 
2.16.1

[Qemu-devel] [PATCH 2/8] hw/intc/armv7m_nvic: Fix ICSR PENDNMISET/CLR handling

[Peter Maydell]

The PENDNMISET/CLR bits in the ICSR should be RAZ/WI from
NonSecure state if the AIRCR.BFHFNMINS bit is zero. We had
misimplemented this as making the bits RAZ/WI from both
Secure and NonSecure states. Fix this bug by checking
attrs.secure so that Secure code can pend and unpend NMIs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 63da0fee34..06b9598fbe 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -830,8 +830,8 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
             }
         }
         /* NMIPENDSET */
-        if ((cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK) &&
-            s->vectors[ARMV7M_EXCP_NMI].pending) {
+        if ((attrs.secure || (cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK))
+            && s->vectors[ARMV7M_EXCP_NMI].pending) {
             val |= (1 << 31);
         }
         /* ISRPREEMPT: RES0 when halting debug not implemented */
@@ -1193,7 +1193,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         break;
     }
     case 0xd04: /* Interrupt Control State (ICSR) */
-        if (cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK) {
+        if (attrs.secure || cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK) {
             if (value & (1 << 31)) {
                 armv7m_nvic_set_pending(s, ARMV7M_EXCP_NMI, false);
             } else if (value & (1 << 30) &&
-- 
2.16.1

[Qemu-devel] [PATCH 3/8] hw/intc/armv7m_nvic: Implement M profile cache maintenance ops

[Peter Maydell]

For M profile cores, cache maintenance operations are done by
writing to special registers in the system register space.
For QEMU, cache operations are always NOPs, since we don't
implement the cache. Implementing these explicitly avoids
a spurious LOG_GUEST_ERROR when the guest uses them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 06b9598fbe..74b25ce92c 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1594,6 +1594,18 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         }
         break;
     }
+    case 0xf50: /* ICIALLU */
+    case 0xf58: /* ICIMVAU */
+    case 0xf5c: /* DCIMVAC */
+    case 0xf60: /* DCISW */
+    case 0xf64: /* DCCMVAU */
+    case 0xf68: /* DCCMVAC */
+    case 0xf6c: /* DCCSW */
+    case 0xf70: /* DCCIMVAC */
+    case 0xf74: /* DCCISW */
+    case 0xf78: /* BPIALL */
+        /* Cache and branch predictor maintenance: for QEMU these always NOP */
+        break;
     default:
     bad_offset:
         qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.16.1

[Qemu-devel] [PATCH 4/8] hw/intc/armv7m_nvic: Implement v8M CPPWR register

[Peter Maydell]

The Coprocessor Power Control Register (CPPWR) is new in v8M.
It allows software to control whether coprocessors are allowed
to power down and lose their state. QEMU doesn't have any
notion of power control, so we choose the IMPDEF option of
making the whole register RAZ/WI (indicating that no coprocessors
can ever power down and lose state).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 74b25ce92c..eb49fd77c7 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -776,6 +776,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
     switch (offset) {
     case 4: /* Interrupt Control Type.  */
         return ((s->num_irq - NVIC_FIRST_IRQ) / 32) - 1;
+    case 0xc: /* CPPWR */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {
+            goto bad_offset;
+        }
+        /* We make the IMPDEF choice that nothing can ever go into a
+         * non-retentive power state, which allows us to RAZ/WI this.
+         */
+        return 0;
     case 0x380 ... 0x3bf: /* NVIC_ITNS<n> */
     {
         int startvec = 8 * (offset - 0x380) + NVIC_FIRST_IRQ;
@@ -1175,6 +1183,12 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
     ARMCPU *cpu = s->cpu;
 
     switch (offset) {
+    case 0xc: /* CPPWR */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {
+            goto bad_offset;
+        }
+        /* Make the IMPDEF choice to RAZ/WI this. */
+        break;
     case 0x380 ... 0x3bf: /* NVIC_ITNS<n> */
     {
         int startvec = 8 * (offset - 0x380) + NVIC_FIRST_IRQ;
-- 
2.16.1

[Qemu-devel] [PATCH 5/8] hw/intc/armv7m_nvic: Implement cache ID registers

[Peter Maydell]

M profile cores have a similar setup for cache ID registers
to A profile:
 * Cache Level ID Register (CLIDR) is a fixed value
 * Cache Type Register (CTR) is a fixed value
 * Cache Size ID Registers (CCSIDR) are a bank of registers;
   which one you see is selected by the Cache Size Selection
   Register (CSSELR)

The only difference is that they're in the NVIC memory mapped
register space rather than being coprocessor registers.
Implement the M profile view of them.

Since neither Cortex-M3 nor Cortex-M4 implement caches,
we don't need to update their init functions and can leave
the ctr/clidr/ccsidr[] fields in their ARMCPU structs at zero.
Newer cores (like the Cortex-M33) will want to be able to
set these ID registers to non-zero values, though.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The CSSELR/CCSIDR parts are a bit under-motivated, because
the Cortex-M33 doesn't implement caches either and so they
are RAZ/WI for that as well as M3/M4, though I'd written all
the code before I realized that. This will be helpful if
we ever need a Cortex-M7 model, though (which does have
a couple of CSSIDR array entries).
---
 target/arm/cpu.h      |  9 +++++++++
 hw/intc/armv7m_nvic.c | 13 +++++++++++++
 target/arm/machine.c  | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f21f68ec4a..99c7cb996f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -453,6 +453,7 @@ typedef struct CPUARMState {
         uint32_t faultmask[M_REG_NUM_BANKS];
         uint32_t aircr; /* only holds r/w state if security extn implemented */
         uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
+        uint32_t csselr[M_REG_NUM_BANKS];
     } v7m;
 
     /* Information associated with an exception about to be taken:
@@ -2443,6 +2444,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
     }
 }
 
+static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
+{
+    /* If all the CLIDR.Ctypem bits are 0 there are no caches, and
+     * CSSELR is RAZ/WI.
+     */
+    return (cpu->clidr & 0x001fffff) != 0;
+}
+
 static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
 {
     if (arm_is_secure(env)) {
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index eb49fd77c7..cc83c9e553 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1025,6 +1025,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         return cpu->id_isar4;
     case 0xd74: /* ISAR5.  */
         return cpu->id_isar5;
+    case 0xd78: /* CLIDR */
+        return cpu->clidr;
+    case 0xd7c: /* CTR */
+        return cpu->ctr;
+    case 0xd80: /* CSSIDR */
+        return cpu->ccsidr[cpu->env.v7m.csselr[attrs.secure] & 0xf];
+    case 0xd84: /* CSSELR */
+        return cpu->env.v7m.csselr[attrs.secure];
     /* TODO: Implement debug registers.  */
     case 0xd90: /* MPU_TYPE */
         /* Unified MPU; if the MPU is not present this value is zero */
@@ -1385,6 +1393,11 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         qemu_log_mask(LOG_UNIMP,
                       "NVIC: Aux fault status registers unimplemented\n");
         break;
+    case 0xd84: /* CSSELR */
+        if (!arm_v7m_csselr_razwi(cpu)) {
+            cpu->env.v7m.csselr[attrs.secure] = value & 0xf;
+        }
+        break;
     case 0xd90: /* MPU_TYPE */
         return; /* RO */
     case 0xd94: /* MPU_CTRL */
diff --git a/target/arm/machine.c b/target/arm/machine.c
index a85c2430d3..968ec30b4a 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -108,6 +108,41 @@ static const VMStateDescription vmstate_m_faultmask_primask = {
     }
 };
 
+/* CSSELR is in a subsection because we didn't implement it previously.
+ * Migration from an old implementation will leave it at zero, which
+ * is OK since the only CPUs in the old implementation make the
+ * register RAZ/WI.
+ * Since there was no version of QEMU which implemented the CSSELR for
+ * just non-secure, we transfer both banks here rather than putting
+ * the secure banked version in the m-security subsection.
+ */
+static bool csselr_vmstate_validate(void *opaque, int version_id)
+{
+    ARMCPU *cpu = opaque;
+
+    return cpu->env.v7m.csselr[M_REG_NS] < sizeof(cpu->ccsidr)
+        && cpu->env.v7m.csselr[M_REG_S] < sizeof(cpu->ccsidr);
+}
+
+static bool m_csselr_needed(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    return !arm_v7m_csselr_razwi(cpu);
+}
+
+static const VMStateDescription vmstate_m_csselr = {
+    .name = "cpu/m/csselr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = m_csselr_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(env.v7m.csselr, ARMCPU, M_REG_NUM_BANKS),
+        VMSTATE_VALIDATE("CSSELR is valid", csselr_vmstate_validate),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_m = {
     .name = "cpu/m",
     .version_id = 4,
@@ -129,6 +164,7 @@ static const VMStateDescription vmstate_m = {
     },
     .subsections = (const VMStateDescription*[]) {
         &vmstate_m_faultmask_primask,
+        &vmstate_m_csselr,
         NULL
     }
 };
-- 
2.16.1

[Qemu-devel] [PATCH 6/8] hw/intc/armv7m_nvic: Implement SCR

[Peter Maydell]

We were previously making the system control register (SCR)
just RAZ/WI. Although we don't implement the functionality
this register controls, we should at least provide the state,
including the banked state for v8M.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h      |  7 +++++++
 hw/intc/armv7m_nvic.c | 12 ++++++++----
 target/arm/machine.c  | 12 ++++++++++++
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 99c7cb996f..46dae607e8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -454,6 +454,7 @@ typedef struct CPUARMState {
         uint32_t aircr; /* only holds r/w state if security extn implemented */
         uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
         uint32_t csselr[M_REG_NUM_BANKS];
+        uint32_t scr[M_REG_NUM_BANKS];
     } v7m;
 
     /* Information associated with an exception about to be taken:
@@ -1220,6 +1221,12 @@ FIELD(V7M_CCR, STKALIGN, 9, 1)
 FIELD(V7M_CCR, DC, 16, 1)
 FIELD(V7M_CCR, IC, 17, 1)
 
+/* V7M SCR bits */
+FIELD(V7M_SCR, SLEEPONEXIT, 1, 1)
+FIELD(V7M_SCR, SLEEPDEEP, 2, 1)
+FIELD(V7M_SCR, SLEEPDEEPS, 3, 1)
+FIELD(V7M_SCR, SEVONPEND, 4, 1)
+
 /* V7M AIRCR bits */
 FIELD(V7M_AIRCR, VECTRESET, 0, 1)
 FIELD(V7M_AIRCR, VECTCLRACTIVE, 1, 1)
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index cc83c9e553..8726be796e 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -863,8 +863,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
         }
         return val;
     case 0xd10: /* System Control.  */
-        /* TODO: Implement SLEEPONEXIT.  */
-        return 0;
+        return cpu->env.v7m.scr[attrs.secure];
     case 0xd14: /* Configuration Control.  */
         /* The BFHFNMIGN bit is the only non-banked bit; we
          * keep it in the non-secure copy of the register.
@@ -1282,8 +1281,13 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
         }
         break;
     case 0xd10: /* System Control.  */
-        /* TODO: Implement control registers.  */
-        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
+        /* 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
+         * is architecturally permitted.
+         */
+        value &= ~(R_V7M_SCR_SLEEPDEEP_MASK | R_V7M_SCR_SLEEPDEEPS_MASK);
+        cpu->env.v7m.scr[attrs.secure] = value;
         break;
     case 0xd14: /* Configuration Control.  */
         /* Enforce RAZ/WI on reserved and must-RAZ/WI bits */
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 968ec30b4a..a5feaa9604 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -143,6 +143,16 @@ static const VMStateDescription vmstate_m_csselr = {
     }
 };
 
+static const VMStateDescription vmstate_m_scr = {
+    .name = "cpu/m/scr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(env.v7m.scr[M_REG_NS], ARMCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_m = {
     .name = "cpu/m",
     .version_id = 4,
@@ -165,6 +175,7 @@ static const VMStateDescription vmstate_m = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_m_faultmask_primask,
         &vmstate_m_csselr,
+        &vmstate_m_scr,
         NULL
     }
 };
@@ -328,6 +339,7 @@ static const VMStateDescription vmstate_m_security = {
         VMSTATE_UINT32(env.sau.rnr, ARMCPU),
         VMSTATE_VALIDATE("SAU_RNR is valid", sau_rnr_vmstate_validate),
         VMSTATE_UINT32(env.sau.ctrl, ARMCPU),
+        VMSTATE_UINT32(env.v7m.scr[M_REG_S], ARMCPU),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.16.1

[Qemu-devel] [PATCH 7/8] target/arm: Implement writing to CONTROL_NS for v8M

[Peter Maydell]

In commit 50f11062d4c896 we added support for MSR/MRS access
to the NS banked special registers, but we forgot to implement
the support for writing to CONTROL_NS. Correct the omission.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3332565101..abb4d94a7f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10388,6 +10388,16 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
             }
             env->v7m.faultmask[M_REG_NS] = val & 1;
             return;
+        case 0x94: /* CONTROL_NS */
+            if (!env->v7m.secure) {
+                return;
+            }
+            write_v7m_control_spsel_for_secstate(env,
+                                                 val & R_V7M_CONTROL_SPSEL_MASK,
+                                                 M_REG_NS);
+            env->v7m.control[M_REG_NS] &= ~R_V7M_CONTROL_NPRIV_MASK;
+            env->v7m.control[M_REG_NS] |= val & R_V7M_CONTROL_NPRIV_MASK;
+            return;
         case 0x98: /* SP_NS */
         {
             /* This gives the non-secure SP selected based on whether we're
-- 
2.16.1

[Qemu-devel] [PATCH 8/8] hw/intc/armv7m_nvic: Fix byte-to-interrupt number conversions

[Peter Maydell]

In many of the NVIC registers relating to interrupts, we
have to convert from a byte offset within a register set
into the number of the first interrupt which is affected.
We were getting this wrong for:
 * reads of NVIC_ISPR<n>, NVIC_ISER<n>, NVIC_ICPR<n>, NVIC_ICER<n>,
   NVIC_IABR<n> -- in all these cases we were missing the "* 8"
   needed to convert from the byte offset to the interrupt number
   (since all these registers use one bit per interrupt)
 * writes of NVIC_IPR<n> had the opposite problem of a spurious
   "* 8" (since these registers use one byte per interrupt)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 8726be796e..9433efd1b8 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1721,7 +1721,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
         /* fall through */
     case 0x180 ... 0x1bf: /* NVIC Clear enable */
         val = 0;
-        startvec = offset - 0x180 + NVIC_FIRST_IRQ; /* vector # */
+        startvec = 8 * (offset - 0x180) + NVIC_FIRST_IRQ; /* vector # */
 
         for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
             if (s->vectors[startvec + i].enabled &&
@@ -1735,7 +1735,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
         /* fall through */
     case 0x280 ... 0x2bf: /* NVIC Clear pend */
         val = 0;
-        startvec = offset - 0x280 + NVIC_FIRST_IRQ; /* vector # */
+        startvec = 8 * (offset - 0x280) + NVIC_FIRST_IRQ; /* vector # */
         for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
             if (s->vectors[startvec + i].pending &&
                 (attrs.secure || s->itns[startvec + i])) {
@@ -1745,7 +1745,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
         break;
     case 0x300 ... 0x33f: /* NVIC Active */
         val = 0;
-        startvec = offset - 0x300 + NVIC_FIRST_IRQ; /* vector # */
+        startvec = 8 * (offset - 0x300) + NVIC_FIRST_IRQ; /* vector # */
 
         for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
             if (s->vectors[startvec + i].active &&
@@ -1860,7 +1860,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
     case 0x300 ... 0x33f: /* NVIC Active */
         return MEMTX_OK; /* R/O */
     case 0x400 ... 0x5ef: /* NVIC Priority */
-        startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
+        startvec = (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
 
         for (i = 0; i < size && startvec + i < s->num_irq; i++) {
             if (attrs.secure || s->itns[startvec + i]) {
-- 
2.16.1

Re: [Qemu-devel] [PATCH 1/8] hw/intc/armv7m_nvic: Don't hardcode M profile ID registers in NVIC

[Philippe Mathieu-Daudé]

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

On 02/05/2018 07:57 AM, Peter Maydell wrote:
> Instead of hardcoding the values of M profile ID registers in the
> NVIC, use the fields in the CPU struct. This will allow us to
> give different M profile CPU types different ID register values.
> This commit includes the addition of the missing ID_ISAR5,
> which exists as RES0 in both v7M and v8M.
> 
> (The values of the ID registers might be wrong for the M4 --
> this commit leaves the behaviour there unchanged.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/intc/armv7m_nvic.c | 30 ++++++++++++++++--------------
>  target/arm/cpu.c      | 28 ++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 360889d30b..63da0fee34 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -990,31 +990,33 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>                        "Aux Fault status registers unimplemented\n");
>          return 0;
>      case 0xd40: /* PFR0.  */
> -        return 0x00000030;
> -    case 0xd44: /* PRF1.  */
> -        return 0x00000200;
> +        return cpu->id_pfr0;
> +    case 0xd44: /* PFR1.  */
> +        return cpu->id_pfr1;
>      case 0xd48: /* DFR0.  */
> -        return 0x00100000;
> +        return cpu->id_dfr0;
>      case 0xd4c: /* AFR0.  */
> -        return 0x00000000;
> +        return cpu->id_afr0;
>      case 0xd50: /* MMFR0.  */
> -        return 0x00000030;
> +        return cpu->id_mmfr0;
>      case 0xd54: /* MMFR1.  */
> -        return 0x00000000;
> +        return cpu->id_mmfr1;
>      case 0xd58: /* MMFR2.  */
> -        return 0x00000000;
> +        return cpu->id_mmfr2;
>      case 0xd5c: /* MMFR3.  */
> -        return 0x00000000;
> +        return cpu->id_mmfr3;
>      case 0xd60: /* ISAR0.  */
> -        return 0x01141110;
> +        return cpu->id_isar0;
>      case 0xd64: /* ISAR1.  */
> -        return 0x02111000;
> +        return cpu->id_isar1;
>      case 0xd68: /* ISAR2.  */
> -        return 0x21112231;
> +        return cpu->id_isar2;
>      case 0xd6c: /* ISAR3.  */
> -        return 0x01111110;
> +        return cpu->id_isar3;
>      case 0xd70: /* ISAR4.  */
> -        return 0x01310102;
> +        return cpu->id_isar4;
> +    case 0xd74: /* ISAR5.  */
> +        return cpu->id_isar5;
>      /* TODO: Implement debug registers.  */
>      case 0xd90: /* MPU_TYPE */
>          /* Unified MPU; if the MPU is not present this value is zero */
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9da6ea505c..223361fb50 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1146,6 +1146,20 @@ static void cortex_m3_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_M);
>      cpu->midr = 0x410fc231;
>      cpu->pmsav7_dregion = 8;
> +    cpu->id_pfr0 = 0x00000030;
> +    cpu->id_pfr1 = 0x00000200;
> +    cpu->id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->id_mmfr0 = 0x00000030;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x00000000;
> +    cpu->id_mmfr3 = 0x00000000;
> +    cpu->id_isar0 = 0x01141110;
> +    cpu->id_isar1 = 0x02111000;
> +    cpu->id_isar2 = 0x21112231;
> +    cpu->id_isar3 = 0x01111110;
> +    cpu->id_isar4 = 0x01310102;
> +    cpu->id_isar5 = 0x00000000;
>  }
>  
>  static void cortex_m4_initfn(Object *obj)
> @@ -1157,6 +1171,20 @@ static void cortex_m4_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP);
>      cpu->midr = 0x410fc240; /* r0p0 */
>      cpu->pmsav7_dregion = 8;
> +    cpu->id_pfr0 = 0x00000030;
> +    cpu->id_pfr1 = 0x00000200;
> +    cpu->id_dfr0 = 0x00100000;
> +    cpu->id_afr0 = 0x00000000;
> +    cpu->id_mmfr0 = 0x00000030;
> +    cpu->id_mmfr1 = 0x00000000;
> +    cpu->id_mmfr2 = 0x00000000;
> +    cpu->id_mmfr3 = 0x00000000;
> +    cpu->id_isar0 = 0x01141110;
> +    cpu->id_isar1 = 0x02111000;
> +    cpu->id_isar2 = 0x21112231;
> +    cpu->id_isar3 = 0x01111110;
> +    cpu->id_isar4 = 0x01310102;
> +    cpu->id_isar5 = 0x00000000;
>  }
>  
>  static void arm_v7m_class_init(ObjectClass *oc, void *data)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 8/8] hw/intc/armv7m_nvic: Fix byte-to-interrupt number conversions

[Philippe Mathieu-Daudé]

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

Hi Peter,

On 02/05/2018 07:57 AM, Peter Maydell wrote:
> In many of the NVIC registers relating to interrupts, we
> have to convert from a byte offset within a register set
> into the number of the first interrupt which is affected.
> We were getting this wrong for:
>  * reads of NVIC_ISPR<n>, NVIC_ISER<n>, NVIC_ICPR<n>, NVIC_ICER<n>,
>    NVIC_IABR<n> -- in all these cases we were missing the "* 8"
>    needed to convert from the byte offset to the interrupt number
>    (since all these registers use one bit per interrupt)
>  * writes of NVIC_IPR<n> had the opposite problem of a spurious
>    "* 8" (since these registers use one byte per interrupt)

What about using inline function (suggested) with those comments to ease
code review, since this code is kinda confusing at first.

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

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

> ---
>  hw/intc/armv7m_nvic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 8726be796e..9433efd1b8 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1721,7 +1721,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>          /* fall through */
>      case 0x180 ... 0x1bf: /* NVIC Clear enable */
>          val = 0;
> -        startvec = offset - 0x180 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x180) + NVIC_FIRST_IRQ; /* vector # */

static uint32_t off2vec_rd(uint32_t offset, uint32_t base)
{
    return 8 * (offset - base) + NVIC_FIRST_IRQ;
}

>  
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
>              if (s->vectors[startvec + i].enabled &&
> @@ -1735,7 +1735,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>          /* fall through */
>      case 0x280 ... 0x2bf: /* NVIC Clear pend */
>          val = 0;
> -        startvec = offset - 0x280 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x280) + NVIC_FIRST_IRQ; /* vector # */
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
>              if (s->vectors[startvec + i].pending &&
>                  (attrs.secure || s->itns[startvec + i])) {
> @@ -1745,7 +1745,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>          break;
>      case 0x300 ... 0x33f: /* NVIC Active */
>          val = 0;
> -        startvec = offset - 0x300 + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = 8 * (offset - 0x300) + NVIC_FIRST_IRQ; /* vector # */
>  
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
>              if (s->vectors[startvec + i].active &&
> @@ -1860,7 +1860,7 @@ static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
>      case 0x300 ... 0x33f: /* NVIC Active */
>          return MEMTX_OK; /* R/O */
>      case 0x400 ... 0x5ef: /* NVIC Priority */
> -        startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
> +        startvec = (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */

static uint32_t off2vec_wr(uint32_t offset, uint32_t base)
{
    return (offset - base) + NVIC_FIRST_IRQ;
}

>  
>          for (i = 0; i < size && startvec + i < s->num_irq; i++) {
>              if (attrs.secure || s->itns[startvec + i]) {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 8/8] hw/intc/armv7m_nvic: Fix byte-to-interrupt number conversions

[Peter Maydell]

On 5 February 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 02/05/2018 07:57 AM, Peter Maydell wrote:
>> In many of the NVIC registers relating to interrupts, we
>> have to convert from a byte offset within a register set
>> into the number of the first interrupt which is affected.
>> We were getting this wrong for:
>>  * reads of NVIC_ISPR<n>, NVIC_ISER<n>, NVIC_ICPR<n>, NVIC_ICER<n>,
>>    NVIC_IABR<n> -- in all these cases we were missing the "* 8"
>>    needed to convert from the byte offset to the interrupt number
>>    (since all these registers use one bit per interrupt)
>>  * writes of NVIC_IPR<n> had the opposite problem of a spurious
>>    "* 8" (since these registers use one byte per interrupt)
>
> What about using inline function (suggested) with those comments to ease
> code review, since this code is kinda confusing at first.
>
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> at any rate:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> ---
>>  hw/intc/armv7m_nvic.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 8726be796e..9433efd1b8 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -1721,7 +1721,7 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>>          /* fall through */
>>      case 0x180 ... 0x1bf: /* NVIC Clear enable */
>>          val = 0;
>> -        startvec = offset - 0x180 + NVIC_FIRST_IRQ; /* vector # */
>> +        startvec = 8 * (offset - 0x180) + NVIC_FIRST_IRQ; /* vector # */
>
> static uint32_t off2vec_rd(uint32_t offset, uint32_t base)
> {
>     return 8 * (offset - base) + NVIC_FIRST_IRQ;
> }

This function name suggests that it's for reads, which isn't
what's going on here. The 8 * version is for registers which
have 1 bit per interrupt. The version without 8* is for
registers which have 8 bits per interrupt. It just happens
that we got the reads wrong for a bunch of 1-bit-per-interrupt
registers and the writes wrong for an 8-bit-per-interrupt reg.
(I have a feeling this is something I noticed at some point in
code review of the original author's patches, and then didn't
realize had been not quite corrected in all the places it needed
to be, and overcorrected in one spot.)

Also, if you hide the "8 *" inside a function here, you lose
the fact that it parallels the multiplication in "end = size * 8"
in each of these loops. So I agree that it's confusing, but
overall I think it's better to have all the logic in one
place so you can read the whole loop easily.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/8] hw/intc/armv7m_nvic: Implement cache ID registers

[Philippe Mathieu-Daudé]

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

Hi Peter,

On 02/05/2018 07:57 AM, Peter Maydell wrote:
> M profile cores have a similar setup for cache ID registers
> to A profile:
>  * Cache Level ID Register (CLIDR) is a fixed value
>  * Cache Type Register (CTR) is a fixed value
>  * Cache Size ID Registers (CCSIDR) are a bank of registers;
>    which one you see is selected by the Cache Size Selection
>    Register (CSSELR)
> 
> The only difference is that they're in the NVIC memory mapped
> register space rather than being coprocessor registers.
> Implement the M profile view of them.
> 
> Since neither Cortex-M3 nor Cortex-M4 implement caches,
> we don't need to update their init functions and can leave
> the ctr/clidr/ccsidr[] fields in their ARMCPU structs at zero.
> Newer cores (like the Cortex-M33) will want to be able to
> set these ID registers to non-zero values, though.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The CSSELR/CCSIDR parts are a bit under-motivated, because
> the Cortex-M33 doesn't implement caches either and so they
> are RAZ/WI for that as well as M3/M4, though I'd written all
> the code before I realized that. This will be helpful if
> we ever need a Cortex-M7 model, though (which does have
> a couple of CSSIDR array entries).

I wonder if it is easier/faster to add a check for the "Instruction not
Data" bit and the level value is not 7 (not permitted) or simple comments.

> ---
>  target/arm/cpu.h      |  9 +++++++++
>  hw/intc/armv7m_nvic.c | 13 +++++++++++++
>  target/arm/machine.c  | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f21f68ec4a..99c7cb996f 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -453,6 +453,7 @@ typedef struct CPUARMState {
>          uint32_t faultmask[M_REG_NUM_BANKS];
>          uint32_t aircr; /* only holds r/w state if security extn implemented */
>          uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
> +        uint32_t csselr[M_REG_NUM_BANKS];
>      } v7m;
>  
>      /* Information associated with an exception about to be taken:
> @@ -2443,6 +2444,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>      }
>  }
>  
> +static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
> +{
> +    /* If all the CLIDR.Ctypem bits are 0 there are no caches, and
> +     * CSSELR is RAZ/WI.
> +     */
> +    return (cpu->clidr & 0x001fffff) != 0;
> +}

Suggestion to be consistent with other bitfields:

/* V7M Cache Level ID (CLIDR) */
FIELD(V7M_CLIDR, CTYPE, 0, 7 * 3)

So we can use:

    return (cpu->clidr & R_V7M_CLIDR_CTYPE_MASK) != 0;

> +
>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  {
>      if (arm_is_secure(env)) {
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index eb49fd77c7..cc83c9e553 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1025,6 +1025,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>          return cpu->id_isar4;
>      case 0xd74: /* ISAR5.  */
>          return cpu->id_isar5;
> +    case 0xd78: /* CLIDR */
> +        return cpu->clidr;
> +    case 0xd7c: /* CTR */
> +        return cpu->ctr;
> +    case 0xd80: /* CSSIDR */
> +        return cpu->ccsidr[cpu->env.v7m.csselr[attrs.secure] & 0xf];

/* V7M Cache Size Selection (CSSELR) */
FIELD(V7M_CSSELR, LEVEL, 1, 3)

> +    case 0xd84: /* CSSELR */
> +        return cpu->env.v7m.csselr[attrs.secure];
>      /* TODO: Implement debug registers.  */
>      case 0xd90: /* MPU_TYPE */
>          /* Unified MPU; if the MPU is not present this value is zero */
> @@ -1385,6 +1393,11 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>          qemu_log_mask(LOG_UNIMP,
>                        "NVIC: Aux fault status registers unimplemented\n");
>          break;
> +    case 0xd84: /* CSSELR */
> +        if (!arm_v7m_csselr_razwi(cpu)) {
> +            cpu->env.v7m.csselr[attrs.secure] = value & 0xf;
> +        }
> +        break;
>      case 0xd90: /* MPU_TYPE */
>          return; /* RO */
>      case 0xd94: /* MPU_CTRL */
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index a85c2430d3..968ec30b4a 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -108,6 +108,41 @@ static const VMStateDescription vmstate_m_faultmask_primask = {
>      }
>  };
>  
> +/* CSSELR is in a subsection because we didn't implement it previously.
> + * Migration from an old implementation will leave it at zero, which
> + * is OK since the only CPUs in the old implementation make the
> + * register RAZ/WI.
> + * Since there was no version of QEMU which implemented the CSSELR for
> + * just non-secure, we transfer both banks here rather than putting
> + * the secure banked version in the m-security subsection.
> + */
> +static bool csselr_vmstate_validate(void *opaque, int version_id)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    return cpu->env.v7m.csselr[M_REG_NS] < sizeof(cpu->ccsidr)
> +        && cpu->env.v7m.csselr[M_REG_S] < sizeof(cpu->ccsidr);
> +}
> +
> +static bool m_csselr_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    return !arm_v7m_csselr_razwi(cpu);
> +}
> +
> +static const VMStateDescription vmstate_m_csselr = {
> +    .name = "cpu/m/csselr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = m_csselr_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(env.v7m.csselr, ARMCPU, M_REG_NUM_BANKS),
> +        VMSTATE_VALIDATE("CSSELR is valid", csselr_vmstate_validate),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
>      .version_id = 4,
> @@ -129,6 +164,7 @@ static const VMStateDescription vmstate_m = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_m_faultmask_primask,
> +        &vmstate_m_csselr,
>          NULL
>      }
>  };
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/8] hw/intc/armv7m_nvic: Implement cache ID registers

[Peter Maydell]

On 5 February 2018 at 23:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 02/05/2018 07:57 AM, Peter Maydell wrote:
>> M profile cores have a similar setup for cache ID registers
>> to A profile:
>>  * Cache Level ID Register (CLIDR) is a fixed value
>>  * Cache Type Register (CTR) is a fixed value
>>  * Cache Size ID Registers (CCSIDR) are a bank of registers;
>>    which one you see is selected by the Cache Size Selection
>>    Register (CSSELR)
>>
>> The only difference is that they're in the NVIC memory mapped
>> register space rather than being coprocessor registers.
>> Implement the M profile view of them.
>>
>> Since neither Cortex-M3 nor Cortex-M4 implement caches,
>> we don't need to update their init functions and can leave
>> the ctr/clidr/ccsidr[] fields in their ARMCPU structs at zero.
>> Newer cores (like the Cortex-M33) will want to be able to
>> set these ID registers to non-zero values, though.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> The CSSELR/CCSIDR parts are a bit under-motivated, because
>> the Cortex-M33 doesn't implement caches either and so they
>> are RAZ/WI for that as well as M3/M4, though I'd written all
>> the code before I realized that. This will be helpful if
>> we ever need a Cortex-M7 model, though (which does have
>> a couple of CSSIDR array entries).
>
> I wonder if it is easier/faster to add a check for the "Instruction not
> Data" bit and the level value is not 7 (not permitted) or simple comments.
>
>> ---
>>  target/arm/cpu.h      |  9 +++++++++
>>  hw/intc/armv7m_nvic.c | 13 +++++++++++++
>>  target/arm/machine.c  | 36 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 58 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index f21f68ec4a..99c7cb996f 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -453,6 +453,7 @@ typedef struct CPUARMState {
>>          uint32_t faultmask[M_REG_NUM_BANKS];
>>          uint32_t aircr; /* only holds r/w state if security extn implemented */
>>          uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
>> +        uint32_t csselr[M_REG_NUM_BANKS];
>>      } v7m;
>>
>>      /* Information associated with an exception about to be taken:
>> @@ -2443,6 +2444,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>>      }
>>  }
>>
>> +static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
>> +{
>> +    /* If all the CLIDR.Ctypem bits are 0 there are no caches, and
>> +     * CSSELR is RAZ/WI.
>> +     */
>> +    return (cpu->clidr & 0x001fffff) != 0;
>> +}
>
> Suggestion to be consistent with other bitfields:
>
> /* V7M Cache Level ID (CLIDR) */
> FIELD(V7M_CLIDR, CTYPE, 0, 7 * 3)
>
> So we can use:
>
>     return (cpu->clidr & R_V7M_CLIDR_CTYPE_MASK) != 0;
>
>> +
>>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>>  {
>>      if (arm_is_secure(env)) {
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index eb49fd77c7..cc83c9e553 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -1025,6 +1025,14 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>>          return cpu->id_isar4;
>>      case 0xd74: /* ISAR5.  */
>>          return cpu->id_isar5;
>> +    case 0xd78: /* CLIDR */
>> +        return cpu->clidr;
>> +    case 0xd7c: /* CTR */
>> +        return cpu->ctr;
>> +    case 0xd80: /* CSSIDR */
>> +        return cpu->ccsidr[cpu->env.v7m.csselr[attrs.secure] & 0xf];
>
> /* V7M Cache Size Selection (CSSELR) */
> FIELD(V7M_CSSELR, LEVEL, 1, 3)

Yes, but the index into the csselr[] array is by both the level
field and the I/D bit: csselr[0] is L1 dcache, csselr[1] is
L1 icache, csselr[2] is L2 dcache, and so on.

I guess we could define some field constants.

thanks
-- PMM