Re: [Qemu-devel] [PATCH v2 03/26] armv7m: Explicit error for bad vector table

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

On 3 December 2015 at 00:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Give an explicit error and abort when a load
> from VECBASE fails.  Otherwise would likely
> jump to 0, which for v7-m holds the reset stack
> pointer address.
> ---
>  target-arm/helper.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 2c631e3..7b76f32 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5414,6 +5414,25 @@ static void do_v7m_exception_exit(CPUARMState *env)
>         pointer.  */
>  }
>
> +static
> +uint32_t arm_v7m_load_vector(ARMCPU *cpu)
> +
> +{
> +    CPUState *cs = &cpu->parent_obj;

This isn't the right way to cast to the base class of a QOM object.
You want:
   CPUState *cs = CPU(cpu);

> +    CPUARMState *env = &cpu->env;
> +    MemTxResult result;
> +    hwaddr vec = env->v7m.vecbase + env->v7m.exception * 4;
> +    uint32_t addr;
> +
> +    addr = address_space_ldl(cs->as, vec,
> +                             MEMTXATTRS_UNSPECIFIED, &result);
> +    if (result != MEMTX_OK) {

We could use a comment here:
   /* Architecturally this should cause a HardFault setting HSFR.VECTTBL,
    * which would then be immediately followed by our failing to load
    * the entry vector for that HardFault, which is a Lockup case.
    * Since we don't model Lockup, we just report this guest error
    * via cpu_abort().
    */

> +        cpu_abort(cs, "Failed to read from exception vector table "
> +                  "entry %08x\n", (unsigned)vec);
> +    }
> +    return addr;
> +}
> +
>  void arm_v7m_cpu_do_interrupt(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -5495,7 +5514,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> -    addr = ldl_phys(cs->as, env->v7m.vecbase + env->v7m.exception * 4);
> +    addr = arm_v7m_load_vector(cpu);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
>  }

The rest of this patch looks OK though.

thanks
-- PMM