Re: [Qemu-arm] [PATCH v2 09/26] armv7m: implement CFSR, HFSR, BFAR, and MMFAR

[Peter Maydell]

On 3 December 2015 at 00:18, Michael Davidsaver <address@hidden> wrote:
> Add the Configurable, HardFault, BusFault and MemManage Status registers.
> Note undefined instructions, violations, and escalations.
>
> No BusFaults are raised at this point.
> ---
>  hw/intc/armv7m_nvic.c | 28 ++++++++++++++++++++++------
>  target-arm/cpu.h      |  4 ++++
>  target-arm/helper.c   |  3 +++
>  target-arm/machine.c  |  8 ++++++--
>  4 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index ca9bd4c..619c320 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -333,6 +333,7 @@ void armv7m_nvic_set_pending(void *opaque, int irq)
>              irq = ARMV7M_EXCP_HARD;
>              vec = &s->vectors[irq];
>
> +            s->cpu->env.v7m.hfsr |= 1<<30; /* FORCED */

I suggest defining some constants for the various FSR bits, eg

#define HFSR_FORCED (1U << 30)
#define HFSR_DEBUGEVT (1U << 31)

etc.

Then you don't need a comment every time you set a bit to say
what it means...

target-arm/cpu.h is probably the best place for these.

>              DPRINTF(0, "Escalate %d to HardFault\n", oldirq);
>          }
>      }
> @@ -548,16 +549,20 @@ static uint32_t nvic_readl(NVICState *s, uint32_t 
> offset)
>          if (s->vectors[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 cpu->env.v7m.cfsr;
>      case 0xd2c: /* Hard Fault Status.  */
> +        return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> -    case 0xd34: /* Mem Manage Address.  */
> +        qemu_log_mask(LOG_UNIMP, "Debug Fault status register 
> unimplemented\n");
> +        return 0;
> +    case 0xd34: /* MMFAR MemManage Fault Address */
> +        return cpu->env.v7m.mmfar;
>      case 0xd38: /* Bus Fault Address.  */
> +        return cpu->env.v7m.bfar;
>      case 0xd3c: /* Aux Fault Status.  */
>          /* TODO: Implement fault status registers.  */
> -        qemu_log_mask(LOG_UNIMP, "Fault status registers unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP,
> +                      "Aux Fault status registers unimplemented\n");
>          return 0;
>      case 0xd40: /* PFR0.  */
>          return 0x00000030;
> @@ -690,13 +695,24 @@ static void nvic_writel(NVICState *s, uint32_t offset, 
> uint32_t value)
>           */
>          break;
>      case 0xd28: /* Configurable Fault Status.  */
> +        cpu->env.v7m.cfsr &= ~value; /* W1C */
> +        break;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu->env.v7m.hfsr &= ~value; /* W1C */
> +        break;
>      case 0xd30: /* Debug Fault Status.  */
> +        qemu_log_mask(LOG_UNIMP,
> +                      "NVIC: debug fault status register unimplemented\n");
> +        break;
>      case 0xd34: /* Mem Manage Address.  */
> +        cpu->env.v7m.mmfar = value;
> +        return;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu->env.v7m.bfar = value;
> +        return;
>      case 0xd3c: /* Aux Fault Status.  */
>          qemu_log_mask(LOG_UNIMP,
> -                      "NVIC: fault status registers unimplemented\n");
> +                      "NVIC: Aux fault status registers unimplemented\n");
>          break;
>      case 0xf00: /* Software Triggered Interrupt Register */
>          if ((value & 0x1ff) < NVIC_MAX_IRQ) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 4b7f78e..4262efc 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -396,6 +396,10 @@ typedef struct CPUARMState {
>          uint32_t vecbase;
>          uint32_t basepri;
>          uint32_t control;
> +        uint32_t cfsr; /* Configurable Fault Status */
> +        uint32_t hfsr; /* HardFault Status */
> +        uint32_t mmfar; /* MemManage Fault Address */
> +        uint32_t bfar; /* BusFault Address */
>          int current_sp;
>          int exception;
>          int exception_prio;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4490b74..d1ca011 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5454,6 +5454,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> +        env->v7m.cfsr |= 1<<16; /* UNDEFINSTR */

We get here for other things than UNDEFINSTR (eg NOCP and maybe
also DIVBYZERO and INVSTATE). You need to figure out which before you
can set the right CFSR bit. If you're lucky then exception.syndrome
will have sufficient information; if not then we may need to sort out
something else (probably borrowing syndrome to pass v7m specific
info as needed).

>          break;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
> @@ -5465,6 +5466,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>           * should set the MMFAR, etc from exception.fsr and 
> exception.vaddress.
>           */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> +        env->v7m.mmfar = env->exception.vaddress;
> +        env->v7m.cfsr = (1<<1)|(1<<7); /* DACCVIOL and MMARVALID */

This change is fixing the TODO comment which you can just see the
tail end of in the context, so you should remove that comment.

You should decode exception.fsr properly to distinguish whether
this is a BusFault or a MemManage fault and what subtype.
(Also whether this is EXCP_DATA_ABORT or EXCP_PREFETCH_ABORT
will tell you whether it's IACCVIOL or DACCVIOL.)

>          break;
>      case EXCP_BKPT:
>          if (semihosting_enabled()) {
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 36a0d15..14a4882 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -92,14 +92,18 @@ static bool m_needed(void *opaque)
>
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = m_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
>          VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
>          VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.bfar, ARMCPU),
>          VMSTATE_INT32(env.v7m.current_sp, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>          VMSTATE_END_OF_LIST()
> --
> 2.1.4

thanks
-- PMM