From: Georg Hofstetter <georg.hofstetter@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index db09f82c31..cdbc00d654 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     target_ulong pc_start;
     int num_insns = 0;
 
+    memset(&ctx, 0x00, sizeof(DisasContext));
     pc_start = tb->pc;
     ctx.pc = pc_start;
     ctx.saved_pc = -1;
-- 
2.17.1

Hi,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: Georg Hofstetter <georg.hofstetter@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/translate.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index db09f82c31..cdbc00d654 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>       target_ulong pc_start;
>       int num_insns = 0;
>   
> +    memset(&ctx, 0x00, sizeof(DisasContext));
>       pc_start = tb->pc;
>       ctx.pc = pc_start;
>       ctx.saved_pc = -1;

To me this looks like fixing a symptom instead of the root cause. Which 
commit did you bisect to? Do you have a reproducer?

Cheers,

Bastian

Hi Sebastian,

in translate.c:gen_mtcr() code accesses hflags within the structure:
    if ((ctx->hflags & TRICORE_HFLAG_KUU) == TRICORE_HFLAG_SM) {
        /* since we're caching PSW make this a special case */

The code expects the HFLAG set for kernel mode in (i guess) preparation for supporting operation modes.
There is no code modifying these flags.
The flags were introduced in 0aaeb11 and there it looks like it was expected to be zeroed - maybe you know a bit more.

As having a stack variable uninitialized is generally a bad idea, so my proposal was to zero it as a whole, as it would be for bss variables.
Alternatively we could initialize them explicitly to zero or TRICORE_HFLAG_SM.

    ctx.hflags = TRICORE_HFLAG_SM;

Not sure though why a cpu state flag is within DisasContext.

Regards,
Georg


-----Ursprüngliche Nachricht-----
Von: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> 
Gesendet: Mittwoch, 5. Juni 2019 11:02
An: David Brenken <david.brenken@efs-auto.org>; qemu-devel@nongnu.org
Cc: Biermanski, Lars (EFS-GH3) <lars.biermanski@efs-auto.de>; Hofstetter, Georg (EFS-GH2) <Georg.Hofstetter@efs-auto.de>; Brenken, David (EFS-GH2) <david.brenken@efs-auto.de>; Rasche, Robert (EFS-GH2) <robert.rasche@efs-auto.de>; Konopik, Andreas (EFS-GH2) <andreas.konopik@efs-auto.de>
Betreff: Re: [Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

Hi,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: Georg Hofstetter <georg.hofstetter@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/translate.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c 
> index db09f82c31..cdbc00d654 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>       target_ulong pc_start;
>       int num_insns = 0;
>   
> +    memset(&ctx, 0x00, sizeof(DisasContext));
>       pc_start = tb->pc;
>       ctx.pc = pc_start;
>       ctx.saved_pc = -1;

To me this looks like fixing a symptom instead of the root cause. Which commit did you bisect to? Do you have a reproducer?

Cheers,

Bastian

Hi Georg,

On 6/6/19 1:44 PM, Hofstetter, Georg (EFS-GH2) wrote:
> Hi Sebastian,
>
> in translate.c:gen_mtcr() code accesses hflags within the structure:
>      if ((ctx->hflags & TRICORE_HFLAG_KUU) == TRICORE_HFLAG_SM) {
>          /* since we're caching PSW make this a special case */
>
> The code expects the HFLAG set for kernel mode in (i guess) preparation for supporting operation modes.
> There is no code modifying these flags.
> The flags were introduced in 0aaeb11 and there it looks like it was expected to be zeroed - maybe you know a bit more.

Yep, the ctx->hflags is supposed to be synced by tb->flags (which is 
normally synced with CPUTriCoreState via cpu_get_tb_cpu_state()) in 
gen_intermediate_code(). Somehow I forgot to add the first sync. So, the 
proper fix is:

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 06c4485e55..44296b3fb1 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8804,6 +8804,7 @@ void gen_intermediate_code(CPUState *cs, 
TranslationBlock *tb, int max_insns)
      ctx.singlestep_enabled = cs->singlestep_enabled;
      ctx.bstate = BS_NONE;
      ctx.mem_idx = cpu_mmu_index(env, false);
+    ctx.hflags = (uint32_t)tb->flags;


      tcg_clear_temp_count();
      gen_tb_start(tb);


Cheers,

Bastian