Re: [Qemu-devel] [PATCH] target-arm/translate.c: Handle non-executable page-straddling Thumb insns

[Laurent Desnogues]

Hello,

I have two minor comments.

On Sat, Sep 19, 2015 at 3:06 PM, Peter Maydell <address@hidden> wrote:
> When the memory we're trying to translate code from is not executable we have
> to turn this into a guest fault. In order to report the correct PC for this
> fault, and to make sure it is not reported until after any other possible
> faults for instructions earlier in execution, we must terminate TBs at
> the end of a page, in case the next instruction is in a non-executable page.
> This is simple for T16, A32 and A64 instructions, which are always aligned
> to their size. However T32 instructions may be 32-bits but only 16-aligned,
> so they can straddle a page boundary.
>
> Correct the condition that checks whether the next instruction will touch
> the following page, to ensure that if we're 2 bytes before the boundary
> and this insn is T32 then we end the TB.
>
> Reported-by: Pavel Dovgalyuk <address@hidden>
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> The other way you could do this would be to check before each 'read halfword'
> in the decoder whether you were going to go off the end of the page, and if
> so roll back anything you'd already generated, but that sounds really painful.
> I'm glad I don't have to fix this bug for x86 :-)
>
>
>  target-arm/translate.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 84a21ac..d5cfe84 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11167,6 +11167,38 @@ undef:
>                         default_exception_el(s));
>  }
>
> +static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
> +{
> +    /* Return true if the insn at dc->pc might cross a page boundary.
> +     * (False positives are OK, false negatives are not.)
> +     */
> +    uint16_t insn;
> +
> +    if ((s->pc & 3) == 0) {
> +        /* At a 4-aligned address we can't be crossing a page */
> +        return false;
> +    }
> +
> +    /* This must be a Thumb insn */
> +    insn = arm_lduw_code(env, s->pc, s->bswap_code);
> +
> +    switch (insn >> 11) {
> +    case 0x1d: /* 0b11101 */
> +    case 0x1e: /* 0b11110 */
> +    case 0x1f: /* 0b11111 */
> +        /* First half of a 32-bit Thumb insn. Thumb-1 cores might
> +         * end up actually treating this as two 16-bit insns (see the
> +         * code at the start of disas_thumb2_insn()) but we don't bother
> +         * to check for that as it is unlikely, and false positives here
> +         * are harmless.
> +         */
> +        return true;
> +    default:
> +        /* 16-bit Thumb insn */
> +        return false;
> +    }

I would have used return (insn >> 11) >= 0x1d instead of a switch.

> +}
> +
>  /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
>     basic block 'tb'. If search_pc is TRUE, also generate PC
>     information for each intermediate instruction. */
> @@ -11183,6 +11215,7 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>      target_ulong next_page_start;
>      int num_insns;
>      int max_insns;
> +    bool end_of_page;
>
>      /* generate intermediate code */
>
> @@ -11404,11 +11437,24 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>           * Also stop translation when a page boundary is reached.  This
>           * ensures prefetch aborts occur at the right place.  */
>          num_insns ++;

You could perhaps take the opportunity to remove that whitespace :-)

> +
> +        /* We want to stop the TB if the next insn starts in a new page,
> +         * or if it spans between this page and the next. This means that
> +         * if we're looking at the last halfword in the page we need to
> +         * see if it's a 16-bit Thumb insn (which will fit in this TB)
> +         * or a 32-bit Thumb insn (which won't).
> +         * This is to avoid generating a silly TB with a single 16-bit insn
> +         * in it at the end of this page (which would execute correctly
> +         * but isn't very efficient).
> +         */
> +        end_of_page = (dc->pc >= next_page_start) ||
> +            ((dc->pc >= next_page_start - 3) && insn_crosses_page(env, dc));
> +
>      } while (!dc->is_jmp && !tcg_op_buf_full() &&
>               !cs->singlestep_enabled &&
>               !singlestep &&
>               !dc->ss_active &&
> -             dc->pc < next_page_start &&
> +             !end_of_page &&
>               num_insns < max_insns);
>
>      if (tb->cflags & CF_LAST_IO) {

Except for the two comments and the question, this looks good to me.

Reviewed-by: Laurent Desnogues <address@hidden>


Laurent