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 <address@hidden> 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 <address@hidden>

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<address@hidden>
> > > ---
> > >  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.
>
> > >      }
> > >  }