Re: [Qemu-devel] [PATCH 2/6] ARM: accessors to Cortex-M3/M4 exception configuration and status registers

[Peter Maydell]

On 7 July 2015 at 19:25, Alex Zuepke <address@hidden> wrote:
>
> Signed-off-by: Alex Zuepke <address@hidden>
> ---
>  hw/intc/armv7m_nvic.c |   70 
> ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index e13b729..e6ae047 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -225,8 +225,8 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
>          /* TODO: Implement SLEEPONEXIT.  */
>          return 0;
>      case 0xd14: /* Configuration Control.  */
> -        /* TODO: Implement Configuration Control bits.  */
> -        return 0;
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.ccr;
>      case 0xd24: /* System Handler Status.  */
>          val = 0;
>          if (s->gic.irq_state[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
> @@ -245,16 +245,23 @@ 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;
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.cfsr;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.hfsr;
>      case 0xd30: /* Debug Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.dfsr;
>      case 0xd34: /* Mem Manage Address.  */

This is MemManage Fault Address.

> +        cpu = ARM_CPU(current_cpu);
> +        return cpu->env.v7m.mmfar;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        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");

"register".

>          return 0;
>      case 0xd40: /* PFR0.  */
>          return 0x00000030;
> @@ -361,9 +368,12 @@ static void nvic_writel(nvic_state *s, uint32_t offset, 
> uint32_t value)
>          }
>          break;
>      case 0xd10: /* System Control.  */
> -    case 0xd14: /* Configuration Control.  */
>          /* TODO: Implement control registers.  */
> -        qemu_log_mask(LOG_UNIMP, "NVIC: SCR and CCR unimplemented\n");
> +        qemu_log_mask(LOG_UNIMP, "NVIC: SCR unimplemented\n");
> +        break;
> +    case 0xd14: /* Configuration Control.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.ccr = value & 0; /* TODO: add used bits */

This is weird and the compiler is likely to complain about that & 0.
Just let the guest write the value it wants to write. (You can
mask off the reserved bits if you like but you don't have to.)

>          break;
>      case 0xd24: /* System Handler Control.  */
>          /* TODO: Real hardware allows you to set/clear the active bits
> @@ -373,13 +383,28 @@ 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;
>      case 0xd28: /* Configurable Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.cfsr &= ~value;
> +        break;
>      case 0xd2c: /* Hard Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.hfsr &= ~value;
> +        break;
>      case 0xd30: /* Debug Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.dfsr &= ~value;
> +        break;
>      case 0xd34: /* Mem Manage Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.mmfar = value;
> +        break;
>      case 0xd38: /* Bus Fault Address.  */
> +        cpu = ARM_CPU(current_cpu);
> +        cpu->env.v7m.bfar = value;
> +        break;
>      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) < s->num_irq) {
> @@ -399,6 +424,7 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr 
> addr,
>      uint32_t offset = addr;
>      int i;
>      uint32_t val;
> +    ARMCPU *cpu;
>
>      switch (offset) {
>      case 0xd18 ... 0xd23: /* System Handler Priority.  */
> @@ -407,6 +433,18 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr 
> addr,
>              val |= s->gic.priority1[(offset - 0xd14) + i][0] << (i * 8);
>          }
>          return val;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
> +        cpu = ARM_CPU(current_cpu);
> +        val = cpu->env.v7m.cfsr;
> +        if (size == 1) {
> +            val >>= (offset - 0xd28) * 8;
> +            return val & 0xff;
> +        }
> +        if ((size == 2) && ((offset & 1) == 0)) {
> +            val >>= (offset - 0xd28) * 8;
> +            return val & 0xffff;
> +        }
> +        break;

You can replace all this with:

    return extract32(cpu->env.v7m.cfsr, (offset - 0xd28) * 8, size * 8);

>      case 0xfe0 ... 0xfff: /* ID.  */
>          if (offset & 3) {
>              return 0;
> @@ -436,6 +474,20 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>          }
>          gic_update(&s->gic);
>          return;
> +    case 0xd28 ... 0xd2b: /* Configurable Fault Status.  */
> +        if (size == 1) {
> +            value <<= (offset - 0xd28) * 8;
> +            offset &= ~3;
> +            size = 4;
> +            break;
> +        }
> +        if ((size == 2) && ((offset & 1) == 0)) {
> +            value <<= (offset - 0xd28) * 8;
> +            offset &= ~3;
> +            size = 4;
> +            break;
> +        }
> +        break;

Both these if blocks have identical contents, and the
"break"s are unnecessary because we're about to do a
break anyway. This could also use a comment saying that
we can expand the write to a a 32-bit write because the
register has write-1-to-clear semantics.

>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value);
> --
> 1.7.9.5
>

thanks
-- PMM