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

[Peter Maydell]

On 19 September 2015 at 14:41, Laurent Desnogues
<address@hidden> wrote:
> 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.

Yes, I guess so.

>> +}
>> +
>>  /* 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 :-)

RTH's v2 series on getting rid of retranslation already does that
in patch 03/22.

thanks
-- PMM