Re: [Qemu-devel] [PATCH 03/18] armv7m: Complain about incorrect exception table entries.

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <address@hidden> wrote:
> For -M  These should always be thumb mode.
> Log a message if this is seen.
>
> Signed-off-by: Michael Davidsaver <address@hidden>

This one's not really correct, I'm afraid (though the spec-mandated
behaviour is a bit subtle).

> ---
>  target-arm/helper.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4408100..4178400 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5396,7 +5396,8 @@ static void do_v7m_exception_exit(CPUARMState *env)
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "M profile return from interrupt with misaligned "
>                        "PC is UNPREDICTABLE\n");
> -        /* Actual hardware seems to ignore the lsbit, and there are several
> +        /* The ARM calls for UsageFault when the T bit isn't set, but
> +         * actual hardware seems to ignore the lsbit, and there are several

No, this is UNPREDICTABLE, as the error message above says. See the
PopStack() pseudocode. No fault of any kind is mandated here (and
ignoring the lsbit in the exception return frame is within the set
of things UNPREDICTABLE allows us and the hardware to do).

>           * RTOSes out there which incorrectly assume the r15 in the stack
>           * frame should be a Thumb-style "lsbit indicates ARM/Thumb" value.
>           */
> @@ -5498,6 +5499,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 %d with incorrect "
> +                      "instruction mode in PC is UNPREDICTABLE\n",
> +                      env->v7m.exception);
> +    }

This is also wrong -- the behaviour here is defined by the
ExceptionTaken pseudocode, which requires that we set the EPSR.T
bit from the low bit of the exception vector table entry, so it's
not UNPREDICTABLE at all. (See also section B1.5.3 of the v7M ARM ARM
rev E.b.)

The part we do not implement of this is that the architecture
mandates that attempts to *execute an instruction* with the T bit
clear should generate a UsageFault. (Similarly, it's valid to
execute a branch instruction with BX that clears the T bit, but
when you get to the destination you'll get a UsageFault on first
attempt to execute an instruction.)

The correct place to implement this behaviour is in translate.c:
in the bit of code that currently reads:

        if (dc->thumb) {
            disas_thumb_insn(env, dc);
            if (dc->condexec_mask) {
                dc->condexec_cond = (dc->condexec_cond & 0xe)
                                   | ((dc->condexec_mask >> 4) & 1);
                dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f;
                if (dc->condexec_mask == 0) {
                    dc->condexec_cond = 0;
                }
            }
        } else {
            unsigned int insn = arm_ldl_code(env, dc->pc, dc->bswap_code);
            dc->pc += 4;
            disas_arm_insn(dc, insn);
        }

the else branch should have:
    if (arm_dc_feature(dc, ARM_FEATURE_M)) {
        /* For M profile any attempt to execute with EPSR.T clear triggers
         * an INVSTATE UsageFault.
         */
        gen_exception(something);
        goto done_generating;
    }

thanks
-- PMM