Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Peter Maydell]

On Tue, 17 Sep 2019 at 13:24, Konopik, Andreas <andreas.konopik@fau.de> wrote:
>
> Hello,
>
> I am wondering why the "Hello world"-program available in TASKING
> (v6.2r1) generates a segfault.
>
> I compiled the program for the 'TC1796' Processor (Infineon TriCore 1
> Family -> AUDO NextGeneration Family).
> QEMU was built with gcc 9.1.0 and I added a memory region, that was
> necessary [1].
> Running QEMU with following options:
> `./qemu/build/tricore-softmmu/qemu-system-tricore -nographic -M
> tricore_testboard -cpu tc1796 -kernel hello.elf`
> led to a segfault in a short time

Hi -- I'm cc'ing the qemu-devel list and the TriCore maintainer;
not everybody reads the -discuss mailing list.

> Using gdb and valgrind I found out that:
> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e. CSFRs,
> which leads to the Segfault
> - The uninitialised values were created by stack allocation of
> DisasContext in 'gen_intermediate_code()'

This definitely sounds like a bug: do you have a stack
backtrace from valgrind or gdb of the bad access and the
segfault?

> The segfault does not occur when configuring QEMU with '--enable-debug'
> flag, so i suspect it might be a compiler error while building QEMU.
> It did not come up before target/tricore implemented the
> DisasContextBase API (commit d013d220c710054a6d755941460f88c186fef7b5).
>
> Does anybody else have this problem?
>
>
> Regards,
>
> Andreas
>
>
>
> [1] Memory-layout addition (according to TASKING memory-layout):
>
> > diff --git a/hw/tricore/tricore_testboard.c
> > b/hw/tricore/tricore_testboard.c
> > index aef3289f8c..c287e0e7f5 100644
> > --- a/hw/tricore/tricore_testboard.c
> > +++ b/hw/tricore/tricore_testboard.c
> > @@ -59,6 +59,7 @@ static void tricore_testboard_init(MachineState
> > *machine, int board_id)
> >     CPUTriCoreState *env;
> >
> >     MemoryRegion *sysmem = get_system_memory();
> > +    MemoryRegion *pflash = g_new(MemoryRegion, 1);
> >     MemoryRegion *ext_cram = g_new(MemoryRegion, 1);
> >     MemoryRegion *ext_dram = g_new(MemoryRegion, 1);
> >     MemoryRegion *int_cram = g_new(MemoryRegion, 1);
> > @@ -68,6 +69,8 @@ static void tricore_testboard_init(MachineState
> > *machine, int board_id)
> >
> >     cpu = TRICORE_CPU(cpu_create(machine->cpu_type));
> >     env = &cpu->env;
> > +    memory_region_init_ram(pflash, NULL, "pflash",
> > +                           2 * MiB, &error_fatal);
> >     memory_region_init_ram(ext_cram, NULL, "powerlink_ext_c.ram",
> >                            2 * MiB, &error_fatal);
> >     memory_region_init_ram(ext_dram, NULL, "powerlink_ext_d.ram",
> > @@ -81,6 +84,7 @@ static void tricore_testboard_init(MachineState
> > *machine, int board_id)
> >     memory_region_init_ram(pcp_text, NULL, "powerlink_pcp_text.ram",
> >                            32 * KiB, &error_fatal);
> >
> > +    memory_region_add_subregion(sysmem, 0xa0000000, pflash);
> >     memory_region_add_subregion(sysmem, 0x80000000, ext_cram);
> >     memory_region_add_subregion(sysmem, 0xa1000000, ext_dram);
> >     memory_region_add_subregion(sysmem, 0xd4000000, int_cram);

I don't know enough about TriCore or the board QEMU is
emulating here to know whether this is the right thing;
Bastian?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Konopik, Andreas]

>> Using gdb and valgrind I found out that:
>> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e. 
>> CSFRs,
>> which leads to the Segfault
>> - The uninitialised values were created by stack allocation of
>> DisasContext in 'gen_intermediate_code()'
> 
> This definitely sounds like a bug: do you have a stack
> backtrace from valgrind or gdb of the bad access and the
> segfault?
> 

GDB:

> [...]
> Thread 3 "qemu-system-tri" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff10a4700 (LWP 146730)]
> 0x00005555556edb67 in gen_mfcr (ret=0xab0, offset=<optimized out>,
>    ctx=<optimized out>)
>    at /home/akonopik/qemu_src/target/tricore/cpu.h:274
> 274	    return (env->features & (1ULL << feature)) != 0;
> (gdb) bt
> #0  0x00005555556edb67 in gen_mfcr
>    (ret=0xab0, offset=<optimized out>, ctx=<optimized out>)
>    at /home/akonopik/qemu_src/target/tricore/cpu.h:274
> #1  0x000055555570bc30 in decode_rlc_opc (op1=<optimized out>, 
> ctx=0x7ffff10a3540)
>    at /home/akonopik/qemu_src/target/tricore/translate.c:6020
> #2  0x000055555570bc30 in decode_32Bit_opc 
> (ctx=ctx@entry=0x7ffff10a3540)
>    at /home/akonopik/qemu_src/target/tricore/translate.c:8680
> #3  0x000055555570bf77 in tricore_tr_translate_insn
>    (dcbase=0x7ffff10a3540, cpu=<optimized out>)
>    at /home/akonopik/qemu_src/target/tricore/translate.c:8856
> #4  0x00005555556e15c9 in translator_loop (ops=
>    0x555555c2b340 <tricore_tr_ops>, db=0x7ffff10a3540, 
> cpu=0x555555dc0480, tb=<optimized out>, max_insns=<optimized out>)
>    at /home/akonopik/qemu_src/accel/tcg/translator.c:94
> #5  0x000055555570d96c in gen_intermediate_code
>    (cs=cs@entry=0x555555dc0480, tb=tb@entry=0x7fffea000280 
> <code_gen_buffer+595>, max_insns=max_insns@entry=512)
>    at /home/akonopik/qemu_src/target/tricore/translate.c:8907
> #6  0x00005555556e01f4 in tb_gen_code
>    (cpu=cpu@entry=0x555555dc0480, pc=pc@entry=2147485022, 
> cs_base=cs_base@entry=0, flags=flags@entry=0, cflags=-16777216, 
> cflags@entry=0)
>    at /home/akonopik/qemu_src/accel/tcg/translate-all.c:1738
> #7  0x00005555556de474 in tb_find
>    (cf_mask=0, tb_exit=0, last_tb=0x7fffea000140 <code_gen_buffer+275>, 
> cpu=0x555555dc0758) at /home/akonopik/qemu_src/accel/tcg/cpu-exec.c:408
> #8  0x00005555556de474 in cpu_exec (cpu=cpu@entry=0x555555dc0480)
>    at /home/akonopik/qemu_src/accel/tcg/cpu-exec.c:730
> #9  0x00005555556a59c0 in tcg_cpu_exec (cpu=0x555555dc0480)
>    at /home/akonopik/qemu_src/cpus.c:1445
> #10 0x00005555556a7831 in qemu_tcg_rr_cpu_thread_fn 
> (arg=arg@entry=0x555555dc0480)
>    at /home/akonopik/qemu_src/cpus.c:1547
> #11 0x00005555558d540b in qemu_thread_start (args=<optimized out>)
>    at /home/akonopik/qemu_src/util/qemu-thread-posix.c:502
> #12 0x00007ffff5a1657f in start_thread () at /usr/lib/libpthread.so.0
> #13 0x00007ffff59460e3 in clone () at /usr/lib/libc.so.6

VALGRIND:

> ==146846== Memcheck, a memory error detector
> ==146846== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et 
> al.
> ==146846== Using Valgrind-3.14.0 and LibVEX; rerun with -h for 
> copyright info
> ==146846== Command:
> /home/akonopik/qemu_src/build/tricore-softmmu/qemu-system-tricore 
> -nographic -M
> tricore_testboard -cpu tc1796 -kernel /home/akonopik/hello.elf
> ==146846==
> --146846-- WARNING: unhandled amd64-linux syscall: 317
> --146846-- You may be able to write your own handler.
> --146846-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
> --146846-- Nevertheless we consider this a bug.  Please report
> --146846-- it at http://valgrind.org/support/bug_reports.html.
> ==146846== Conditional jump or move depends on uninitialised value(s)
> ==146846==    at 0x223D94: tcg_target_init (tcg-target.inc.c:3776)
> ==146846==    by 0x223D94: tcg_context_init (tcg.c:961)
> ==146846==    by 0x293E29: cpu_gen_init (translate-all.c:240)
> ==146846==    by 0x293E29: tcg_exec_init (translate-all.c:1148)
> ==146846==    by 0x275333: tcg_init (tcg-all.c:63)
> ==146846==    by 0x274DE3: accel_init_machine (accel.c:63)
> ==146846==    by 0x274DE3: configure_accelerator (accel.c:110)
> ==146846==    by 0x210236: main (vl.c:4185)
> ==146846==  Uninitialised value was created by a stack allocation
> ==146846==    at 0x5E3E117: ??? (in /usr/lib/libglib-2.0.so.0.6200.0)
> ==146846==
> QEMU 4.1.50 monitor - type 'help' for more information
> (qemu) ==146846== Thread 3:
> ==146846== Use of uninitialised value of size 8
> ==146846==    at 0x2A1B67: gen_mfcr.isra.0 (csfr.def:9)
> ==146846==    by 0x2BFC2F: decode_rlc_opc (translate.c:6020)
> ==146846==    by 0x2BFC2F: decode_32Bit_opc (translate.c:8680)
> ==146846==    by 0x2BFF76: tricore_tr_translate_insn (translate.c:8856)
> ==146846==    by 0x2955C8: translator_loop (translator.c:94)
> ==146846==    by 0x2C196B: gen_intermediate_code (translate.c:8907)
> ==146846==    by 0x2941F3: tb_gen_code (translate-all.c:1738)
> ==146846==    by 0x292473: tb_find (cpu-exec.c:408)
> ==146846==    by 0x292473: cpu_exec (cpu-exec.c:730)
> ==146846==    by 0x2599BF: tcg_cpu_exec (cpus.c:1445)
> ==146846==    by 0x25B830: qemu_tcg_rr_cpu_thread_fn (cpus.c:1547)
> ==146846==    by 0x48940A: qemu_thread_start (qemu-thread-posix.c:502)
> ==146846==    by 0x6DEE57E: start_thread (in 
> /usr/lib/libpthread-2.29.so)
> ==146846==    by 0x6F040E2: clone (in /usr/lib/libc-2.29.so)
> ==146846==  Uninitialised value was created by a stack allocation
> ==146846==    at 0x2C1940: gen_intermediate_code (translate.c:8905)
> ==146846==
> ==146846== Use of uninitialised value of size 8
> ==146846==    at 0x2A302C: gen_mtcr.isra.0 (csfr.def:9)
> ==146846==    by 0x2BFC9C: decode_rlc_opc (translate.c:6046)
> ==146846==    by 0x2BFC9C: decode_32Bit_opc (translate.c:8680)
> ==146846==    by 0x2BFF76: tricore_tr_translate_insn (translate.c:8856)
> ==146846==    by 0x2955C8: translator_loop (translator.c:94)
> ==146846==    by 0x2C196B: gen_intermediate_code (translate.c:8907)
> ==146846==    by 0x2941F3: tb_gen_code (translate-all.c:1738)
> ==146846==    by 0x292473: tb_find (cpu-exec.c:408)
> ==146846==    by 0x292473: cpu_exec (cpu-exec.c:730)
> ==146846==    by 0x2599BF: tcg_cpu_exec (cpus.c:1445)
> ==146846==    by 0x25B830: qemu_tcg_rr_cpu_thread_fn (cpus.c:1547)
> ==146846==    by 0x48940A: qemu_thread_start (qemu-thread-posix.c:502)
> ==146846==    by 0x6DEE57E: start_thread (in 
> /usr/lib/libpthread-2.29.so)
> ==146846==    by 0x6F040E2: clone (in /usr/lib/libc-2.29.so)
> ==146846==  Uninitialised value was created by a stack allocation
> ==146846==    at 0x2C1940: gen_intermediate_code (translate.c:8905)
> [...]


Regards,

Andreas

Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Bastian Koppelmann]

Hi Andreas,

On 9/17/19 2:32 PM, Peter Maydell wrote:
> On Tue, 17 Sep 2019 at 13:24, Konopik, Andreas <andreas.konopik@fau.de> wrote:
>> Hello,
>>
>> I am wondering why the "Hello world"-program available in TASKING
>> (v6.2r1) generates a segfault.
>>
>> I compiled the program for the 'TC1796' Processor (Infineon TriCore 1
>> Family -> AUDO NextGeneration Family).
>> QEMU was built with gcc 9.1.0 and I added a memory region, that was
>> necessary [1].
>> Running QEMU with following options:
>> `./qemu/build/tricore-softmmu/qemu-system-tricore -nographic -M
>> tricore_testboard -cpu tc1796 -kernel hello.elf`
>> led to a segfault in a short time
> Hi -- I'm cc'ing the qemu-devel list and the TriCore maintainer;
> not everybody reads the -discuss mailing list.
>
>> Using gdb and valgrind I found out that:
>> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e. CSFRs,
>> which leads to the Segfault
>> - The uninitialised values were created by stack allocation of
>> DisasContext in 'gen_intermediate_code()'


Sounds like a thing we already fixed (see 
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05496.html). 
However, I see that you used a version already containing the fix.


> This definitely sounds like a bug: do you have a stack
> backtrace from valgrind or gdb of the bad access and the
> segfault?


Yes, or a small reproducer program.


>>> diff --git a/hw/tricore/tricore_testboard.c
>>> b/hw/tricore/tricore_testboard.c
>>> index aef3289f8c..c287e0e7f5 100644
>>> --- a/hw/tricore/tricore_testboard.c
>>> +++ b/hw/tricore/tricore_testboard.c
>>> @@ -59,6 +59,7 @@ static void tricore_testboard_init(MachineState
>>> *machine, int board_id)
>>>      CPUTriCoreState *env;
>>>
>>>      MemoryRegion *sysmem = get_system_memory();
>>> +    MemoryRegion *pflash = g_new(MemoryRegion, 1);
>>>      MemoryRegion *ext_cram = g_new(MemoryRegion, 1);
>>>      MemoryRegion *ext_dram = g_new(MemoryRegion, 1);
>>>      MemoryRegion *int_cram = g_new(MemoryRegion, 1);
>>> @@ -68,6 +69,8 @@ static void tricore_testboard_init(MachineState
>>> *machine, int board_id)
>>>
>>>      cpu = TRICORE_CPU(cpu_create(machine->cpu_type));
>>>      env = &cpu->env;
>>> +    memory_region_init_ram(pflash, NULL, "pflash",
>>> +                           2 * MiB, &error_fatal);
>>>      memory_region_init_ram(ext_cram, NULL, "powerlink_ext_c.ram",
>>>                             2 * MiB, &error_fatal);
>>>      memory_region_init_ram(ext_dram, NULL, "powerlink_ext_d.ram",
>>> @@ -81,6 +84,7 @@ static void tricore_testboard_init(MachineState
>>> *machine, int board_id)
>>>      memory_region_init_ram(pcp_text, NULL, "powerlink_pcp_text.ram",
>>>                             32 * KiB, &error_fatal);
>>>
>>> +    memory_region_add_subregion(sysmem, 0xa0000000, pflash);
>>>      memory_region_add_subregion(sysmem, 0x80000000, ext_cram);
>>>      memory_region_add_subregion(sysmem, 0xa1000000, ext_dram);
>>>      memory_region_add_subregion(sysmem, 0xd4000000, int_cram);
> I don't know enough about TriCore or the board QEMU is
> emulating here to know whether this is the right thing;
> Bastian?


As the name suggest this is just a Testboard (and thus is not linked to 
any real board) that initializes the minimal amount of memory that is 
needed to run a binary. If you're interested you can implement a real 
board. I'm happy to take such a patch :)


Cheers,

Bastian

Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Peter Maydell]

On Tue, 17 Sep 2019 at 14:06, Konopik, Andreas <andreas.konopik@fau.de> wrote:
>
> >> Using gdb and valgrind I found out that:
> >> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e.
> >> CSFRs,
> >> which leads to the Segfault
> >> - The uninitialised values were created by stack allocation of
> >> DisasContext in 'gen_intermediate_code()'
> >
> > This definitely sounds like a bug: do you have a stack
> > backtrace from valgrind or gdb of the bad access and the
> > segfault?
> >
> > [...]
> > Thread 3 "qemu-system-tri" received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7ffff10a4700 (LWP 146730)]
> > 0x00005555556edb67 in gen_mfcr (ret=0xab0, offset=<optimized out>,
> >    ctx=<optimized out>)
> >    at /home/akonopik/qemu_src/target/tricore/cpu.h:274
> > 274       return (env->features & (1ULL << feature)) != 0;
> > (gdb) bt
> > #0  0x00005555556edb67 in gen_mfcr
> >    (ret=0xab0, offset=<optimized out>, ctx=<optimized out>)
> >    at /home/akonopik/qemu_src/target/tricore/cpu.h:274

It looks like tricore_tr_init_disas_context() isn't
initializing ctx->env. If this is the problem then this
patch ought to fix it:

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index c574638c9f7..305d896cd2c 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8793,6 +8793,7 @@ static void
tricore_tr_init_disas_context(DisasContextBase *dcbase,
     CPUTriCoreState *env = cs->env_ptr;
     ctx->mem_idx = cpu_mmu_index(env, false);
     ctx->hflags = (uint32_t)ctx->base.tb->flags;
+    ctx->env = env;
 }

 static void tricore_tr_tb_start(DisasContextBase *db, CPUState *cpu)


Aside to Bastian: passing the CPU env pointer into the
DisasContext isn't ideal, because it makes it quite easy
for translate.c code to accidentally use fields of the
env struct that aren't valid for use at translate time.
I think the only uses of ctx->env you have are for checking
feature bits -- I recommend following what target/arm does here:
 * in tricore_tr_init_disas_context(), instead of copying the
   env pointer, just copy env->features into ctx->features
 * have a tricore_dc_feature(DisasContext *dc, int feature)
   that checks for the feature bit in dc->features

That way you only have access in translate.c code to
information that's safe to bake into generated code, and
it's harder to accidentally introduce bugs where generated
code depends on CPU state that isn't kept in the TB flags.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-discuss] Segmentation fault on target tricore

[Bastian Koppelmann]

Hi Peter,

On 9/17/19 3:45 PM, Peter Maydell wrote:
> On Tue, 17 Sep 2019 at 14:06, Konopik, Andreas <andreas.konopik@fau.de> wrote:
>>>> Using gdb and valgrind I found out that:
>>>> - 'gen_mtcr()' and 'gen_mfcr()' access uninitialized values, i.e.
>>>> CSFRs,
>>>> which leads to the Segfault
>>>> - The uninitialised values were created by stack allocation of
>>>> DisasContext in 'gen_intermediate_code()'
>>> This definitely sounds like a bug: do you have a stack
>>> backtrace from valgrind or gdb of the bad access and the
>>> segfault?
>>>
>>> [...]
>>> Thread 3 "qemu-system-tri" received signal SIGSEGV, Segmentation fault.
>>> [Switching to Thread 0x7ffff10a4700 (LWP 146730)]
>>> 0x00005555556edb67 in gen_mfcr (ret=0xab0, offset=<optimized out>,
>>>     ctx=<optimized out>)
>>>     at /home/akonopik/qemu_src/target/tricore/cpu.h:274
>>> 274       return (env->features & (1ULL << feature)) != 0;
>>> (gdb) bt
>>> #0  0x00005555556edb67 in gen_mfcr
>>>     (ret=0xab0, offset=<optimized out>, ctx=<optimized out>)
>>>     at /home/akonopik/qemu_src/target/tricore/cpu.h:274
> It looks like tricore_tr_init_disas_context() isn't
> initializing ctx->env. If this is the problem then this
> patch ought to fix it:
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index c574638c9f7..305d896cd2c 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8793,6 +8793,7 @@ static void
> tricore_tr_init_disas_context(DisasContextBase *dcbase,
>       CPUTriCoreState *env = cs->env_ptr;
>       ctx->mem_idx = cpu_mmu_index(env, false);
>       ctx->hflags = (uint32_t)ctx->base.tb->flags;
> +    ctx->env = env;
>   }
>
>   static void tricore_tr_tb_start(DisasContextBase *db, CPUState *cpu)

thanks for the patch. I'll add it to my queue.


>
>
> Aside to Bastian: passing the CPU env pointer into the
> DisasContext isn't ideal, because it makes it quite easy
> for translate.c code to accidentally use fields of the
> env struct that aren't valid for use at translate time.
> I think the only uses of ctx->env you have are for checking
> feature bits -- I recommend following what target/arm does here:
>   * in tricore_tr_init_disas_context(), instead of copying the
>     env pointer, just copy env->features into ctx->features
>   * have a tricore_dc_feature(DisasContext *dc, int feature)
>     that checks for the feature bit in dc->features
>
> That way you only have access in translate.c code to
> information that's safe to bake into generated code, and
> it's harder to accidentally introduce bugs where generated
> code depends on CPU state that isn't kept in the TB flags.


Yes, this is not intended to stay this way. However, it was a necessary 
change for the translate_loop conversion. I'll try removing ctx->env 
from TriCore.

Cheers,

Bastian


>
> thanks
> -- PMM
>