Re: [PATCH RFC v2 01/30] target/arm: support Cortex-M0+ MPU

["Alex Bennée" <alex.bennee@linaro.org>]

Gilles Grimaud <gilles.grimaud@univ-lille.fr> writes:

> From: gilles grimaud <gilles.grimaud@univ-lille.fr>
>
> Cortex-M0+ implementations may provide an Armv6 PMSA MPU with eight regions. QEMU already marks M-profile CPUs as PMSA, but only exposes and allocates the existing region model for Armv7 CPUs. This prevents a Cortex-M0+ board from enabling its optional MPU.
>

Just a comment for all patches, you should ensure your editor wraps at
80 columns (72 for title) as patches are often read on terminals.

It's good habit to run through checkpatch.pl to pick up the minor
oft-forgotten stuff.

> Use QEMU's PMSAv7 state and translation path for the compatible
> Armv6-M MPU register layout. Permit CONTROL.nPRIV changes when the
> optional MPU is present and clear the region state on reset.

Hmm will need to think about this - the file is explicitly targeting the
armv7_nvic so we might want to consider renaming if indeed special
casing the differences is the right thing to do.

That does make the spread of changes about both nvic and cpu hard to
review. Is env->pmsav7.drbar related to env.v7m.vecbase[!attrs.secure]
for example.

>
> Signed-off-by: gilles grimaud <gilles.grimaud@univ-lille.fr>
> ---
>  hw/intc/armv7m_nvic.c     |  3 +++
>  target/arm/cpu.c          | 12 +++++++++---
>  target/arm/ptw.c          |  3 ++-
>  target/arm/tcg/m_helper.c | 10 +++++++++-
>  4 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index a7651f831e..3140599ac1 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1626,6 +1626,9 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>          break;
>      case 0xd08: /* Vector Table Offset.  */
>          cpu->env.v7m.vecbase[attrs.secure] = value & 0xffffff80;
> +        if (!arm_feature(&cpu->env, ARM_FEATURE_M_SECURITY)) {
> +            cpu->env.v7m.vecbase[!attrs.secure] = value & 0xffffff80;
> +        }
>          break;
>      case 0xd0c: /* Application Interrupt/Reset Control (AIRCR) */
>          if ((value >> R_V7M_AIRCR_VECTKEY_SHIFT) == 0x05fa) {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 77aa78f00e..f58a1db843 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -617,7 +617,8 @@ static void arm_cpu_reset_hold(Object *obj, ResetType type)
>                             sizeof(*env->pmsav8.rlar[M_REG_S])
>                             * cpu->pmsav7_dregion);
>                  }
> -            } else if (arm_feature(env, ARM_FEATURE_V7)) {
> +            } else if (arm_feature(env, ARM_FEATURE_V7) ||
> +                       arm_feature(env, ARM_FEATURE_M)) {
>                  memset(env->pmsav7.drbar, 0,
>                         sizeof(*env->pmsav7.drbar) * cpu->pmsav7_dregion);
>                  memset(env->pmsav7.drsr, 0,
> @@ -1656,7 +1657,11 @@ static void arm_cpu_post_init(Object *obj)
>  #ifndef CONFIG_USER_ONLY
>      if (arm_feature(&cpu->env, ARM_FEATURE_PMSA)) {
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property);
> -        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
> +        /*
> +         * QEMU's PMSAv7 state also models the Armv6-M MPU register layout.
> +         */
> +        if (arm_feature(&cpu->env, ARM_FEATURE_V7) ||
> +            arm_feature(&cpu->env, ARM_FEATURE_M)) {
>              qdev_property_add_static(DEVICE(obj),
>                                       &arm_cpu_pmsav7_dregion_property);
>          }
> @@ -2332,7 +2337,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>      if (arm_feature(env, ARM_FEATURE_PMSA) &&
> -        arm_feature(env, ARM_FEATURE_V7)) {
> +        (arm_feature(env, ARM_FEATURE_V7) ||
> +         arm_feature(env, ARM_FEATURE_M))) {
>          uint32_t nr = cpu->pmsav7_dregion;
>  
>          if (nr > 0xff) {
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index a29de0385f..b366a8d3dd 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -3901,7 +3901,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env, S1Translate *ptw,
>              /* PMSAv8 */
>              ret = get_phys_addr_pmsav8(env, ptw, address, access_type,
>                                         result, fi);
> -        } else if (arm_feature(env, ARM_FEATURE_V7)) {
> +        } else if (arm_feature(env, ARM_FEATURE_V7) ||
> +                   arm_feature(env, ARM_FEATURE_M)) {
>              /* PMSAv7 */

And again by definition this is PMSAv7, not v6 - you'll note we have
plenty of places where we do check against ARM_FEATURE_V6 (or V6K).

>              ret = get_phys_addr_pmsav7(env, ptw, address, access_type,
>                                         result, fi);
> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> index f4ba93b291..667b2a3cbd 100644
> --- a/target/arm/tcg/m_helper.c
> +++ b/target/arm/tcg/m_helper.c
> @@ -16,6 +16,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
>  #include "exec/page-protection.h"
> +#include "exec/cputlb.h"
>  #ifdef CONFIG_TCG
>  #include "accel/tcg/cpu-ldst-common.h"
>  #include "semihosting/common-semi.h"
> @@ -2775,9 +2776,16 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>                             !arm_v7m_is_handler_mode(env))) {
>              write_v7m_control_spsel(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
>          }
> -        if (cur_el > 0 && arm_feature(env, ARM_FEATURE_M_MAIN)) {
> +        if (cur_el > 0 && (arm_feature(env, ARM_FEATURE_M_MAIN) ||
> +                           env_archcpu(env)->has_mpu)) {
> +            uint32_t old_control = env->v7m.control[env->v7m.secure];
> +
>              env->v7m.control[env->v7m.secure] &= ~R_V7M_CONTROL_NPRIV_MASK;
>              env->v7m.control[env->v7m.secure] |= val & R_V7M_CONTROL_NPRIV_MASK;
> +            if ((old_control ^ env->v7m.control[env->v7m.secure]) &
> +                R_V7M_CONTROL_NPRIV_MASK) {
> +                tlb_flush(env_cpu(env));
> +            }
>          }
>          if (cpu_isar_feature(aa32_vfp_simd, env_archcpu(env))) {
>              /*

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro