[v1,1/1] target/riscv: Print CPU and privledge in disas

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/translate.c | 7 +++++++
 1 file changed, 7 insertions(+)

On Fri, Sep 27, 2019 at 8:55 AM Alistair Francis
<alistair.francis@wdc.com> wrote:

typo "privledge" in the commit title

>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/translate.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index adeddb85f6..537af0003e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -810,7 +810,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>
>  static void riscv_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
>  {
> +#ifndef CONFIG_USER_ONLY
> +    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> +    CPURISCVState *env = &rvcpu->env;
> +#endif
>      qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> +#ifndef CONFIG_USER_ONLY
> +    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);

Since this patch wants to be helpful for debugging, would it make more
sense to print out the priv mode string instead of the number, eg:
priv: M.

But I am fine with just printing out the number.

> +#endif
>      log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
>  }

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>

On 9/27/19 2:45 AM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/translate.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index adeddb85f6..537af0003e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -810,7 +810,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>  
>  static void riscv_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
>  {
> +#ifndef CONFIG_USER_ONLY
> +    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> +    CPURISCVState *env = &rvcpu->env;
> +#endif
>      qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> +#ifndef CONFIG_USER_ONLY
> +    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
> +#endif

Nit: can be extracted as another function to reduce the ifdef'ry and
simply use:

       log_cpu_priv(cpu);

>      log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
>  }
>  

With function previously defined:

static void log_cpu_priv(const CPUState *cpu)
{
#ifndef CONFIG_USER_ONLY
    RISCVCPU *rvcpu = RISCV_CPU(cpu);
    CPURISCVState *env = &rvcpu->env;

    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
#endif
}

Anyway, fixing the typo in the patch subject:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On Fri, Sep 27, 2019 at 2:10 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 9/27/19 2:45 AM, Alistair Francis wrote:
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/translate.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index adeddb85f6..537af0003e 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -810,7 +810,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
> >
> >  static void riscv_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
> >  {
> > +#ifndef CONFIG_USER_ONLY
> > +    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> > +    CPURISCVState *env = &rvcpu->env;
> > +#endif
> >      qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> > +#ifndef CONFIG_USER_ONLY
> > +    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
> > +#endif
>
> Nit: can be extracted as another function to reduce the ifdef'ry and
> simply use:
>
>        log_cpu_priv(cpu);
>
> >      log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
> >  }
> >
>
> With function previously defined:
>
> static void log_cpu_priv(const CPUState *cpu)
> {
> #ifndef CONFIG_USER_ONLY
>     RISCVCPU *rvcpu = RISCV_CPU(cpu);
>     CPURISCVState *env = &rvcpu->env;
>
>     qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
> #endif
> }
>
> Anyway, fixing the typo in the patch subject:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks I have included your comment and Bin's comments in v2.

Alistair

>

On 9/26/19 5:45 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/translate.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index adeddb85f6..537af0003e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -810,7 +810,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>  
>  static void riscv_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
>  {
> +#ifndef CONFIG_USER_ONLY
> +    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> +    CPURISCVState *env = &rvcpu->env;
> +#endif
>      qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> +#ifndef CONFIG_USER_ONLY
> +    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
> +#endif
>      log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
>  }

I'm curious about the motivation here.

In particular, what difference does it make what cpu the TB is generated for?
It would seem like the more relevant place to look for this is with -d cpu or
-d exec where the TB is actually executed, which could well be multiple cpus.

As for env->priv... is there really anything in the input assembly that depends
on the privilege?  Or conversely, why priv and not all of the other bits that
are in tb_flags?  And are you placing that here in -d in_asm because are always
also using either -d op or -d op_opt, and it somehow looks better to have this
at the top?


r~

On Fri, Sep 27, 2019 at 10:18 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/26/19 5:45 PM, Alistair Francis wrote:
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/translate.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index adeddb85f6..537af0003e 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -810,7 +810,14 @@ static void riscv_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
> >
> >  static void riscv_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
> >  {
> > +#ifndef CONFIG_USER_ONLY
> > +    RISCVCPU *rvcpu = RISCV_CPU(cpu);
> > +    CPURISCVState *env = &rvcpu->env;
> > +#endif
> >      qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> > +#ifndef CONFIG_USER_ONLY
> > +    qemu_log("CPU: %d; priv: "TARGET_FMT_ld"\n", cpu->cpu_index, env->priv);
> > +#endif
> >      log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
> >  }
>
> I'm curious about the motivation here.
>
> In particular, what difference does it make what cpu the TB is generated for?
> It would seem like the more relevant place to look for this is with -d cpu or
> -d exec where the TB is actually executed, which could well be multiple cpus.

The main reason is that the sifive_u machine now has two RISC-V
clusters so it's useful to see what is happening on each CPU. -d cpu
and -d exec are much more difficult to comprehend so aren't always as
useful.

>
> As for env->priv... is there really anything in the input assembly that depends
> on the privilege?  Or conversely, why priv and not all of the other bits that
> are in tb_flags?  And are you placing that here in -d in_asm because are always
> also using either -d op or -d op_opt, and it somehow looks better to have this
> at the top?

I think it's just clearer to see what privileged mode the instructions
were generated for, it helps with debugging boot issues. Again it
comes back to the other options being difficult to comprehend, in_asm
I find is the clearest to understand (although it has lots of
limitations).

Alistair

>
>
> r~

On 9/27/19 10:23 AM, Alistair Francis wrote:
>> I'm curious about the motivation here.
>>
>> In particular, what difference does it make what cpu the TB is generated for?
>> It would seem like the more relevant place to look for this is with -d cpu or
>> -d exec where the TB is actually executed, which could well be multiple cpus.
> 
> The main reason is that the sifive_u machine now has two RISC-V
> clusters so it's useful to see what is happening on each CPU. -d cpu
> and -d exec are much more difficult to comprehend so aren't always as
> useful.

Hmm.  I wonder if it might be time to find a way to be able to optionally split
the qemu_log output on a per-cpu basis.  It wouldn't necessarily always be
helpful, because we'd still have the case of a given TB being translated by one
cpu and executed on another, but it could make some things easier to follow.


r~

On Fri, Sep 27, 2019 at 10:53 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/27/19 10:23 AM, Alistair Francis wrote:
> >> I'm curious about the motivation here.
> >>
> >> In particular, what difference does it make what cpu the TB is generated for?
> >> It would seem like the more relevant place to look for this is with -d cpu or
> >> -d exec where the TB is actually executed, which could well be multiple cpus.
> >
> > The main reason is that the sifive_u machine now has two RISC-V
> > clusters so it's useful to see what is happening on each CPU. -d cpu
> > and -d exec are much more difficult to comprehend so aren't always as
> > useful.
>
> Hmm.  I wonder if it might be time to find a way to be able to optionally split
> the qemu_log output on a per-cpu basis.  It wouldn't necessarily always be
> helpful, because we'd still have the case of a given TB being translated by one
> cpu and executed on another, but it could make some things easier to follow.

A per cpu-cluster basis would be useful.

Until then though this patch helps.

Alistair

>
>
> r~