qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] riscv/gdb: add V bit to priv register


From: Yanfeng Liu
Subject: Re: [PATCH v3] riscv/gdb: add V bit to priv register
Date: Sat, 14 Dec 2024 10:01:28 +0800
User-agent: Evolution 3.44.4-0ubuntu2

On Fri, 2024-12-13 at 10:04 +0100, Mario Fleischmann wrote:
> Hi,
> 
> apologies for the delayed review; I've just gotten to it now.
> 
> On 06.12.2024 01:14, Yanfeng Liu wrote:
> > This adds virtualization mode (V bit) as bit(2) of register `priv`
> > per RiscV debug spec v1.0.0-rc3. Checked with gdb-multiarch v12.1.
> > 
> > Note that GDB may display `INVALID` tag for the value when V bit
> > is set, this doesn't affect accessing to the bit.
> > 
> > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
> > ---
> >   target/riscv/gdbstub.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index c07df972f1..8cc095cda3 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -213,7 +213,10 @@ static int riscv_gdb_get_virtual(CPUState *cs,
> > GByteArray *buf, int n)
> >           RISCVCPU *cpu = RISCV_CPU(cs);
> >           CPURISCVState *env = &cpu->env;
> >   
> > -        return gdb_get_regl(buf, env->priv);
> > +        /* Per RiscV debug spec v1.0.0 rc3 */
> 
> Now that rc4 is released, you might also cite "RISC-V Debug
> Specification v1.0.0 rc4".
Okay, will do.
> > +        target_ulong vbit = (env->virt_enabled) ? BIT(2) : 0;
> > +
> > +        return gdb_get_regl(buf, env->priv | vbit);
> >   #endif
> >       }
> >       return 0;
> > @@ -230,6 +233,8 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t
> > *mem_buf, int n)
> >           if (env->priv == PRV_RESERVED) {
> >               env->priv = PRV_S;
> >           }
> > +        env->virt_enabled = (env->priv == PRV_M) ? 0 :
> > +                            ((ldtul_p(mem_buf) & BIT(2)) >> 2);
> 
> Looking at the other places in the source code where virt_enabled is 
> set, we should also check here if the H extension is active.
> Alternatively, you might also consider using riscv_cpu_set_mode():
> 
> Message-ID: <20240711-smcntrpmf_v7-v8-1-b7c38ae7b263@rivosinc.com>
> Date: Thu, 11 Jul 2024 15:31:04 -0700
> Subject: [PATCH v8 01/13] target/riscv: Combine set_mode and set_virt 
> functions.
> From: Atish Patra <atishp@rivosinc.com>
> 
Thanks for this, I will check both and see what is easier for a QEMU newbie
later.

> > 
> >   #endif
> >           return sizeof(target_ulong);
> >       }
> 
> In addition, we need to swap the virtual supervisor registers from the H 
> extension, e.g. vsstatus:
> 
> "When V=1, vsstatus substitutes for the usual sstatus, so instructions 
> that normally read or modify sstatus actually access vsstatus instead." 
> (privileged spec)
> 
> With the current patch, I was able to read and modify V, but the 
> registers were not changing:
> 
> (gdb) info register $priv
> priv           0x4      prv:4 [INVALID]
> (gdb) info register $sstatus
> sstatus        0x200004022      8589951010
> (gdb) set $priv = 0x0
> (gdb) info register $priv
> priv           0x0      prv:0 [User/Application]
> (gdb) info register $sstatus
> sstatus        0x200004022      8589951010
> 
> Take a look at riscv_cpu_swap_hypervisor_regs() which I believe does the 
> thing we need here. Note that the function is supposed be called before 
> the mode switch.

thanks again, I will check that swapping method and use it before changing the v
bit. 

Regards,
yf





reply via email to

[Prev in Thread] Current Thread [Next in Thread]