On 19 September 2015 at 14:41, Laurent Desnogues
<laurent.desnog...@gmail.com> wrote:
> 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.
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
