Re: [Qemu-devel] [PATCH] ARM - Remove fixed map code buffer restriction

[andrzej zaborowski <balrogg@gmail.com>]

Hi,

On 12 December 2011 16:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2011 15:37, Dr. David Alan Gilbert
> <david.gilbert@linaro.org> wrote:
>> On ARM, don't map the code buffer at a fixed location, and fix up the
>> call/goto tcg routines to let it do long jumps.
>>
>> Mapping the code buffer at a fixed address could sometimes result in it being
>> mapped over the top of the heap with pretty random results.
>>
>> This diff is against v1.0.
>
> Patches should always be against master, although we're still
> close enough to 1.0 that this will probably apply anyway.
>
> Comments like "This diff is against v1.0." go below the '---' so they
> don't appear in the final commit log.
>
> Code generally looks OK to me.

To me as well, I'll apply if there are no objections.  I remember
Peter asked about MAP_FIXED on other hosts, has there been a
resolution?

>
>> Signed-off-by: Dr. David Alan Gilbert <david.gilbert@linaro.org>
>> ---
>>  exec.c               |    4 +---
>>  tcg/arm/tcg-target.c |   31 ++++++++++++-------------------
>>  2 files changed, 13 insertions(+), 22 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 6b92198..ef83da1 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -497,9 +497,7 @@ static void code_gen_alloc(unsigned long tb_size)
>>         if (code_gen_buffer_size > (512 * 1024 * 1024))
>>             code_gen_buffer_size = (512 * 1024 * 1024);
>>  #elif defined(__arm__)
>> -        /* Map the buffer below 32M, so we can use direct calls and branches */
>> -        flags |= MAP_FIXED;
>> -        start = (void *) 0x01000000UL;
>> +        /* Keep the buffer no bigger than 16GB to branch between blocks */
>>         if (code_gen_buffer_size > 16 * 1024 * 1024)
>>             code_gen_buffer_size = 16 * 1024 * 1024;
>>  #elif defined(__s390x__)
>> diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c
>> index e05a64f..730d913 100644
>> --- a/tcg/arm/tcg-target.c
>> +++ b/tcg/arm/tcg-target.c
>> @@ -842,6 +842,12 @@ static inline void tcg_out_st8(TCGContext *s, int cond,
>>         tcg_out_st8_12(s, cond, rd, rn, offset);
>>  }
>>
>> +/* The _goto case is normally between TBs within the same code buffer,
>> +   and with the code buffer limited to 16GB we shouldn't need the long
>> +   case.
>> +
>> +   .... except to the prologue that is in its own buffer.
>> +     */
>>  static inline void tcg_out_goto(TCGContext *s, int cond, uint32_t addr)
>>  {
>>     int32_t val;
>> @@ -855,22 +861,20 @@ static inline void tcg_out_goto(TCGContext *s, int cond, uint32_t addr)
>>     if (val - 8 < 0x01fffffd && val - 8 > -0x01fffffd)
>>         tcg_out_b(s, cond, val);
>>     else {
>> -#if 1
>> -        tcg_abort();
>> -#else
>>         if (cond == COND_AL) {
>>             tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_PC, -4);
>> -            tcg_out32(s, addr); /* XXX: This is l->u.value, can we use it? */
>> +            tcg_out32(s, addr);
>
> I see these XXXs have been there since the ARM tcg target was
> introduced. Does anybody know what they were referring to? Andrzej?

David asked me about them, but I couldn't figure it out. (the part in
tcg_out_call apparently is just copy&pasted from tcg_ouy_goto)

BTW: I think we can also use the "ld" branch when we see the goto
target is in Thumb mode.

Cheers