[2/3] armv7-m: fix non-IRQ exceptions

Handlers will not be entered unless v7m.exception is updated.
For example, an invalid instruction won't invoke UsageError,
but rather re-executes the invalid instruction forever.

Add warn and fix of mis-aligned handlers.

Ensure exception return "addresses" always fault,
and trap them just before the EXCP_DATA_ABORT
handler would be invoked and execute return instead
of MemManage.
This removes the need for the "armv7m.hack" MemoryRegion.
---
 hw/arm/armv7m.c     |  8 --------
 target-arm/helper.c | 27 +++++++++++++++++++++------
 2 files changed, 21 insertions(+), 14 deletions(-)

On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
> Handlers will not be entered unless v7m.exception is updated.
> For example, an invalid instruction won't invoke UsageError,
> but rather re-executes the invalid instruction forever.
>
> Add warn and fix of mis-aligned handlers.
>
> Ensure exception return "addresses" always fault,
> and trap them just before the EXCP_DATA_ABORT
> handler would be invoked and execute return instead
> of MemManage.
> This removes the need for the "armv7m.hack" MemoryRegion.
> ---
>  hw/arm/armv7m.c     |  8 --------
>  target-arm/helper.c | 27 +++++++++++++++++++++------
>  2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index eb214db..0fc95de 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -178,7 +178,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>      uint64_t lowaddr;
>      int i;
>      int big_endian;
> -    MemoryRegion *hack = g_new(MemoryRegion, 1);
>
>      if (cpu_model == NULL) {
>         cpu_model = "cortex-m3";
> @@ -226,13 +225,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>          }
>      }
>
> -    /* Hack to map an additional page of ram at the top of the address
> -       space.  This stops qemu complaining about executing code outside RAM
> -       when returning from an exception.  */
> -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000, &error_fatal);
> -    vmstate_register_ram_global(hack);
> -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
> -

CC PMM, Alistair and Marcin. They were discussing this recently.

Regards,
Peter

>      qemu_register_reset(armv7m_reset, cpu);
>      return pic;
>  }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..56b238f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5346,18 +5346,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      switch (cs->exception_index) {
>      case EXCP_UDEF:
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_USAGE;
> +        break;
>      case EXCP_SWI:
>          /* The PC already points to the next instruction.  */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_SVC;
> +        break;
>      case EXCP_PREFETCH_ABORT:
>      case EXCP_DATA_ABORT:
> -        /* TODO: if we implemented the MPU registers, this is where we
> -         * should set the MMFAR, etc from exception.fsr and exception.vaddress.
> -         */
> +        if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0) {
> +            /* this isn't a real fault, but rather a result of return from interrupt */
> +            do_v7m_exception_exit(env);
> +            return;
> +        }
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> -        return;
> +        env->v7m.exception = ARMV7M_EXCP_MEM;
> +        break;
>      case EXCP_BKPT:
>          if (semihosting_enabled()) {
>              int nr;
> @@ -5407,6 +5412,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
> +    if(!env->thumb) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "M profile interrupt handler with misaligned "
> +                      "PC is UNPREDICTABLE\n");
> +        env->thumb = 1;
> +    }
>  }
>
>  /* Function used to synchronize QEMU's AArch64 register set with AArch32
> @@ -6682,6 +6693,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>      *phys_ptr = address;
>      *prot = 0;
>
> +    /* ensure exception returns take precidence */
> +    if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0)
> +        return true;
> +
>      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
>          get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
>      } else { /* MPU enabled */
> --
> 2.1.4
>

I'm starting to doubt my diagnosis.  The bug may be in my understanding of
the interrupt priorities.  I'll have to do another test program.
On Oct 11, 2015 11:25 AM, "Peter Crosthwaite" <crosthwaitepeter@gmail.com>
wrote:

> On Fri, Oct 9, 2015 at 6:28 AM, Michael Davidsaver
> <mdavidsaver@gmail.com> wrote:
> > Handlers will not be entered unless v7m.exception is updated.
> > For example, an invalid instruction won't invoke UsageError,
> > but rather re-executes the invalid instruction forever.
> >
> > Add warn and fix of mis-aligned handlers.
> >
> > Ensure exception return "addresses" always fault,
> > and trap them just before the EXCP_DATA_ABORT
> > handler would be invoked and execute return instead
> > of MemManage.
> > This removes the need for the "armv7m.hack" MemoryRegion.
> > ---
> >  hw/arm/armv7m.c     |  8 --------
> >  target-arm/helper.c | 27 +++++++++++++++++++++------
> >  2 files changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> > index eb214db..0fc95de 100644
> > --- a/hw/arm/armv7m.c
> > +++ b/hw/arm/armv7m.c
> > @@ -178,7 +178,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
> int mem_size, int num_irq,
> >      uint64_t lowaddr;
> >      int i;
> >      int big_endian;
> > -    MemoryRegion *hack = g_new(MemoryRegion, 1);
> >
> >      if (cpu_model == NULL) {
> >         cpu_model = "cortex-m3";
> > @@ -226,13 +225,6 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory,
> int mem_size, int num_irq,
> >          }
> >      }
> >
> > -    /* Hack to map an additional page of ram at the top of the address
> > -       space.  This stops qemu complaining about executing code outside
> RAM
> > -       when returning from an exception.  */
> > -    memory_region_init_ram(hack, NULL, "armv7m.hack", 0x1000,
> &error_fatal);
> > -    vmstate_register_ram_global(hack);
> > -    memory_region_add_subregion(system_memory, 0xfffff000, hack);
> > -
>
> CC PMM, Alistair and Marcin. They were discussing this recently.
>
> Regards,
> Peter
>
> >      qemu_register_reset(armv7m_reset, cpu);
> >      return pic;
> >  }
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 8367997..56b238f 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -5346,18 +5346,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> >      switch (cs->exception_index) {
> >      case EXCP_UDEF:
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_USAGE;
> > +        break;
> >      case EXCP_SWI:
> >          /* The PC already points to the next instruction.  */
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SVC);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_SVC;
> > +        break;
> >      case EXCP_PREFETCH_ABORT:
> >      case EXCP_DATA_ABORT:
> > -        /* TODO: if we implemented the MPU registers, this is where we
> > -         * should set the MMFAR, etc from exception.fsr and
> exception.vaddress.
> > -         */
> > +        if(env->v7m.exception!=0 &&
> env->exception.vaddress>=0xfffffff0) {
> > +            /* this isn't a real fault, but rather a result of return
> from interrupt */
> > +            do_v7m_exception_exit(env);
> > +            return;
> > +        }
> >          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
> > -        return;
> > +        env->v7m.exception = ARMV7M_EXCP_MEM;
> > +        break;
> >      case EXCP_BKPT:
> >          if (semihosting_enabled()) {
> >              int nr;
> > @@ -5407,6 +5412,12 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
> >      addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
> >      env->regs[15] = addr & 0xfffffffe;
> >      env->thumb = addr & 1;
> > +    if(!env->thumb) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "M profile interrupt handler with misaligned "
> > +                      "PC is UNPREDICTABLE\n");
> > +        env->thumb = 1;
> > +    }
> >  }
> >
> >  /* Function used to synchronize QEMU's AArch64 register set with AArch32
> > @@ -6682,6 +6693,10 @@ static bool get_phys_addr_pmsav7(CPUARMState
> *env, uint32_t address,
> >      *phys_ptr = address;
> >      *prot = 0;
> >
> > +    /* ensure exception returns take precidence */
> > +    if(env->v7m.exception!=0 && env->exception.vaddress>=0xfffffff0)
> > +        return true;
> > +
> >      if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */
> >          get_phys_addr_pmsav7_default(env, mmu_idx, address, prot);
> >      } else { /* MPU enabled */
> > --
> > 2.1.4
> >
>

On 11 October 2015 at 19:58, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> I'm starting to doubt my diagnosis.  The bug may be in my understanding of
> the interrupt priorities.  I'll have to do another test program.

Note that our handling of prioritization of the internal
exceptions is pretty badly broken. The fix for this probably
involves a redesign rather than a point fix :-/

thanks
-- PMM