Re: [Qemu-devel] [PATCH 3/3] armv7-m: add MPU to cortex-m3 and cortex-m4

[Peter Crosthwaite]

On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
<address@hidden> wrote:
> The M series MPU is almost the same as the already
> implemented R series MPU.  So use the M series
> and translate as best we can.
>

There is some work on list for this that never got a respin:

https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg01945.html

> The HFNMIENA bit in MPU_CTRL is not implemented.
>
> Implement CFSR and MMFAR to report fault address
> to MemManage handler.
>
> Add MPU feature flag to cortex-m3 and -m4.
> ---
>  hw/intc/armv7m_nvic.c | 154 
> ++++++++++++++++++++++++++++++++++++++++++++++++--
>  target-arm/cpu-qom.h  |   4 ++
>  target-arm/cpu.c      |  14 +++++
>  target-arm/helper.c   |   7 +++
>  4 files changed, 174 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index a671d84..94011cf 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -245,12 +245,11 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t 
> offset)
>          if (s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled) val |= (1 << 18);
>          return val;
>      case 0xd28: /* Configurable Fault Status.  */
> -        /* TODO: Implement Fault Status.  */
> -        qemu_log_mask(LOG_UNIMP, "Configurable Fault Status 
> unimplemented\n");
> -        return 0;
> +        return ARM_CPU(current_cpu)->pmsav7_cfsr;

You should avoid dereferenced inline QOM casts and create a local variable.

> +    case 0xd34: /* MMFAR MemManage Fault Address */
> +        return ARM_CPU(current_cpu)->pmsav7_mmfar;

Why reorder the addresses in the switch?

>      case 0xd2c: /* Hard Fault Status.  */
>      case 0xd30: /* Debug Fault Status.  */
> -    case 0xd34: /* Mem Manage Address.  */
>      case 0xd38: /* Bus Fault Address.  */
>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> @@ -283,6 +282,55 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t 
> offset)
>      case 0xd70: /* ISAR4.  */
>          return 0x01310102;
>      /* TODO: Implement debug registers.  */
> +    case 0xd90: /* MPU_TYPE */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->has_mpu ? (cpu->pmsav7_dregion<<8) : 0;
> +        break;
> +    case 0xd94: /* MPU_CTRL */
> +        val = 0;
> +        cpu = ARM_CPU(current_cpu);
> +        if(cpu->env.cp15.sctlr_el[0] & SCTLR_M)
> +            val |= 1; /* ENABLE */
> +        /* HFNMIENA not implemented, see nvic_writel() */
> +        if(cpu->env.cp15.sctlr_el[0] & SCTLR_BR)
> +            val |= 4; /* PRIVDEFENA */
> +        return val;
> +    case 0xd98: /* MPU_RNR */
> +        return ARM_CPU(current_cpu)->env.cp15.c6_rgnr;
> +    case 0xd9c: /* MPU_RBAR */
> +    case 0xda4: /* MPU_RBAR_A1 */
> +    case 0xdaC: /* MPU_RBAR_A2 */
> +    case 0xdb4: /* MPU_RBAR_A3 */
> +    {
> +        uint32_t range;
> +        cpu = ARM_CPU(current_cpu);
> +        if(offset==0xd9c)

spaces around ==

> +            range = cpu->env.cp15.c6_rgnr;
> +        else
> +            range = (offset-0xda4)/8;
> +
> +        if(range>=cpu->pmsav7_dregion) return 0;

{} for if body, return on new line. If you run your patch through
scripts/checkpatch.pl it will detect some of these conventions.

> +
> +        return (cpu->env.pmsav7.drbar[range]&(0x1f)) | (range&0xf);

Spaces around &, parentheses around hex constant not needed.

> +    }
> +    case 0xda0: /* MPU_RASR */
> +    case 0xda8: /* MPU_RASR_A1 */
> +    case 0xdb0: /* MPU_RASR_A2 */
> +    case 0xdb8: /* MPU_RASR_A3 */
> +    {
> +        uint32_t range;
> +        cpu = ARM_CPU(current_cpu);
> +
> +        if(offset==0xda0)
> +            range = cpu->env.cp15.c6_rgnr;
> +        else
> +            range = (offset-0xda8)/8;
> +
> +        if(range>=cpu->pmsav7_dregion) return 0;
> +
> +        return ((cpu->env.pmsav7.dracr[range]&0xffff)<<16)
> +                | (cpu->env.pmsav7.drsr[range]&0xffff);
> +    }

More style nits here.

Regards,
Peter

>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "NVIC: Bad read offset 0x%x\n", 
> offset);
>          return 0;
> @@ -376,14 +424,110 @@ static void nvic_writel(nvic_state *s, uint32_t 
> offset, uint32_t value)
>          s->gic.irq_state[ARMV7M_EXCP_USAGE].enabled = (value & (1 << 18)) != 
> 0;
>          break;