Re: [Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <address@hidden> wrote:

A commit message that wasn't just the one-line summary would
be nice. Sometimes a patch really is trivial enough that it's
not worth describing in more than just a single line, but
those situations are the exception rather than the rule.

> Signed-off-by: Alex Zuepke <address@hidden>
> ---
>  hw/intc/armv7m_nvic.c |  113 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/cpu.h      |    6 +++
>  target-arm/helper.c   |    7 ++-
>  target-arm/machine.c  |    1 +
>  4 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 369ef94..5820414 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -289,6 +289,36 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t 
> offset)
>          return 0x01111110;
>      case 0xd70: /* ISAR4.  */
>          return 0x01310102;
> +    case 0xd90: /* MPU type register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->pmsav7_dregion << 8;
> +    case 0xd94: /* MPU control register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.mpu_ctrl;
> +    case 0xd98: /* MPU_RNR.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.cp15.c6_rgnr;
> +    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
> +    case 0xda4: /* MPU_RBAR_A1.  */
> +    case 0xdac: /* MPU_RBAR_A2.  */
> +    case 0xdb4: /* MPU_RBAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            return 0;

QEMU coding style wants braces even for single-line if statements.
(You might find scripts/checkpatch.pl helpful for flagging up
some style violations.)

> +        val = cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr];
> +        val |= cpu->env.cp15.c6_rgnr;

You need to mask this, we only want the bottom 4 bits of
c6_rgnr.

> +        return val;
> +    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
> +    case 0xda8: /* MPU_RSAR_A1.  */
> +    case 0xdb0: /* MPU_RSAR_A2.  */
> +    case 0xdb8: /* MPU_RSAR_A3.  */

These are all "RASR", not "RSAR".


> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            return 0;
> +        val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
> +        val <<= 16;
> +        val |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +        return val;
>      /* TODO: Implement debug registers.  */
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", 
> offset);
> @@ -406,6 +436,57 @@ static void nvic_writel(nvic_state *s, uint32_t offset, 
> uint32_t value)
>          qemu_log_mask(LOG_UNIMP,
>                        "NVIC: AUX fault status registers unimplemented\n");
>          break;
> +    case 0xd94: /* MPU control register.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        cpu->env.v7m.mpu_ctrl = value & 0x7;
> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_ENABLE)
> +            cpu->env.cp15.sctlr_ns |= SCTLR_M;
> +        else
> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_M;
> +        /* TODO: mimic MPU_CTRL_HFNMIENA */

That will be interesting to implement.

> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_PRIVDEFENA)
> +            cpu->env.cp15.sctlr_ns |= SCTLR_BR;
> +        else
> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_BR;

This is kind of ugly. Wouldn't it be nicer to make the code
which tests for MMU-enabled and privdefena be M-profile aware?

> +        /* This may enable/disable the MMU, so do a TLB flush.  */
> +        tlb_flush(CPU(cpu), 1);
> +        break;
> +    case 0xd98: /* MPU_RNR.  */
> +        cpu = ARM_CPU(current_cpu);
> +        value &= 0xff;
> +        if (value < cpu->pmsav7_dregion)
> +            cpu->env.cp15.c6_rgnr = value;
> +        break;
> +    case 0xd9c: /* MPU_RBAR: MPU region base address register.  */
> +    case 0xda4: /* MPU_RBAR_A1.  */
> +    case 0xdac: /* MPU_RBAR_A2.  */
> +    case 0xdb4: /* MPU_RBAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if (value & 0x10) {
> +            /* region update */
> +            uint32_t region = value & 0x0f;
> +            if (region < cpu->pmsav7_dregion)
> +                cpu->env.cp15.c6_rgnr = region;
> +        }
> +        value &= ~0x1f;
> +        cpu->env.pmsav7.drbar[cpu->env.cp15.c6_rgnr] = value;
> +        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +        break;
> +    case 0xda0: /* MPU_RSAR: MPU region attribute and size register.  */
> +    case 0xda8: /* MPU_RSAR_A1.  */
> +    case 0xdb0: /* MPU_RSAR_A2.  */
> +    case 0xdb8: /* MPU_RSAR_A3.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] = value >> 16;
> +        cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr] = value & 0xffff;
> +        tlb_flush(CPU(cpu), 1); /* Mappings may have changed - purge! */
> +        break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < s->num_irq) {
>              gic_set_pending_private(&s->gic, 0, value & 0x1ff);
> @@ -445,6 +526,19 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr 
> addr,
>              return val & 0xffff;
>          }
>          break;
> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if ((size == 2) && (offset & 7) == 0) {
> +            val = cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +            return val & 0xffff;
> +        }
> +        if ((size == 2) && (offset & 7) == 2) {
> +            val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
> +            return val & 0xffff;
> +        }

RASR is UNPREDICTABLE for non-word-size access, so we don't need
this at all.

>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              return 0;
> @@ -465,6 +559,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>      nvic_state *s = (nvic_state *)opaque;
>      uint32_t offset = addr;
>      int i;
> +    ARMCPU *cpu;
>
>      switch (offset) {
>      case 0xd18 ... 0xd23: /* System Handler Priority.  */
> @@ -488,6 +583,24 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>              break;
>          }
>          break;
> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
> +        cpu = ARM_CPU(current_cpu);
> +        if (cpu->pmsav7_dregion == 0)
> +            break;
> +        if ((size == 2) && (offset & 7) == 0) {
> +            value |= cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr] << 16;
> +            offset &= ~2;
> +            size = 4;
> +            break;
> +        }
> +        if ((size == 2) && (offset & 7) == 2) {
> +            value <<= 16;
> +            value |= cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
> +            offset &= ~2;
> +            size = 4;
> +            break;
> +        }
> +        break;

Don't need this either.

>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1089f63..d603f71 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -393,6 +393,7 @@ typedef struct CPUARMState {
>          uint32_t dfsr;
>          uint32_t mmfar;
>          uint32_t bfar;
> +        uint32_t mpu_ctrl;
>      } v7m;
>
>      /* Information associated with an exception about to be taken:
> @@ -903,6 +904,11 @@ enum arm_cpu_mode {
>  #define DFSR_BKPT           0x00000002
>  #define DFSR_HALTED         0x00000001
>
> +/* V7M MPU_CTRL bits */
> +#define MPU_CTRL_PRIVDEFENA 0x00000004
> +#define MPU_CTRL_HFNMIENA   0x00000002
> +#define MPU_CTRL_ENABLE     0x00000001
> +
>  /* If adding a feature bit which corresponds to a Linux ELF
>   * HWCAP bit, remember to update the feature-bit-to-hwcap
>   * mapping in linux-user/elfload.c:get_elf_hwcap().
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 637dbf6..542e075 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4552,7 +4552,10 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      case EXCP_DATA_ABORT:
>          env->v7m.mmfar = env->exception.vaddress;
>          env->v7m.cfsr |= CFSR_MMARVALID;
> -        /* TODO: further decoding of exception.fsr */
> +        if (cs->exception_index == EXCP_PREFETCH_ABORT)
> +            env->v7m.cfsr |= CFSR_IACCVIOL;
> +        else
> +            env->v7m.cfsr |= CFSR_DACCVIOL;
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
>          return;
>      case EXCP_BKPT:
> @@ -5973,6 +5976,8 @@ 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);
> +            if ((ap == 7) && arm_feature(env, ARM_FEATURE_M))
> +                ap = 6;
>
>              if (is_user) { /* User mode AP bit decoding */
>                  switch (ap) {
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 11dcf29..0bdeaba 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -107,6 +107,7 @@ static const VMStateDescription vmstate_m = {
>          VMSTATE_UINT32(env.v7m.dfsr, ARMCPU),
>          VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
>          VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.mpu_ctrl, ARMCPU),

Missing version bump. (You might want to put mpu_ctrl in the
list with the others, to save bumping it twice.)

>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
> 1.7.9.5
>

thanks
-- PMM