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 <[email protected]> > > --- > > 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: <[email protected]> > 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 <[email protected]> > 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
