[Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Scott Wood]

If tci_out_label is called in the context of tcg_gen_code_search_pc, we
could be overwriting an already patched relocation with zero -- and not
repatch it because the set_label is past search_pc, causing a QEMU crash
when it tries to branch to a zero label.

Not writing anything to the relocation area seems to be in line with what
other backends do from the couple I looked at (x86, ppc).

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 tcg/tci/tcg-target.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
index 453f187..3c6b0f5 100644
--- a/tcg/tci/tcg-target.c
+++ b/tcg/tci/tcg-target.c
@@ -487,7 +487,7 @@ static void tci_out_label(TCGContext *s, TCGArg arg)
         assert(label->u.value);
     } else {
         tcg_out_reloc(s, s->code_ptr, sizeof(tcg_target_ulong), arg, 0);
-        tcg_out_i(s, 0);
+        s->code_ptr += sizeof(tcg_target_ulong);
     }
 }
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Stefan Weil]

Am 19.06.2012 04:31, schrieb Scott Wood:
> If tci_out_label is called in the context of tcg_gen_code_search_pc, we
> could be overwriting an already patched relocation with zero -- and not
> repatch it because the set_label is past search_pc, causing a QEMU crash
> when it tries to branch to a zero label.
>
> Not writing anything to the relocation area seems to be in line with what
> other backends do from the couple I looked at (x86, ppc).

Thanks, this might fix a crash which I have seen from time to time.
I'll run tests as soon as possible.

Could you please also look at the other backends?

I saw from git history that ppc once had the same bug.
The sparc backend (and maybe others) might still have it.

Regards,
Stefan W.

>
> Signed-off-by: Scott Wood<scottwood@freescale.com>
> ---
>   tcg/tci/tcg-target.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
> index 453f187..3c6b0f5 100644
> --- a/tcg/tci/tcg-target.c
> +++ b/tcg/tci/tcg-target.c
> @@ -487,7 +487,7 @@ static void tci_out_label(TCGContext *s, TCGArg arg)
>           assert(label->u.value);
>       } else {
>           tcg_out_reloc(s, s->code_ptr, sizeof(tcg_target_ulong), arg, 0);
> -        tcg_out_i(s, 0);
> +        s->code_ptr += sizeof(tcg_target_ulong);
>       }
>   }
>

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Blue Swirl]

On Tue, Jun 19, 2012 at 5:53 AM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 19.06.2012 04:31, schrieb Scott Wood:
>
>> If tci_out_label is called in the context of tcg_gen_code_search_pc, we
>> could be overwriting an already patched relocation with zero -- and not
>> repatch it because the set_label is past search_pc, causing a QEMU crash
>> when it tries to branch to a zero label.
>>
>> Not writing anything to the relocation area seems to be in line with what
>> other backends do from the couple I looked at (x86, ppc).
>
>
> Thanks, this might fix a crash which I have seen from time to time.
> I'll run tests as soon as possible.
>
> Could you please also look at the other backends?
>
> I saw from git history that ppc once had the same bug.
> The sparc backend (and maybe others) might still have it.

Confirmed for Sparc.

>
> Regards,
> Stefan W.
>
>
>>
>> Signed-off-by: Scott Wood<scottwood@freescale.com>
>> ---
>>  tcg/tci/tcg-target.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
>> index 453f187..3c6b0f5 100644
>> --- a/tcg/tci/tcg-target.c
>> +++ b/tcg/tci/tcg-target.c
>> @@ -487,7 +487,7 @@ static void tci_out_label(TCGContext *s, TCGArg arg)
>>          assert(label->u.value);
>>      } else {
>>          tcg_out_reloc(s, s->code_ptr, sizeof(tcg_target_ulong), arg, 0);
>> -        tcg_out_i(s, 0);
>> +        s->code_ptr += sizeof(tcg_target_ulong);

 I like this fix. Other similar fixes rewrote the fixed part of the
opcode and not the label, but the fixed part may cross byte
boundaries.

>>      }
>>  }
>>
>

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Scott Wood]

On 06/19/2012 12:53 AM, Stefan Weil wrote:
> Am 19.06.2012 04:31, schrieb Scott Wood:
>> If tci_out_label is called in the context of tcg_gen_code_search_pc, we
>> could be overwriting an already patched relocation with zero -- and not
>> repatch it because the set_label is past search_pc, causing a QEMU crash
>> when it tries to branch to a zero label.
>>
>> Not writing anything to the relocation area seems to be in line with what
>> other backends do from the couple I looked at (x86, ppc).
> 
> Thanks, this might fix a crash which I have seen from time to time.
> I'll run tests as soon as possible.
> 
> Could you please also look at the other backends?
> 
> I saw from git history that ppc once had the same bug.
> The sparc backend (and maybe others) might still have it.

SPARC looks wrong; the others look OK as far as I can tell from a quick
glance, without being familiar with all of the architectures.

-Scott

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Peter Maydell]

On 19 June 2012 22:52, Scott Wood <scottwood@freescale.com> wrote:
> On 06/19/2012 12:53 AM, Stefan Weil wrote:
>> I saw from git history that ppc once had the same bug.
>> The sparc backend (and maybe others) might still have it.
>
> SPARC looks wrong; the others look OK as far as I can tell from a quick
> glance, without being familiar with all of the architectures.

Agreed. I think we fixed most of the others as part of avoiding
problems with cache incoherency if you write the branch offset
as zero and then rewrite it with the right target at reloc time.

-- PMM

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Stefan Weil]

Am 19.06.2012 20:02, schrieb Blue Swirl:
> On Tue, Jun 19, 2012 at 5:53 AM, Stefan Weil <sw@weilnetz.de> wrote:
>> Am 19.06.2012 04:31, schrieb Scott Wood:
>>
>>> If tci_out_label is called in the context of tcg_gen_code_search_pc, we
>>> could be overwriting an already patched relocation with zero -- and not
>>> repatch it because the set_label is past search_pc, causing a QEMU crash
>>> when it tries to branch to a zero label.
>>>
>>> Not writing anything to the relocation area seems to be in line with 
>>> what
>>> other backends do from the couple I looked at (x86, ppc).
>>
>>
>> Thanks, this might fix a crash which I have seen from time to time.
>> I'll run tests as soon as possible.

Tested-by: Stefan Weil <sw@weilnetz.de>

The patch fixes my test scenario with a guest booting a Debian ARM kernel
on a x86_64 host (Linux or W64).

The following command crashes while the ARM Linux guest is booting
(shortly after "Freeing init memory") with SIGSEGV caused by tb_ptr == 2:

$ qemu-system-arm -M versatilepb -m 192 \
     -kernel vmlinuz-3.2.0-2-versatile -initrd initrd.img-3.2.0-2-versatile

With the patch applied, the ARM Linux boots correctly.

Blue, maybe that test also works with a SPARC host.
I have copied the ARM kernel and initrd to http://qemu.weilnetz.de/arm/.

Regards,
Stefan W.

>>
>> Could you please also look at the other backends?
>>
>> I saw from git history that ppc once had the same bug.
>> The sparc backend (and maybe others) might still have it.
>
> Confirmed for Sparc.
>
>>
>> Regards,
>> Stefan W.
>>
>>
>>>
>>> Signed-off-by: Scott Wood<scottwood@freescale.com>
>>> ---
>>>  tcg/tci/tcg-target.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
>>> index 453f187..3c6b0f5 100644
>>> --- a/tcg/tci/tcg-target.c
>>> +++ b/tcg/tci/tcg-target.c
>>> @@ -487,7 +487,7 @@ static void tci_out_label(TCGContext *s, TCGArg arg)
>>>          assert(label->u.value);
>>>      } else {
>>>          tcg_out_reloc(s, s->code_ptr, sizeof(tcg_target_ulong), 
>>> arg, 0);
>>> -        tcg_out_i(s, 0);
>>> +        s->code_ptr += sizeof(tcg_target_ulong);
>
> I like this fix. Other similar fixes rewrote the fixed part of the
> opcode and not the label, but the fixed part may cross byte
> boundaries.
>
>>>      }
>>>  }
>>>
>>
>

Re: [Qemu-devel] [PATCH] tci: don't write zero for reloc in tci_out_label

[Blue Swirl]

Thanks, applied.

On Tue, Jun 19, 2012 at 2:31 AM, Scott Wood <scottwood@freescale.com> wrote:
> If tci_out_label is called in the context of tcg_gen_code_search_pc, we
> could be overwriting an already patched relocation with zero -- and not
> repatch it because the set_label is past search_pc, causing a QEMU crash
> when it tries to branch to a zero label.
>
> Not writing anything to the relocation area seems to be in line with what
> other backends do from the couple I looked at (x86, ppc).
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  tcg/tci/tcg-target.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
> index 453f187..3c6b0f5 100644
> --- a/tcg/tci/tcg-target.c
> +++ b/tcg/tci/tcg-target.c
> @@ -487,7 +487,7 @@ static void tci_out_label(TCGContext *s, TCGArg arg)
>         assert(label->u.value);
>     } else {
>         tcg_out_reloc(s, s->code_ptr, sizeof(tcg_target_ulong), arg, 0);
> -        tcg_out_i(s, 0);
> +        s->code_ptr += sizeof(tcg_target_ulong);
>     }
>  }
>
> --
> 1.7.5.4
>
>