Re: [PATCH] target/riscv: Dump sstatus CSR in riscv_cpu_dump_state()

[Bin Meng <bmeng.cn@gmail.com>]

On Fri, Nov 25, 2022 at 7:58 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Nov 23, 2022 at 2:07 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Alistair,
> >
> > On Wed, Nov 23, 2022 at 8:03 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 2:07 AM Bin Meng <bmeng@tinylab.org> wrote:
> > > >
> > > > sstatus register dump is currently missing in riscv_cpu_dump_state().
> > > >
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1332
> > > > Signed-off-by: Bin Meng <bmeng@tinylab.org>
> > > >
> > > > ---
> > > >
> > > >  target/riscv/cpu.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index d14e95c9dc..80d76f0181 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -382,6 +382,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> > > >              CSR_MHARTID,
> > > >              CSR_MSTATUS,
> > > >              CSR_MSTATUSH,
> > > > +            CSR_SSTATUS,
> > >
> > > I don't think we need this. mstatus contains all of the information
> > > already and there is limited space to print all of this information
> > > out.
> > >
> >
> > I am not sure what limited space restricts this? This is CPU state
> > dump, and printing sstatus CSR seems reasonable to me. We do the
> > similar thing in the gdb stub too.
>
> Limited space in that there is only so much text we want to dump to
> the screen. As new extensions are added this will continue to grow,
> and sstatus doesn't provide any more information then the mstatus
> register that we already show.
>

On a screen, yes, but we can log to a file so there is no size limitation.

> GDB is a little different as people can interactively probe the
> registers they are interested in.

The dump routine is one of the debug methods too. If gdb stub provides
$sstatus directly I think we should do the same for dump_state for
consistency. Otherwise we can just provide $mstatus in gdb stub and
let user figure out sstatus value.

Regards,
Bin