Re: [Qemu-devel] [PATCH v2 13/26] armv7m: implement CCR

[Peter Maydell <peter.maydell@linaro.org>]

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Implement Configuration and Control register.
> Handle STACKALIGN and USERSETMPEND bits.

The patch doesn't seem to do anything with USERSETMPEND,
though it does do something with NONBASETHRDENA.

> ---
>  hw/intc/armv7m_nvic.c | 15 +++++++++++----
>  target-arm/cpu.h      |  1 +
>  target-arm/helper.c   |  8 +++-----
>  target-arm/machine.c  |  1 +
>  4 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 7d261ae..0f9ca6a 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -534,8 +534,7 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset)
>          /* TODO: Implement SLEEPONEXIT.  */
>          return 0;
>      case 0xd14: /* Configuration Control.  */
> -        /* TODO: Implement Configuration Control bits.  */
> -        return 0;
> +        return cpu->env.v7m.ccr;
>      case 0xd24: /* System Handler Status.  */
>          val = 0;
>          if (s->vectors[ARMV7M_EXCP_MEM].active) val |= (1 << 0);
> @@ -685,9 +684,17 @@ static void nvic_writel(NVICState *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.  */
> +        value &= 0x31b;
> +        if (value & 0x118) {
> +            qemu_log_mask(LOG_UNIMP, "CCR unimplemented bits"
> +                                     " BFHFNMIGN, DIV_0_TRP, UNALIGN_TRP");
> +            value &= ~0x118;

There's not much point in clearing out the bits we don't implement;
we may as well let them read-as-written.

> +        }
> +        cpu->env.v7m.ccr = value;
>          break;
>      case 0xd24: /* System Handler Control.  */
>          /* TODO: Real hardware allows you to set/clear the active bits
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b98ef89..4e1b8cf 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -396,6 +396,7 @@ typedef struct CPUARMState {
>          uint32_t vecbase;
>          uint32_t basepri;
>          uint32_t control;
> +        uint32_t ccr; /* Configuration and Control */
>          uint32_t cfsr; /* Configurable Fault Status */
>          uint32_t hfsr; /* HardFault Status */
>          uint32_t mmfar; /* MemManage Fault Address */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index f7e496d..17d1ca0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5412,7 +5412,7 @@ static void do_v7m_exception_exit(CPUARMState *env)
>          break;
>      case 0x9: /* Return to Thread mode w/ Main stack */
>      case 0xd: /* Return to Thread mode w/ Process stack */
> -        if (env->v7m.exception_prio != 0x100) {
> +        if ((env->v7m.exception_prio != 0x100) && !(env->v7m.ccr & 1)) {

A #define for CCR_NONBASETHRDENA would be nice (and for
CCR_STKALIGN).

>              /* Attempt to return to Thread mode
>               * from nested handler while NONBASETHRDENA not set.
>               */
> @@ -5582,10 +5582,8 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>
>      qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
>
> -    /* Align stack pointer.  */
> -    /* ??? Should only do this if Configuration Control Register
> -       STACKALIGN bit is set.  */
> -    if (env->regs[13] & 4) {
> +    /* Align stack pointer (STACKALIGN)  */
> +    if ((env->regs[13] & 4) && (env->v7m.ccr & (1<<9))) {

Spaces around operators.

>          env->regs[13] -= 4;
>          xpsr |= 0x200;
>      }
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 14a4882..7aee41e 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -100,6 +100,7 @@ static const VMStateDescription vmstate_m = {
>          VMSTATE_UINT32(env.v7m.vecbase, ARMCPU),
>          VMSTATE_UINT32(env.v7m.basepri, ARMCPU),
>          VMSTATE_UINT32(env.v7m.control, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.ccr, ARMCPU),
>          VMSTATE_UINT32(env.v7m.cfsr, ARMCPU),
>          VMSTATE_UINT32(env.v7m.hfsr, ARMCPU),
>          VMSTATE_UINT32(env.v7m.mmfar, ARMCPU),

Changing the vmstate implies that we should be bumping its
version number.

thanks
-- PMM