Hello,
I have two minor comments.
On Sat, Sep 19, 2015 at 3:06 PM, Peter Maydell <peter.mayd...@linaro.org> 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 <pavel.dovga...@ispras.ru>
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> 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 <laurent.desnog...@gmail.com>
Laurent
