Re: [PATCH 2/2] target/riscv: Make the "virt" register writable by GDB

[LIU Zhiwei]

On 2023/3/9 11:05, Jim Shu wrote:
> On Mon, Mar 6, 2023 at 7:26 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> > On 2023/3/5 17:42, Jim Shu wrote:
> > > This patch also enables debugger to set current privilege mode to
> > > VU/VS-mode.
> > >
> > > Extend previous commit 81d2929c41d32af138f3562f5a7b309f6eac7ca7 to
> > > support H-extension.
> > >
> > > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > > ---
> > >    target/riscv/gdbstub.c | 18 ++++++++++++++++--
> > >    1 file changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > > index 1755fd9d51..a7f234beaf 100644
> > > --- a/target/riscv/gdbstub.c
> > > +++ b/target/riscv/gdbstub.c
> > > @@ -203,15 +203,29 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs, 
> > > GByteArray *buf, int n)
> > >
> > >    static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
> > >    {
> > > +#ifdef CONFIG_USER_ONLY
> > > +    if (n >= 0 && n <= 1) {
> > > +        return sizeof(target_ulong);
> > > +    }
> > > +#else
> > > +    bool virt;
> > > +
> > >        if (n == 0) {
> > > -#ifndef CONFIG_USER_ONLY
> > >            cs->priv = ldtul_p(mem_buf) & 0x3;
> > >            if (cs->priv == PRV_H) {
> > >                cs->priv = PRV_S;
> > >            }
> > > -#endif
> > > +        return sizeof(target_ulong);
> > We should return according to the misa_mxl_max. And this is a bug before
> > your commit.
> Hi Zhiwei,
>
> After reading other gdbstub.c code, I think it is OK to use
> 'sizeof(target_ulong)' as virtual register length.

No, you should refer to the riscv_cpu_gdb_read(write)_register.

> Its length is 32-bit in RV32 and is 64-bit in RV64/RV128.
Although we don't support MXLEN 32bit  currently on qemu-system-riscv64, 
we should not introduce more code
to make dynamic xlen support complex.

Zhiwei

> We don't
> need to handle RV128 specially.
> Virtual register length is same as CSR length and
> 'riscv_gdb_set_csr()' also use 'sizeof(target_ulong)'.
>
> Jim Shu
>
> > > +    } else if (n == 1) {
> > > +        virt = ldtul_p(mem_buf) & 0x1;
> > > +        if ((cs->priv == PRV_M) && (virt == true)) {
> > > +            /* M-mode only supports V=0. */
> > > +            virt = false;
> > > +        }
> > > +        riscv_cpu_set_virt_enabled(cs, virt);
> > >            return sizeof(target_ulong);
> > Same error here. Otherwise,
> >
> > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> >
> > Zhiwei
> >
> > >        }
> > > +#endif
> > >        return 0;
> > >    }