[Qemu-devel] [PATCH for-2.10 0/5] M profile MPU bugfixes

[Qemu-devel] [PATCH for-2.10 0/5] M profile MPU bugfixes

[Peter Maydell]

This patchset fixes some bugs in the M profile MPU code:
 * the guest shouldn't be able to make system space executable
 * PPB region accesses should not be subject to MPU lookups
 * we were not resetting the PMSAv7 MPU state for M profile CPUs
 * we weren't migrating the MPU_RNR state

The renaming from cp15.c6_rgnr to pmsav7.rnr is not strictly
necessary for 2.10, but it doesn't affect many places in the code
and it restores the invariant that no mutable M profile CPU
state is stored in env->cp15.something fields.

thanks
-- PMM

Peter Maydell (5):
  target/arm: Don't do MPU lookups for addresses in M profile PPB region
  target/arm: Don't allow guest to make System space executable for M
    profile
  target/arm: Rename cp15.c6_rgnr to pmsav7.rnr
  target/arm: Move PMSAv7 reset into arm_cpu_reset() so M profile MPUs
    get reset
  target/arm: Migrate MPU_RNR register state for M profile cores

 hw/intc/armv7m_nvic.c | 14 +++++------
 target/arm/cpu.c      | 14 +++++++++++
 target/arm/cpu.h      |  3 +--
 target/arm/helper.c   | 67 +++++++++++++++++++++++++++++++++++----------------
 target/arm/machine.c  | 30 ++++++++++++++++++++++-
 5 files changed, 97 insertions(+), 31 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 1/5] target/arm: Don't do MPU lookups for addresses in M profile PPB region

[Peter Maydell]

The M profile PMSAv7 specification says that if the address being looked
up is in the PPB region (0xe0000000 - 0xe00fffff) then we do not use
the MPU regions but always use the default memory map. Implement this
(we were previously behaving like an R profile PMSAv7, which does not
special case this).

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 4ed32c5..ceef225 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8244,6 +8244,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu,
     }
 }
 
+static inline bool is_ppb_region(CPUARMState *env, uint32_t address)
+{
+    /* True if address is in the M profile PPB region 0xe0000000 - 0xe00fffff */
+    return arm_feature(env, ARM_FEATURE_M) &&
+        extract32(address, 20, 12) == 0xe00;
+}
+
 static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                                  int access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
@@ -8255,7 +8262,15 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
     *phys_ptr = address;
     *prot = 0;
 
-    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
+    if (regime_translation_disabled(env, mmu_idx) ||
+        is_ppb_region(env, address)) {
+        /* MPU disabled or M profile PPB access: use default memory map.
+         * The other case which uses the default memory map in the
+         * v7M ARM ARM pseudocode is exception vector reads from the vector
+         * table. In QEMU those accesses are done in arm_v7m_load_vector(),
+         * which always does a direct read using address_space_ldl(), rather
+         * than going via this function, so we don't need to check that here.
+         */
         get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
     } else { /* MPU enabled */
         for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 2/5] target/arm: Don't allow guest to make System space executable for M profile

[Peter Maydell]

For an M profile v7PMSA, the system space (0xe0000000 - 0xffffffff) can
never be executable, even if the guest tries to set the MPU registers
up that way. Enforce this restriction.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ceef225..169c361 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8251,6 +8251,14 @@ static inline bool is_ppb_region(CPUARMState *env, uint32_t address)
         extract32(address, 20, 12) == 0xe00;
 }
 
+static inline bool is_system_region(CPUARMState *env, uint32_t address)
+{
+    /* True if address is in the M profile system region
+     * 0xe0000000 - 0xffffffff
+     */
+    return arm_feature(env, ARM_FEATURE_M) && extract32(address, 29, 3) == 0x7;
+}
+
 static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                                  int access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
@@ -8354,6 +8362,12 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
             get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
         } else { /* a MPU hit! */
             uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
+            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
+
+            if (is_system_region(env, address)) {
+                /* System space is always execute never */
+                xn = 1;
+            }
 
             if (is_user) { /* User mode AP bit decoding */
                 switch (ap) {
@@ -8394,7 +8408,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
             }
 
             /* execute never */
-            if (env->pmsav7.dracr[n] & (1 << 12)) {
+            if (xn) {
                 *prot &= ~PAGE_EXEC;
             }
         }
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 3/5] target/arm: Rename cp15.c6_rgnr to pmsav7.rnr

[Peter Maydell]

Almost all of the PMSAv7 state is in the pmsav7 substruct of
the ARM CPU state structure. The exception is the region
number register, which is in cp15.c6_rgnr. This exception
is a bit odd for M profile, which otherwise generally does
not store state in the cp15 substruct.

Rename cp15.c6_rgnr to pmsav7.rnr accordingly.

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

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 26a4b2d..323e2d4 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -536,13 +536,13 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
     case 0xd94: /* MPU_CTRL */
         return cpu->env.v7m.mpu_ctrl;
     case 0xd98: /* MPU_RNR */
-        return cpu->env.cp15.c6_rgnr;
+        return cpu->env.pmsav7.rnr;
     case 0xd9c: /* MPU_RBAR */
     case 0xda4: /* MPU_RBAR_A1 */
     case 0xdac: /* MPU_RBAR_A2 */
     case 0xdb4: /* MPU_RBAR_A3 */
     {
-        int region = cpu->env.cp15.c6_rgnr;
+        int region = cpu->env.pmsav7.rnr;
 
         if (region >= cpu->pmsav7_dregion) {
             return 0;
@@ -554,7 +554,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
     case 0xdb0: /* MPU_RASR_A2 */
     case 0xdb8: /* MPU_RASR_A3 */
     {
-        int region = cpu->env.cp15.c6_rgnr;
+        int region = cpu->env.pmsav7.rnr;
 
         if (region >= cpu->pmsav7_dregion) {
             return 0;
@@ -681,7 +681,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
                           PRIu32 "/%" PRIu32 "\n",
                           value, cpu->pmsav7_dregion);
         } else {
-            cpu->env.cp15.c6_rgnr = value;
+            cpu->env.pmsav7.rnr = value;
         }
         break;
     case 0xd9c: /* MPU_RBAR */
@@ -702,9 +702,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
                               region, cpu->pmsav7_dregion);
                 return;
             }
-            cpu->env.cp15.c6_rgnr = region;
+            cpu->env.pmsav7.rnr = region;
         } else {
-            region = cpu->env.cp15.c6_rgnr;
+            region = cpu->env.pmsav7.rnr;
         }
 
         if (region >= cpu->pmsav7_dregion) {
@@ -720,7 +720,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
     case 0xdb0: /* MPU_RASR_A2 */
     case 0xdb8: /* MPU_RASR_A3 */
     {
-        int region = cpu->env.cp15.c6_rgnr;
+        int region = cpu->env.pmsav7.rnr;
 
         if (region >= cpu->pmsav7_dregion) {
             return;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 102c58a..b39d64a 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -305,8 +305,6 @@ typedef struct CPUARMState {
             uint64_t par_el[4];
         };
 
-        uint32_t c6_rgnr;
-
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint64_t c9_pmcr; /* performance monitor control register */
@@ -519,6 +517,7 @@ typedef struct CPUARMState {
         uint32_t *drbar;
         uint32_t *drsr;
         uint32_t *dracr;
+        uint32_t rnr;
     } pmsav7;
 
     void *nvic;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 169c361..63187de 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2385,7 +2385,7 @@ static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
         return 0;
     }
 
-    u32p += env->cp15.c6_rgnr;
+    u32p += env->pmsav7.rnr;
     return *u32p;
 }
 
@@ -2399,7 +2399,7 @@ static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
-    u32p += env->cp15.c6_rgnr;
+    u32p += env->pmsav7.rnr;
     tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
     *u32p = value;
 }
@@ -2447,7 +2447,7 @@ static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
       .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
     { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
       .access = PL1_RW,
-      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr),
+      .fieldoffset = offsetof(CPUARMState, pmsav7.rnr),
       .writefn = pmsav7_rgnr_write },
     REGINFO_SENTINEL
 };
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 1a40469..93c1a78 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -151,7 +151,7 @@ static bool pmsav7_rgnr_vmstate_validate(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
 
-    return cpu->env.cp15.c6_rgnr < cpu->pmsav7_dregion;
+    return cpu->env.pmsav7.rnr < cpu->pmsav7_dregion;
 }
 
 static const VMStateDescription vmstate_pmsav7 = {
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 4/5] target/arm: Move PMSAv7 reset into arm_cpu_reset() so M profile MPUs get reset

[Peter Maydell]

When the PMSAv7 implementation was originally added it was for R profile
CPUs only, and reset was handled using the cpreg .resetfn hooks.
Unfortunately for M profile cores this doesn't work, because they do
not register any cpregs. Move the reset handling into arm_cpu_reset(),
where it will work for both R profile and M profile cores.

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

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 96d1f84..05c038b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -232,6 +232,20 @@ static void arm_cpu_reset(CPUState *s)
 
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
 #endif
+
+    if (arm_feature(env, ARM_FEATURE_PMSA) &&
+        arm_feature(env, ARM_FEATURE_V7)) {
+        if (cpu->pmsav7_dregion > 0) {
+            memset(env->pmsav7.drbar, 0,
+                   sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion);
+            memset(env->pmsav7.drsr, 0,
+                   sizeof(*env->pmsav7.drsr) * cpu->pmsav7_dregion);
+            memset(env->pmsav7.dracr, 0,
+                   sizeof(*env->pmsav7.dracr) * cpu->pmsav7_dregion);
+        }
+        env->pmsav7.rnr = 0;
+    }
+
     set_flush_to_zero(1, &env->vfp.standard_fp_status);
     set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
     set_default_nan_mode(1, &env->vfp.standard_fp_status);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 63187de..a9247e9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2404,18 +2404,6 @@ static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
     *u32p = value;
 }
 
-static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
-{
-    ARMCPU *cpu = arm_env_get_cpu(env);
-    uint32_t *u32p = *(uint32_t **)raw_ptr(env, ri);
-
-    if (!u32p) {
-        return;
-    }
-
-    memset(u32p, 0, sizeof(*u32p) * cpu->pmsav7_dregion);
-}
-
 static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                               uint64_t value)
 {
@@ -2433,22 +2421,30 @@ static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 
 static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
+    /* Reset for all these registers is handled in arm_cpu_reset(),
+     * because the PMSAv7 is also used by M-profile CPUs, which do
+     * not register cpregs but still need the state to be reset.
+     */
     { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_NO_RAW,
       .fieldoffset = offsetof(CPUARMState, pmsav7.drbar),
-      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+      .readfn = pmsav7_read, .writefn = pmsav7_write,
+      .resetfn = arm_cp_reset_ignore },
     { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
       .access = PL1_RW, .type = ARM_CP_NO_RAW,
       .fieldoffset = offsetof(CPUARMState, pmsav7.drsr),
-      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+      .readfn = pmsav7_read, .writefn = pmsav7_write,
+      .resetfn = arm_cp_reset_ignore },
     { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
       .access = PL1_RW, .type = ARM_CP_NO_RAW,
       .fieldoffset = offsetof(CPUARMState, pmsav7.dracr),
-      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
+      .readfn = pmsav7_read, .writefn = pmsav7_write,
+      .resetfn = arm_cp_reset_ignore },
     { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
       .access = PL1_RW,
       .fieldoffset = offsetof(CPUARMState, pmsav7.rnr),
-      .writefn = pmsav7_rgnr_write },
+      .writefn = pmsav7_rgnr_write,
+      .resetfn = arm_cp_reset_ignore },
     REGINFO_SENTINEL
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH for-2.10 5/5] target/arm: Migrate MPU_RNR register state for M profile cores

[Peter Maydell]

The PMSAv7 region number register is migrated for R profile
cores using the cpreg scheme, but M profile doesn't use
cpregs, and so we weren't migrating the MPU_RNR register state
at all. Fix that by adding a migration subsection for the
M profile case.

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

diff --git a/target/arm/machine.c b/target/arm/machine.c
index 93c1a78..1f66da4 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -171,6 +171,29 @@ static const VMStateDescription vmstate_pmsav7 = {
     }
 };
 
+static bool pmsav7_rnr_needed(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    CPUARMState *env = &cpu->env;
+
+    /* For R profile cores pmsav7.rnr is migrated via the cpreg
+     * "RGNR" definition in helper.h. For M profile we have to
+     * migrate it separately.
+     */
+    return arm_feature(env, ARM_FEATURE_M);
+}
+
+static const VMStateDescription vmstate_pmsav7_rnr = {
+    .name = "cpu/pmsav7-rnr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pmsav7_rnr_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(env.pmsav7.rnr, ARMCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
                     VMStateField *field)
 {
@@ -377,6 +400,11 @@ const VMStateDescription vmstate_arm_cpu = {
         &vmstate_iwmmxt,
         &vmstate_m,
         &vmstate_thumb2ee,
+        /* pmsav7_rnr must come before pmsav7 so that we have the
+         * region number before we test it in the VMSTATE_VALIDATE
+         * in vmstate_pmsav7.
+         */
+        &vmstate_pmsav7_rnr,
         &vmstate_pmsav7,
         NULL
     }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 3/5] target/arm: Rename cp15.c6_rgnr to pmsav7.rnr

[Philippe Mathieu-Daudé]

Hi Peter,

you missed some rgnr -> rnr :|

On 07/27/2017 07:59 AM, Peter Maydell wrote:
> Almost all of the PMSAv7 state is in the pmsav7 substruct of
> the ARM CPU state structure. The exception is the region
> number register, which is in cp15.c6_rgnr. This exception
> is a bit odd for M profile, which otherwise generally does
> not store state in the cp15 substruct.
> 
> Rename cp15.c6_rgnr to pmsav7.rnr accordingly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/intc/armv7m_nvic.c | 14 +++++++-------
>   target/arm/cpu.h      |  3 +--
>   target/arm/helper.c   |  6 +++---
>   target/arm/machine.c  |  2 +-
>   4 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 26a4b2d..323e2d4 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -536,13 +536,13 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
>       case 0xd94: /* MPU_CTRL */
>           return cpu->env.v7m.mpu_ctrl;
>       case 0xd98: /* MPU_RNR */
> -        return cpu->env.cp15.c6_rgnr;
> +        return cpu->env.pmsav7.rnr;
>       case 0xd9c: /* MPU_RBAR */
>       case 0xda4: /* MPU_RBAR_A1 */
>       case 0xdac: /* MPU_RBAR_A2 */
>       case 0xdb4: /* MPU_RBAR_A3 */
>       {
> -        int region = cpu->env.cp15.c6_rgnr;
> +        int region = cpu->env.pmsav7.rnr;
>   
>           if (region >= cpu->pmsav7_dregion) {
>               return 0;
> @@ -554,7 +554,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
>       case 0xdb0: /* MPU_RASR_A2 */
>       case 0xdb8: /* MPU_RASR_A3 */
>       {
> -        int region = cpu->env.cp15.c6_rgnr;
> +        int region = cpu->env.pmsav7.rnr;
>   
>           if (region >= cpu->pmsav7_dregion) {
>               return 0;
> @@ -681,7 +681,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>                             PRIu32 "/%" PRIu32 "\n",
>                             value, cpu->pmsav7_dregion);
>           } else {
> -            cpu->env.cp15.c6_rgnr = value;
> +            cpu->env.pmsav7.rnr = value;
>           }
>           break;
>       case 0xd9c: /* MPU_RBAR */
> @@ -702,9 +702,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>                                 region, cpu->pmsav7_dregion);
>                   return;
>               }
> -            cpu->env.cp15.c6_rgnr = region;
> +            cpu->env.pmsav7.rnr = region;
>           } else {
> -            region = cpu->env.cp15.c6_rgnr;
> +            region = cpu->env.pmsav7.rnr;
>           }
>   
>           if (region >= cpu->pmsav7_dregion) {
> @@ -720,7 +720,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>       case 0xdb0: /* MPU_RASR_A2 */
>       case 0xdb8: /* MPU_RASR_A3 */
>       {
> -        int region = cpu->env.cp15.c6_rgnr;
> +        int region = cpu->env.pmsav7.rnr;
>   
>           if (region >= cpu->pmsav7_dregion) {
>               return;
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 102c58a..b39d64a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -305,8 +305,6 @@ typedef struct CPUARMState {
>               uint64_t par_el[4];
>           };
>   
> -        uint32_t c6_rgnr;
> -
>           uint32_t c9_insn; /* Cache lockdown registers.  */
>           uint32_t c9_data;
>           uint64_t c9_pmcr; /* performance monitor control register */
> @@ -519,6 +517,7 @@ typedef struct CPUARMState {
>           uint32_t *drbar;
>           uint32_t *drsr;
>           uint32_t *dracr;
> +        uint32_t rnr;
>       } pmsav7;
>   
>       void *nvic;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 169c361..63187de 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2385,7 +2385,7 @@ static uint64_t pmsav7_read(CPUARMState *env, const ARMCPRegInfo *ri)
>           return 0;
>       }
>   
> -    u32p += env->cp15.c6_rgnr;
> +    u32p += env->pmsav7.rnr;
>       return *u32p;
>   }
>   
> @@ -2399,7 +2399,7 @@ static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
>           return;
>       }
>   
> -    u32p += env->cp15.c6_rgnr;
> +    u32p += env->pmsav7.rnr;
>       tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
>       *u32p = value;
>   }
> @@ -2447,7 +2447,7 @@ static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>         .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
>       { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,

"RGNR" -> "RNR"

>         .access = PL1_RW,
> -      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr),
> +      .fieldoffset = offsetof(CPUARMState, pmsav7.rnr),
>         .writefn = pmsav7_rgnr_write },

pmsav7_rnr_write

>       REGINFO_SENTINEL
>   };
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 1a40469..93c1a78 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -151,7 +151,7 @@ static bool pmsav7_rgnr_vmstate_validate(void *opaque, int version_id)

pmsav7_rnr_vmstate_validate()

>   {
>       ARMCPU *cpu = opaque;
>   
> -    return cpu->env.cp15.c6_rgnr < cpu->pmsav7_dregion;
> +    return cpu->env.pmsav7.rnr < cpu->pmsav7_dregion;
>   }
>   
>   static const VMStateDescription vmstate_pmsav7 = {
> 
[...]
         VMSTATE_VALIDATE("rgnr is valid", pmsav7_rgnr_vmstate_validate),

also this one "rnr is valid" ^^^

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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 5/5] target/arm: Migrate MPU_RNR register state for M profile cores

[Philippe Mathieu-Daudé]

On 07/27/2017 07:59 AM, Peter Maydell wrote:
> The PMSAv7 region number register is migrated for R profile
> cores using the cpreg scheme, but M profile doesn't use
> cpregs, and so we weren't migrating the MPU_RNR register state
> at all. Fix that by adding a migration subsection for the
> M profile case.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>   target/arm/machine.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 93c1a78..1f66da4 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -171,6 +171,29 @@ static const VMStateDescription vmstate_pmsav7 = {
>       }
>   };
>   
> +static bool pmsav7_rnr_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
> +
> +    /* For R profile cores pmsav7.rnr is migrated via the cpreg
> +     * "RGNR" definition in helper.h. For M profile we have to
> +     * migrate it separately.
> +     */
> +    return arm_feature(env, ARM_FEATURE_M);
> +}
> +
> +static const VMStateDescription vmstate_pmsav7_rnr = {
> +    .name = "cpu/pmsav7-rnr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pmsav7_rnr_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(env.pmsav7.rnr, ARMCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
>                       VMStateField *field)
>   {
> @@ -377,6 +400,11 @@ const VMStateDescription vmstate_arm_cpu = {
>           &vmstate_iwmmxt,
>           &vmstate_m,
>           &vmstate_thumb2ee,
> +        /* pmsav7_rnr must come before pmsav7 so that we have the
> +         * region number before we test it in the VMSTATE_VALIDATE
> +         * in vmstate_pmsav7.
> +         */
> +        &vmstate_pmsav7_rnr,
>           &vmstate_pmsav7,
>           NULL
>       }
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 3/5] target/arm: Rename cp15.c6_rgnr to pmsav7.rnr

[Philippe Mathieu-Daudé]

On 07/27/2017 07:43 PM, Philippe Mathieu-Daudé wrote:
> On 07/27/2017 07:59 AM, Peter Maydell wrote:
[...]
>> -    u32p += env->cp15.c6_rgnr;
>> +    u32p += env->pmsav7.rnr;
>>       tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
>>       *u32p = value;
>>   }
>> @@ -2447,7 +2447,7 @@ static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>>         .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = 
>> pmsav7_reset },
>>       { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 
>> = 0,
> 
> "RGNR" -> "RNR"

Ah "RGNR" for -R and "RNR" for -M hmmm... still better keep the name 
matching the field, "rnr".

> 
>>         .access = PL1_RW,
>> -      .fieldoffset = offsetof(CPUARMState, cp15.c6_rgnr),
>> +      .fieldoffset = offsetof(CPUARMState, pmsav7.rnr),
>>         .writefn = pmsav7_rgnr_write },
> 
> pmsav7_rnr_write
> 
>>       REGINFO_SENTINEL
>>   };
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 1a40469..93c1a78 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -151,7 +151,7 @@ static bool pmsav7_rgnr_vmstate_validate(void 
>> *opaque, int version_id)
> 
> pmsav7_rnr_vmstate_validate()
> 
>>   {
>>       ARMCPU *cpu = opaque;
>> -    return cpu->env.cp15.c6_rgnr < cpu->pmsav7_dregion;
>> +    return cpu->env.pmsav7.rnr < cpu->pmsav7_dregion;
>>   }
>>   static const VMStateDescription vmstate_pmsav7 = {
>>
> [...]
>          VMSTATE_VALIDATE("rgnr is valid", pmsav7_rgnr_vmstate_validate),
> 
> also this one "rnr is valid" ^^^
> 
> once fixed:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 1/5] target/arm: Don't do MPU lookups for addresses in M profile PPB region

[Philippe Mathieu-Daudé]

On 07/27/2017 07:59 AM, Peter Maydell wrote:
> The M profile PMSAv7 specification says that if the address being looked
> up is in the PPB region (0xe0000000 - 0xe00fffff) then we do not use
> the MPU regions but always use the default memory map. Implement this
> (we were previously behaving like an R profile PMSAv7, which does not
> special case this).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 4ed32c5..ceef225 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8244,6 +8244,13 @@ static bool pmsav7_use_background_region(ARMCPU *cpu,
>       }
>   }
>   
> +static inline bool is_ppb_region(CPUARMState *env, uint32_t address)
> +{
> +    /* True if address is in the M profile PPB region 0xe0000000 - 0xe00fffff */
> +    return arm_feature(env, ARM_FEATURE_M) &&
> +        extract32(address, 20, 12) == 0xe00;
> +}
> +
>   static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                                    int access_type, ARMMMUIdx mmu_idx,
>                                    hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> @@ -8255,7 +8262,15 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>       *phys_ptr = address;
>       *prot = 0;
>   
> -    if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> +    if (regime_translation_disabled(env, mmu_idx) ||
> +        is_ppb_region(env, address)) {
> +        /* MPU disabled or M profile PPB access: use default memory map.
> +         * The other case which uses the default memory map in the
> +         * v7M ARM ARM pseudocode is exception vector reads from the vector
> +         * table. In QEMU those accesses are done in arm_v7m_load_vector(),
> +         * which always does a direct read using address_space_ldl(), rather
> +         * than going via this function, so we don't need to check that here.
> +         */

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

>           get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>       } else { /* MPU enabled */
>           for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) {
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 2/5] target/arm: Don't allow guest to make System space executable for M profile

[Philippe Mathieu-Daudé]

Hi Peter,

On 07/27/2017 07:59 AM, Peter Maydell wrote:
> For an M profile v7PMSA, the system space (0xe0000000 - 0xffffffff) can
> never be executable, even if the guest tries to set the MPU registers
> up that way. Enforce this restriction.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/helper.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ceef225..169c361 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8251,6 +8251,14 @@ static inline bool is_ppb_region(CPUARMState *env, uint32_t address)
>           extract32(address, 20, 12) == 0xe00;
>   }
>   

I wonder if these should renamed pmsav7_is_ppb_region() and 
pmsav7_is_system_region().

> +static inline bool is_system_region(CPUARMState *env, uint32_t address)
> +{
> +    /* True if address is in the M profile system region
> +     * 0xe0000000 - 0xffffffff
> +     */
> +    return arm_feature(env, ARM_FEATURE_M) && extract32(address, 29, 3) == 0x7;
> +}
> +
>   static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                                    int access_type, ARMMMUIdx mmu_idx,
>                                    hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> @@ -8354,6 +8362,12 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>               get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>           } else { /* a MPU hit! */
>               uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);

Maybe names access_perms/execute_never are easier to read..

> +            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
> +

clear MemManage exceptions:

                *fsr &= ~0xff;

> +            if (is_system_region(env, address)) {
> +                /* System space is always execute never */
> +                xn = 1;

                } else {
                    xn = extract32(env->pmsav7.dracr[n], 12, 1);

> +            }
>   
>               if (is_user) { /* User mode AP bit decoding */
>                   switch (ap) {
> @@ -8394,7 +8408,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>               }
>   
>               /* execute never */
> -            if (env->pmsav7.dracr[n] & (1 << 12)) {
> +            if (xn) {
>                   *prot &= ~PAGE_EXEC;

and here we now can set eXecuteNever violation:

		    *fsr |= R_V7M_CFSR_IACCVIOL_MASK;

>               }
>           }
> 
     }
     *fsr = 0x00d; /* Permission fault */

I don't understand this mask, I don't have bit [2] defined in my 
datashit, maybe it was expected to turn on exception Entry/Return which 
I have defined as bits 4 and 3 respectively, so I'd rather see here:

     *fsr |= R_V7M_CFSR_MUNSTKERR_MASK | R_V7M_CFSR_MSTKERR_MASK;

What do you think?

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 4/5] target/arm: Move PMSAv7 reset into arm_cpu_reset() so M profile MPUs get reset

[Philippe Mathieu-Daudé]

On 07/27/2017 07:59 AM, Peter Maydell wrote:
> When the PMSAv7 implementation was originally added it was for R profile
> CPUs only, and reset was handled using the cpreg .resetfn hooks.
> Unfortunately for M profile cores this doesn't work, because they do
> not register any cpregs. Move the reset handling into arm_cpu_reset(),
> where it will work for both R profile and M profile cores.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>   target/arm/cpu.c    | 14 ++++++++++++++
>   target/arm/helper.c | 28 ++++++++++++----------------
>   2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 96d1f84..05c038b 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -232,6 +232,20 @@ static void arm_cpu_reset(CPUState *s)
>   
>       env->vfp.xregs[ARM_VFP_FPEXC] = 0;
>   #endif
> +
> +    if (arm_feature(env, ARM_FEATURE_PMSA) &&
> +        arm_feature(env, ARM_FEATURE_V7)) {
> +        if (cpu->pmsav7_dregion > 0) {
> +            memset(env->pmsav7.drbar, 0,
> +                   sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion);
> +            memset(env->pmsav7.drsr, 0,
> +                   sizeof(*env->pmsav7.drsr) * cpu->pmsav7_dregion);
> +            memset(env->pmsav7.dracr, 0,
> +                   sizeof(*env->pmsav7.dracr) * cpu->pmsav7_dregion);
> +        }
> +        env->pmsav7.rnr = 0;
> +    }
> +
>       set_flush_to_zero(1, &env->vfp.standard_fp_status);
>       set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
>       set_default_nan_mode(1, &env->vfp.standard_fp_status);
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 63187de..a9247e9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2404,18 +2404,6 @@ static void pmsav7_write(CPUARMState *env, const ARMCPRegInfo *ri,
>       *u32p = value;
>   }
>   
> -static void pmsav7_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> -{
> -    ARMCPU *cpu = arm_env_get_cpu(env);
> -    uint32_t *u32p = *(uint32_t **)raw_ptr(env, ri);
> -
> -    if (!u32p) {
> -        return;
> -    }
> -
> -    memset(u32p, 0, sizeof(*u32p) * cpu->pmsav7_dregion);
> -}
> -
>   static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                                 uint64_t value)
>   {
> @@ -2433,22 +2421,30 @@ static void pmsav7_rgnr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>   }
>   
>   static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
> +    /* Reset for all these registers is handled in arm_cpu_reset(),
> +     * because the PMSAv7 is also used by M-profile CPUs, which do
> +     * not register cpregs but still need the state to be reset.
> +     */
>       { .name = "DRBAR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 0,
>         .access = PL1_RW, .type = ARM_CP_NO_RAW,
>         .fieldoffset = offsetof(CPUARMState, pmsav7.drbar),
> -      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +      .readfn = pmsav7_read, .writefn = pmsav7_write,
> +      .resetfn = arm_cp_reset_ignore },
>       { .name = "DRSR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 2,
>         .access = PL1_RW, .type = ARM_CP_NO_RAW,
>         .fieldoffset = offsetof(CPUARMState, pmsav7.drsr),
> -      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +      .readfn = pmsav7_read, .writefn = pmsav7_write,
> +      .resetfn = arm_cp_reset_ignore },
>       { .name = "DRACR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 1, .opc2 = 4,
>         .access = PL1_RW, .type = ARM_CP_NO_RAW,
>         .fieldoffset = offsetof(CPUARMState, pmsav7.dracr),
> -      .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn = pmsav7_reset },
> +      .readfn = pmsav7_read, .writefn = pmsav7_write,
> +      .resetfn = arm_cp_reset_ignore },
>       { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 = 0,
>         .access = PL1_RW,
>         .fieldoffset = offsetof(CPUARMState, pmsav7.rnr),
> -      .writefn = pmsav7_rgnr_write },
> +      .writefn = pmsav7_rgnr_write,
> +      .resetfn = arm_cp_reset_ignore },
>       REGINFO_SENTINEL
>   };
>   
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 3/5] target/arm: Rename cp15.c6_rgnr to pmsav7.rnr

[Peter Maydell]

On 27 July 2017 at 23:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 07/27/2017 07:43 PM, Philippe Mathieu-Daudé wrote:
>>
>> On 07/27/2017 07:59 AM, Peter Maydell wrote:
>
> [...]
>>>
>>> -    u32p += env->cp15.c6_rgnr;
>>> +    u32p += env->pmsav7.rnr;
>>>       tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
>>>       *u32p = value;
>>>   }
>>> @@ -2447,7 +2447,7 @@ static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>>>         .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn =
>>> pmsav7_reset },
>>>       { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 =
>>> 0,
>>
>>
>> "RGNR" -> "RNR"
>
>
> Ah "RGNR" for -R and "RNR" for -M hmmm... still better keep the name
> matching the field, "rnr".

It's a bit awkward, yes -- we're going to get a mismatch one way
or the other.

In this patch I wanted only to change the field name, not
anything else (it's already a bit borderline for 2.10).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 2/5] target/arm: Don't allow guest to make System space executable for M profile

[Peter Maydell]

On 28 July 2017 at 00:59, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 07/27/2017 07:59 AM, Peter Maydell wrote:
>>
>> For an M profile v7PMSA, the system space (0xe0000000 - 0xffffffff) can
>> never be executable, even if the guest tries to set the MPU registers
>> up that way. Enforce this restriction.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   target/arm/helper.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index ceef225..169c361 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -8251,6 +8251,14 @@ static inline bool is_ppb_region(CPUARMState *env,
>> uint32_t address)
>>           extract32(address, 20, 12) == 0xe00;
>>   }
>>
>
>
> I wonder if these should renamed pmsav7_is_ppb_region() and
> pmsav7_is_system_region().

Yeah, perhaps better; I'm never quite sure how much disambiguation
to put in to file-local function names. Maybe m_is_ppb_region()?
PPB and system region are M profile concepts, not PMSAv7 ones.
That doesn't seem any clearer than where we started though :-(

>> +static inline bool is_system_region(CPUARMState *env, uint32_t address)
>> +{
>> +    /* True if address is in the M profile system region
>> +     * 0xe0000000 - 0xffffffff
>> +     */
>> +    return arm_feature(env, ARM_FEATURE_M) && extract32(address, 29, 3)
>> == 0x7;
>> +}
>> +
>>   static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>                                    int access_type, ARMMMUIdx mmu_idx,
>>                                    hwaddr *phys_ptr, int *prot, uint32_t
>> *fsr)
>> @@ -8354,6 +8362,12 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
>> uint32_t address,
>>               get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>           } else { /* a MPU hit! */
>>               uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
>
>
> Maybe names access_perms/execute_never are easier to read..

Following existing practice in the LPAE code, we use the
field names that the architecture spec uses.

>> +            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
>> +
>
>
> clear MemManage exceptions:
>
>                *fsr &= ~0xff;

>> +            if (is_system_region(env, address)) {
>> +                /* System space is always execute never */
>> +                xn = 1;
>
>
>                } else {
>                    xn = extract32(env->pmsav7.dracr[n], 12, 1);
>
>> +            }
>>                 if (is_user) { /* User mode AP bit decoding */
>>                   switch (ap) {
>> @@ -8394,7 +8408,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
>> uint32_t address,
>>               }
>>                 /* execute never */
>> -            if (env->pmsav7.dracr[n] & (1 << 12)) {
>> +            if (xn) {
>>                   *prot &= ~PAGE_EXEC;
>
>
> and here we now can set eXecuteNever violation:
>
>                     *fsr |= R_V7M_CFSR_IACCVIOL_MASK;

No, *fsr is not an M profile CFSR, it's an A/R profile short
descriptor format fault status value (because on R profile
that's what it will be used as, and M profile is using the
same MPU handling code here). We do the conversion in
arm_v7m_cpu_do_interrupt(), where we look at the exception_index
and the exception.fsr to identify what CFSR bits to set.

>>               }
>>           }
>>
>     }
>     *fsr = 0x00d; /* Permission fault */
>
> I don't understand this mask, I don't have bit [2] defined in my datashit,
> maybe it was expected to turn on exception Entry/Return which I have defined
> as bits 4 and 3 respectively, so I'd rather see here:
>
>     *fsr |= R_V7M_CFSR_MUNSTKERR_MASK | R_V7M_CFSR_MSTKERR_MASK;

See above, *fsr isn't a v7m CFSR.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 2/5] target/arm: Don't allow guest to make System space executable for M profile

[Philippe Mathieu-Daudé]

On 07/28/2017 05:51 AM, Peter Maydell wrote:
> On 28 July 2017 at 00:59, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Peter,
>>
>> On 07/27/2017 07:59 AM, Peter Maydell wrote:
>>>
>>> For an M profile v7PMSA, the system space (0xe0000000 - 0xffffffff) can
>>> never be executable, even if the guest tries to set the MPU registers
>>> up that way. Enforce this restriction.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    target/arm/helper.c | 16 +++++++++++++++-
>>>    1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>> index ceef225..169c361 100644
>>> --- a/target/arm/helper.c
>>> +++ b/target/arm/helper.c
>>> @@ -8251,6 +8251,14 @@ static inline bool is_ppb_region(CPUARMState *env,
>>> uint32_t address)
>>>            extract32(address, 20, 12) == 0xe00;
>>>    }
>>>
>>
>>
>> I wonder if these should renamed pmsav7_is_ppb_region() and
>> pmsav7_is_system_region().
> 
> Yeah, perhaps better; I'm never quite sure how much disambiguation
> to put in to file-local function names. Maybe m_is_ppb_region()?
> PPB and system region are M profile concepts, not PMSAv7 ones.
> That doesn't seem any clearer than where we started though :-(

m_is_ppb_region() isn't bad.

> 
>>> +static inline bool is_system_region(CPUARMState *env, uint32_t address)
>>> +{
>>> +    /* True if address is in the M profile system region
>>> +     * 0xe0000000 - 0xffffffff
>>> +     */
>>> +    return arm_feature(env, ARM_FEATURE_M) && extract32(address, 29, 3)
>>> == 0x7;
>>> +}
>>> +
>>>    static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>>                                     int access_type, ARMMMUIdx mmu_idx,
>>>                                     hwaddr *phys_ptr, int *prot, uint32_t
>>> *fsr)
>>> @@ -8354,6 +8362,12 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
>>> uint32_t address,
>>>                get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>>>            } else { /* a MPU hit! */
>>>                uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3);
>>
>>
>> Maybe names access_perms/execute_never are easier to read..
> 
> Following existing practice in the LPAE code, we use the
> field names that the architecture spec uses.

I see, but below xn has an helpful comment /* execute never */ that 
eases code review, maybe add both comment on declaration.

> 
>>> +            uint32_t xn = extract32(env->pmsav7.dracr[n], 12, 1);
>>> +
>>
>>
>> clear MemManage exceptions:
>>
>>                 *fsr &= ~0xff;
> 
>>> +            if (is_system_region(env, address)) {
>>> +                /* System space is always execute never */
>>> +                xn = 1;
>>
>>
>>                 } else {
>>                     xn = extract32(env->pmsav7.dracr[n], 12, 1);
>>
>>> +            }
>>>                  if (is_user) { /* User mode AP bit decoding */
>>>                    switch (ap) {
>>> @@ -8394,7 +8408,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
>>> uint32_t address,
>>>                }
>>>                  /* execute never */
>>> -            if (env->pmsav7.dracr[n] & (1 << 12)) {
>>> +            if (xn) {
>>>                    *prot &= ~PAGE_EXEC;
>>
>>
>> and here we now can set eXecuteNever violation:
>>
>>                      *fsr |= R_V7M_CFSR_IACCVIOL_MASK;
> 
> No, *fsr is not an M profile CFSR, it's an A/R profile short
> descriptor format fault status value (because on R profile
> that's what it will be used as, and M profile is using the
> same MPU handling code here). We do the conversion in
> arm_v7m_cpu_do_interrupt(), where we look at the exception_index
> and the exception.fsr to identify what CFSR bits to set.
> 

Ok I missed that, thank for correcting me.

>>>                }
>>>            }
>>>
>>      }
>>      *fsr = 0x00d; /* Permission fault */
>>
>> I don't understand this mask, I don't have bit [2] defined in my datashit,
>> maybe it was expected to turn on exception Entry/Return which I have defined
>> as bits 4 and 3 respectively, so I'd rather see here:
>>
>>      *fsr |= R_V7M_CFSR_MUNSTKERR_MASK | R_V7M_CFSR_MSTKERR_MASK;
> 
> See above, *fsr isn't a v7m CFSR.

Yes, 0x00d is Permission fault using short-descriptor translation.

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

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 3/5] target/arm: Rename cp15.c6_rgnr to pmsav7.rnr

[Philippe Mathieu-Daudé]

On 07/28/2017 05:42 AM, Peter Maydell wrote:
> On 27 July 2017 at 23:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 07/27/2017 07:43 PM, Philippe Mathieu-Daudé wrote:
>>>
>>> On 07/27/2017 07:59 AM, Peter Maydell wrote:
>>
>> [...]
>>>>
>>>> -    u32p += env->cp15.c6_rgnr;
>>>> +    u32p += env->pmsav7.rnr;
>>>>        tlb_flush(CPU(cpu)); /* Mappings may have changed - purge! */
>>>>        *u32p = value;
>>>>    }
>>>> @@ -2447,7 +2447,7 @@ static const ARMCPRegInfo pmsav7_cp_reginfo[] = {
>>>>          .readfn = pmsav7_read, .writefn = pmsav7_write, .resetfn =
>>>> pmsav7_reset },
>>>>        { .name = "RGNR", .cp = 15, .crn = 6, .opc1 = 0, .crm = 2, .opc2 =
>>>> 0,
>>>
>>>
>>> "RGNR" -> "RNR"
>>
>>
>> Ah "RGNR" for -R and "RNR" for -M hmmm... still better keep the name
>> matching the field, "rnr".
> 
> It's a bit awkward, yes -- we're going to get a mismatch one way
> or the other.
> 
> In this patch I wanted only to change the field name, not
> anything else (it's already a bit borderline for 2.10).

Fine by me for what's worth. So either ways:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.10 0/5] M profile MPU bugfixes

[Peter Maydell]

On 27 July 2017 at 11:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes some bugs in the M profile MPU code:
>  * the guest shouldn't be able to make system space executable
>  * PPB region accesses should not be subject to MPU lookups
>  * we were not resetting the PMSAv7 MPU state for M profile CPUs
>  * we weren't migrating the MPU_RNR state
>
> The renaming from cp15.c6_rgnr to pmsav7.rnr is not strictly
> necessary for 2.10, but it doesn't affect many places in the code
> and it restores the invariant that no mutable M profile CPU
> state is stored in env->cp15.something fields.

Applied to target-arm queue for 2.10, with the minor function
renames (s/is_/m_is_/) suggested by Philippe.

thanks
-- PMM