[04/18] armv7m: Explicit error for bad vector table

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.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 target-arm/helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

On 9 November 2015 at 01:11, 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.
>
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  target-arm/helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4178400..1d7ac43 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5496,7 +5496,17 @@ 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);
> +    {
> +        MemTxResult result;
> +        addr = address_space_ldl(cs->as,
> +                                 env->v7m.vecbase + env->v7m.exception * 4,
> +                                 MEMTXATTRS_UNSPECIFIED, &result);
> +        if (result != MEMTX_OK) {
> +            cpu_abort(cs, "Failed to read from exception vector table "
> +                      "entry %08x\n",
> +                      env->v7m.vecbase + env->v7m.exception * 4);
> +        }
> +    }

The behaviour on a failed vector table read is actually architecturally
specified: we should take a nested exception (escalated to HardFault).
If it happens while we're trying to take a HardFault in the first place
then we go into Lockup (where the CPU sits around repeatedly trying
to execute an instruction at 0xFFFFFFFE; it is technically possible
to get back out of Lockup by taking an NMI or a system reset).

That said, trying to get nested exceptions and priority escalation
right is fairly involved, and implementing lockup is both involved
and an exercise in pointlessness. So I think this code is an
improvement overall. I would suggest some small changes, though:

(1) factor this out into its own function, something like:
static uint32_t v7m_read_vector(CPUARMState *env, int excnum)
so the calling code can just do
   addr = v7m_read_vector(env, env->v7m.exception);
(2) use a local variable for "env->v7m.vecbase + excnum * 4"
rather than calculating it twice

thanks
-- PMM

On 11/17/2015 12:33 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, 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.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  target-arm/helper.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 4178400..1d7ac43 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -5496,7 +5496,17 @@ 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);
>> +    {
>> +        MemTxResult result;
>> +        addr = address_space_ldl(cs->as,
>> +                                 env->v7m.vecbase + env->v7m.exception * 4,
>> +                                 MEMTXATTRS_UNSPECIFIED, &result);
>> +        if (result != MEMTX_OK) {
>> +            cpu_abort(cs, "Failed to read from exception vector table "
>> +                      "entry %08x\n",
>> +                      env->v7m.vecbase + env->v7m.exception * 4);
>> +        }
>> +    }
> 
> The behaviour on a failed vector table read is actually architecturally
> specified: we should take a nested exception (escalated to HardFault).
> If it happens while we're trying to take a HardFault in the first place
> then we go into Lockup (where the CPU sits around repeatedly trying
> to execute an instruction at 0xFFFFFFFE; it is technically possible
> to get back out of Lockup by taking an NMI or a system reset).
> 
> That said, trying to get nested exceptions and priority escalation
> right is fairly involved, and implementing lockup is both involved
> and an exercise in pointlessness. So I think this code is an
> improvement overall.

This is my thinking as well.  One point against it is that abort() is inconvenient when using '-gdb'.  I'm not sure if there is something else which could be done (cpu halt?).

> I would suggest some small changes, though:
> 
> (1) factor this out into its own function, something like:
> static uint32_t v7m_read_vector(CPUARMState *env, int excnum)
> so the calling code can just do
>    addr = v7m_read_vector(env, env->v7m.exception);
> (2) use a local variable for "env->v7m.vecbase + excnum * 4"
> rather than calculating it twice

Done.

On 2 December 2015 at 22:55, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 11/17/2015 12:33 PM, Peter Maydell wrote:
>> The behaviour on a failed vector table read is actually architecturally
>> specified: we should take a nested exception (escalated to HardFault).
>> If it happens while we're trying to take a HardFault in the first place
>> then we go into Lockup (where the CPU sits around repeatedly trying
>> to execute an instruction at 0xFFFFFFFE; it is technically possible
>> to get back out of Lockup by taking an NMI or a system reset).
>>
>> That said, trying to get nested exceptions and priority escalation
>> right is fairly involved, and implementing lockup is both involved
>> and an exercise in pointlessness. So I think this code is an
>> improvement overall.
>
> This is my thinking as well.  One point against it is that abort() is
> inconvenient when using '-gdb'.  I'm not sure if there is something
> else which could be done (cpu halt?).

I can't think of any immediately good suggestions. It is probably
worth at least commenting (a) places where we are not doing the
architectural behaviour (b) places which should go into Lockup.
Then we can find them later if we come up with a better idea...

thanks
-- PMM