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

[Richard Henderson <rth@twiddle.net>]

On 10/21/2014 05:14 AM, Pavel Dovgalyuk 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 <pavel.dovgaluk@ispras.ru>
> ---
>  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 &&

I'd like to know more about the problem you're seeing here.

QEMU can handle TB's that span two full pages.  The poster child for this
support is of course the i386 port.  There, the test we use is

   (pc_ptr - pc_start) >= (TARGET_PAGE_SIZE - 32)

Since pc_start can be anywhere at all, this test allows a 4k sliding window
across two pages.  The -32 slop there is presumably intended to prevent us from
starting that one last instruction that might push us to 3 pages[1].

And, by "handle", I mean invalidate such TB's when either page gets
overwritten, signalling that they need to be retranslated.

My guess is that you are talking about adjusting the point at which an
exception is recognized when executing a sequence that falls off the end of a
mapping.

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.

That said, for the special case of an isa limited to 2 and 4 byte insns[3],
this is probably a good change.  After we terminate this TB, execute it, and
start translating the next, we'll have three cases:

  (1) The next thumb insn is 2 bytes, and it gets a TB all by itself.
  (2) The next thumb insn is 4 bytes,
      (a) the next page is mapped, and it gets a TB to itself,
      (b) the next page is unmapped, and we signal it immediately.

But 2b is perfect because that's exactly when the signal would be delivered by
hardware.  We could improve things by allowing case 2a to continue translation
in the new page, but that may be rare enough not be worth the extra checks.

I will say that it's probably worth moving the sense of the check.  One can
achieve the same results by subtracting from next_page_start when it is
initialized.  That moves the arithmetic out of the loop, and also allows you to
write a larger comment about the logic and not have to place it in the middle
of this rather large while condition.


r~


[1] Why 32 when the maximum insn size is more like 15 bytes, I don't know.  But
it likely doesn't matter since I'd expect such large TB's to fill up the opcode
buffer first.  There would have to be a lot of nops on that page.

[2] Or ought to have done so, for a well written translator.

[3] Hello, MIPS.  Leon, the test for mips is (now) incorrect:

        if ((ctx.pc & (TARGET_PAGE_SIZE - 1)) == 0)
            break;

may never succeed for mips16 and micromips.