Message ID | 1447031505-12477-5-git-send-email-mdavidsaver@gmail.com |
---|---|
State | New |
Headers | show |
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
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); + } + } env->regs[15] = addr & 0xfffffffe; env->thumb = addr & 1; if (!env->thumb) {
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(-)