Re: [Qemu-devel] [PATCH] arm: fix TB alignment check

[Peter Maydell]

On 21 October 2014 at 13:14, Pavel Dovgalyuk <address@hidden> wrote:
> Sometimes page faults happen during the translation of the target 
> instructions.
> To avoid the faults in the middle of the TB we have to stop translation at
> the end of the page. Current implementation of ARM translation assumes that
> instructions are aligned to their own size (4 or 2 bytes). But in thumb2 mode
> 4-byte instruction can be aligned to 2 bytes. In some cases such an alignment
> leads to page fault.
> This patch adds check that allows translation of such instructions only in
> the beginning of the TB.
>
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> ---
>  target-arm/translate.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 2c0b1de..bc3a16b 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11124,7 +11124,8 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>               !cs->singlestep_enabled &&
>               !singlestep &&
>               !dc->ss_active &&
> -             dc->pc < next_page_start &&
> +             /* +3 is for unaligned Thumb2 instructions */
> +             dc->pc + 3 < next_page_start &&
>               num_insns < max_insns);
>
>      if (tb->cflags & CF_LAST_IO) {

I finally got round to looking again at this patch from last year, which
I didn't really understand the first time round. Having investigated
more closely, this is a correctness issue (we report the page fault
with the wrong guest PC). Those get my attention much quicker if
clearly described as such :-)

As Laurent suggested, this code will give an awkward stub TB with
a single 16bit Thumb insn in it if the last insn in the page
happens to be a 16 bit Thumb insn. I'll post a patch shortly which
avoids this effect (and which has more commentary on why we need
to do this).

Incidentally, this remark by rth:
> To be honest qemu doesn't attempt to care about that much for targets that 
> have
> insns that span pages.  We've proven[2] that there are no branches or forced
> exceptions that would change control flow earlier, therefore any time 
> execution
> begins at pc_start, it will likely fall through to the next page.

fails to consider the case of non-forced exceptions (like those
on loads or stores). I think every target has to care about this
otherwise for situations like:
 load/store insn that should fault
 other insn that spans page boundary into a non-executable page

we will report the fault for the execution on the non-executable
page, when we should have reported the fault for the load/store
(and we'll get the faulting PC wrong too).

thanks
-- PMM