* [PATCH v4 0/3] RISC-V: Populate mtval and stval
@ 2021-12-20 6:49 Alistair Francis
2021-12-20 6:49 ` [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext Alistair Francis
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Alistair Francis @ 2021-12-20 6:49 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng
From: Alistair Francis <alistair.francis@wdc.com>
Populate mtval and stval when taking an illegal instruction exception.
The RISC-V spec states that "The stval register can optionally also be
used to return the faulting instruction bits on an illegal instruction
exception...". In this case we are always writing the value on an
illegal instruction.
This doesn't match all CPUs (some CPUs won't write the data), but in
QEMU let's just populate the value on illegal instructions. This won't
break any guest software, but will provide more information to guests.
Alistair Francis (3):
target/riscv: Set the opcode in DisasContext
target/riscv: Fixup setting GVA
target/riscv: Implement the stval/mtval illegal instruction
target/riscv/cpu.h | 2 ++
target/riscv/cpu_helper.c | 24 +++++++++---------------
target/riscv/translate.c | 5 +++++
3 files changed, 16 insertions(+), 15 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext 2021-12-20 6:49 [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis @ 2021-12-20 6:49 ` Alistair Francis 2021-12-21 7:08 ` Bin Meng 2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Alistair Francis @ 2021-12-20 6:49 UTC (permalink / raw) To: qemu-riscv, qemu-devel Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng, Richard Henderson From: Alistair Francis <alistair.francis@wdc.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/riscv/translate.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 1d57bc97b5..24251bc8cc 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -586,6 +586,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) if (!has_ext(ctx, RVC)) { gen_exception_illegal(ctx); } else { + ctx->opcode = opcode; ctx->pc_succ_insn = ctx->base.pc_next + 2; if (!decode_insn16(ctx, opcode)) { gen_exception_illegal(ctx); @@ -596,6 +597,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) opcode32 = deposit32(opcode32, 16, 16, translator_lduw(env, &ctx->base, ctx->base.pc_next + 2)); + ctx->opcode = opcode32; ctx->pc_succ_insn = ctx->base.pc_next + 4; if (!decode_insn32(ctx, opcode32)) { gen_exception_illegal(ctx); -- 2.31.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext 2021-12-20 6:49 ` [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext Alistair Francis @ 2021-12-21 7:08 ` Bin Meng 0 siblings, 0 replies; 12+ messages in thread From: Bin Meng @ 2021-12-21 7:08 UTC (permalink / raw) To: Alistair Francis Cc: open list:RISC-V, Bin Meng, Richard Henderson, qemu-devel@nongnu.org Developers, Alistair Francis, Alistair Francis, Palmer Dabbelt On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/riscv/translate.c | 2 ++ > 1 file changed, 2 insertions(+) > I remember I once reviewed the whole series. Not sure what changed in the versions Reviewed-by: Bin Meng <bmeng.cn@gmail.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/3] target/riscv: Fixup setting GVA 2021-12-20 6:49 [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis 2021-12-20 6:49 ` [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext Alistair Francis @ 2021-12-20 6:49 ` Alistair Francis 2021-12-20 19:38 ` Richard Henderson 2021-12-21 7:30 ` Bin Meng 2021-12-20 6:49 ` [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis 2022-01-07 3:33 ` [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis 3 siblings, 2 replies; 12+ messages in thread From: Alistair Francis @ 2021-12-20 6:49 UTC (permalink / raw) To: qemu-riscv, qemu-devel Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng From: Alistair Francis <alistair.francis@wdc.com> In preperation for adding support for the illegal instruction address let's fixup the Hypervisor extension setting GVA logic and improve the variable names. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- target/riscv/cpu_helper.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 9eeed38c7e..9e1f5ee177 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) RISCVCPU *cpu = RISCV_CPU(cs); CPURISCVState *env = &cpu->env; + bool write_gva = false; uint64_t s; /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide @@ -975,7 +976,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; target_ulong deleg = async ? env->mideleg : env->medeleg; - bool write_tval = false; target_ulong tval = 0; target_ulong htval = 0; target_ulong mtval2 = 0; @@ -1004,7 +1004,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) case RISCV_EXCP_INST_PAGE_FAULT: case RISCV_EXCP_LOAD_PAGE_FAULT: case RISCV_EXCP_STORE_PAGE_FAULT: - write_tval = true; + write_gva = true; tval = env->badaddr; break; default: @@ -1041,18 +1041,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) if (riscv_has_ext(env, RVH)) { target_ulong hdeleg = async ? env->hideleg : env->hedeleg; - if (env->two_stage_lookup && write_tval) { - /* - * If we are writing a guest virtual address to stval, set - * this to 1. If we are trapping to VS we will set this to 0 - * later. - */ - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1); - } else { - /* For other HS-mode traps, we set this to 0. */ - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); - } - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { /* Trap to VS mode */ /* @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) cause == IRQ_VS_EXT) { cause = cause - 1; } - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); + write_gva = false; } else if (riscv_cpu_virt_enabled(env)) { /* Trap into HS mode, from virt */ riscv_cpu_swap_hypervisor_regs(env); @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) env->hstatus = set_field(env->hstatus, HSTATUS_SPV, riscv_cpu_virt_enabled(env)); + htval = env->guest_phys_fault_addr; riscv_cpu_set_virt_enabled(env, 0); @@ -1079,7 +1068,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) /* Trap into HS mode */ env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); htval = env->guest_phys_fault_addr; + write_gva = false; } + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva); } s = env->mstatus; -- 2.31.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA 2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis @ 2021-12-20 19:38 ` Richard Henderson 2021-12-21 7:30 ` Bin Meng 1 sibling, 0 replies; 12+ messages in thread From: Richard Henderson @ 2021-12-20 19:38 UTC (permalink / raw) To: Alistair Francis, qemu-riscv, qemu-devel Cc: Bin Meng, Palmer Dabbelt, bmeng.cn, Alistair Francis, alistair23 On 12/19/21 10:49 PM, Alistair Francis wrote: > From: Alistair Francis<alistair.francis@wdc.com> > > In preperation for adding support for the illegal instruction address > let's fixup the Hypervisor extension setting GVA logic and improve the > variable names. > > Signed-off-by: Alistair Francis<alistair.francis@wdc.com> > --- > target/riscv/cpu_helper.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA 2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis 2021-12-20 19:38 ` Richard Henderson @ 2021-12-21 7:30 ` Bin Meng 2022-01-06 4:04 ` Alistair Francis 1 sibling, 1 reply; 12+ messages in thread From: Bin Meng @ 2021-12-21 7:30 UTC (permalink / raw) To: Alistair Francis Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers, Alistair Francis, Alistair Francis, Palmer Dabbelt On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > In preperation for adding support for the illegal instruction address typo: preparation > let's fixup the Hypervisor extension setting GVA logic and improve the > variable names. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/cpu_helper.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 9eeed38c7e..9e1f5ee177 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > + bool write_gva = false; > uint64_t s; > > /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > @@ -975,7 +976,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; > target_ulong deleg = async ? env->mideleg : env->medeleg; > - bool write_tval = false; > target_ulong tval = 0; > target_ulong htval = 0; > target_ulong mtval2 = 0; > @@ -1004,7 +1004,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > case RISCV_EXCP_INST_PAGE_FAULT: > case RISCV_EXCP_LOAD_PAGE_FAULT: > case RISCV_EXCP_STORE_PAGE_FAULT: > - write_tval = true; > + write_gva = true; > tval = env->badaddr; > break; > default: > @@ -1041,18 +1041,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > if (riscv_has_ext(env, RVH)) { > target_ulong hdeleg = async ? env->hideleg : env->hedeleg; > > - if (env->two_stage_lookup && write_tval) { > - /* > - * If we are writing a guest virtual address to stval, set > - * this to 1. If we are trapping to VS we will set this to 0 > - * later. > - */ > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1); > - } else { > - /* For other HS-mode traps, we set this to 0. */ > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > - } > - > if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > /* Trap to VS mode */ > /* > @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > cause == IRQ_VS_EXT) { > cause = cause - 1; > } > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > + write_gva = false; > } else if (riscv_cpu_virt_enabled(env)) { > /* Trap into HS mode, from virt */ > riscv_cpu_swap_hypervisor_regs(env); > @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, > riscv_cpu_virt_enabled(env)); > > + > htval = env->guest_phys_fault_addr; > > riscv_cpu_set_virt_enabled(env, 0); > @@ -1079,7 +1068,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) > /* Trap into HS mode */ > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > htval = env->guest_phys_fault_addr; > + write_gva = false; > } > + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva); This does not look correct to me. The env->hstatus[GVA] should remain untouched in the 2nd and 3rd branch. It only needs to be set in the first branch. > } > > s = env->mstatus; Regards, Bin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA 2021-12-21 7:30 ` Bin Meng @ 2022-01-06 4:04 ` Alistair Francis 2022-01-07 2:07 ` Bin Meng 0 siblings, 1 reply; 12+ messages in thread From: Alistair Francis @ 2022-01-06 4:04 UTC (permalink / raw) To: Bin Meng Cc: Alistair Francis, open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers, Alistair Francis, Palmer Dabbelt On Tue, Dec 21, 2021 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis > <alistair.francis@opensource.wdc.com> wrote: > > > > From: Alistair Francis <alistair.francis@wdc.com> > > > > In preperation for adding support for the illegal instruction address > > typo: preparation Fixed > > > let's fixup the Hypervisor extension setting GVA logic and improve the > > variable names. > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > --- > > target/riscv/cpu_helper.c | 21 ++++++--------------- > > 1 file changed, 6 insertions(+), 15 deletions(-) > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > index 9eeed38c7e..9e1f5ee177 100644 > > --- a/target/riscv/cpu_helper.c > > +++ b/target/riscv/cpu_helper.c > > @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > > RISCVCPU *cpu = RISCV_CPU(cs); > > CPURISCVState *env = &cpu->env; > > + bool write_gva = false; > > uint64_t s; > > > > /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > > @@ -975,7 +976,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); > > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; > > target_ulong deleg = async ? env->mideleg : env->medeleg; > > - bool write_tval = false; > > target_ulong tval = 0; > > target_ulong htval = 0; > > target_ulong mtval2 = 0; > > @@ -1004,7 +1004,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > case RISCV_EXCP_INST_PAGE_FAULT: > > case RISCV_EXCP_LOAD_PAGE_FAULT: > > case RISCV_EXCP_STORE_PAGE_FAULT: > > - write_tval = true; > > + write_gva = true; > > tval = env->badaddr; > > break; > > default: > > @@ -1041,18 +1041,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > if (riscv_has_ext(env, RVH)) { > > target_ulong hdeleg = async ? env->hideleg : env->hedeleg; > > > > - if (env->two_stage_lookup && write_tval) { > > - /* > > - * If we are writing a guest virtual address to stval, set > > - * this to 1. If we are trapping to VS we will set this to 0 > > - * later. > > - */ > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1); > > - } else { > > - /* For other HS-mode traps, we set this to 0. */ > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > - } > > - > > if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > > /* Trap to VS mode */ > > /* > > @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > cause == IRQ_VS_EXT) { > > cause = cause - 1; > > } > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > + write_gva = false; > > } else if (riscv_cpu_virt_enabled(env)) { > > /* Trap into HS mode, from virt */ > > riscv_cpu_swap_hypervisor_regs(env); > > @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, > > riscv_cpu_virt_enabled(env)); > > > > + > > htval = env->guest_phys_fault_addr; > > > > riscv_cpu_set_virt_enabled(env, 0); > > @@ -1079,7 +1068,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > /* Trap into HS mode */ > > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > > htval = env->guest_phys_fault_addr; > > + write_gva = false; > > } > > + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva); > > This does not look correct to me. > > The env->hstatus[GVA] should remain untouched in the 2nd and 3rd > branch. It only needs to be set in the first branch. The RISC-V spec says: ""' Field GVA (Guest Virtual Address) is written by the implementation whenever a trap is taken into HS-mode. For any trap (breakpoint, address misaligned, access fault, page fault, or guest- page fault) that writes a guest virtual address to stval, GVA is set to 1. For any other trap into HS-mode, GVA is set to 0. """ So it has to be set in the second and third branches as they are trapping into HS mode. I guess we could not touch it in the first branch (Trap to VS mode), but that means we would need to ensure it is updated later, so it seems easiest to just clear it here. In the second branch (Trap into HS mode, from virt) we set GVA based on the trap cause, which seems correct. In the third case (Trap into HS mode) we are trapping from HS to HS so we want to clear GVA. Alistair > > > } > > > > s = env->mstatus; > > Regards, > Bin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA 2022-01-06 4:04 ` Alistair Francis @ 2022-01-07 2:07 ` Bin Meng 0 siblings, 0 replies; 12+ messages in thread From: Bin Meng @ 2022-01-07 2:07 UTC (permalink / raw) To: Alistair Francis Cc: Alistair Francis, open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers, Alistair Francis, Palmer Dabbelt On Thu, Jan 6, 2022 at 12:04 PM Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, Dec 21, 2021 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis > > <alistair.francis@opensource.wdc.com> wrote: > > > > > > From: Alistair Francis <alistair.francis@wdc.com> > > > > > > In preperation for adding support for the illegal instruction address > > > > typo: preparation > > Fixed > > > > > > let's fixup the Hypervisor extension setting GVA logic and improve the > > > variable names. > > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > > --- > > > target/riscv/cpu_helper.c | 21 ++++++--------------- > > > 1 file changed, 6 insertions(+), 15 deletions(-) > > > > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > > index 9eeed38c7e..9e1f5ee177 100644 > > > --- a/target/riscv/cpu_helper.c > > > +++ b/target/riscv/cpu_helper.c > > > @@ -967,6 +967,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > > > > RISCVCPU *cpu = RISCV_CPU(cs); > > > CPURISCVState *env = &cpu->env; > > > + bool write_gva = false; > > > uint64_t s; > > > > > > /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > > > @@ -975,7 +976,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > bool async = !!(cs->exception_index & RISCV_EXCP_INT_FLAG); > > > target_ulong cause = cs->exception_index & RISCV_EXCP_INT_MASK; > > > target_ulong deleg = async ? env->mideleg : env->medeleg; > > > - bool write_tval = false; > > > target_ulong tval = 0; > > > target_ulong htval = 0; > > > target_ulong mtval2 = 0; > > > @@ -1004,7 +1004,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > case RISCV_EXCP_INST_PAGE_FAULT: > > > case RISCV_EXCP_LOAD_PAGE_FAULT: > > > case RISCV_EXCP_STORE_PAGE_FAULT: > > > - write_tval = true; > > > + write_gva = true; > > > tval = env->badaddr; > > > break; > > > default: > > > @@ -1041,18 +1041,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > if (riscv_has_ext(env, RVH)) { > > > target_ulong hdeleg = async ? env->hideleg : env->hedeleg; > > > > > > - if (env->two_stage_lookup && write_tval) { > > > - /* > > > - * If we are writing a guest virtual address to stval, set > > > - * this to 1. If we are trapping to VS we will set this to 0 > > > - * later. > > > - */ > > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1); > > > - } else { > > > - /* For other HS-mode traps, we set this to 0. */ > > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > > - } > > > - > > > if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > > > /* Trap to VS mode */ > > > /* > > > @@ -1063,7 +1051,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > cause == IRQ_VS_EXT) { > > > cause = cause - 1; > > > } > > > - env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > > + write_gva = false; > > > } else if (riscv_cpu_virt_enabled(env)) { > > > /* Trap into HS mode, from virt */ > > > riscv_cpu_swap_hypervisor_regs(env); > > > @@ -1072,6 +1060,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, > > > riscv_cpu_virt_enabled(env)); > > > > > > + > > > htval = env->guest_phys_fault_addr; > > > > > > riscv_cpu_set_virt_enabled(env, 0); > > > @@ -1079,7 +1068,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > /* Trap into HS mode */ > > > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > > > htval = env->guest_phys_fault_addr; > > > + write_gva = false; > > > } > > > + env->hstatus = set_field(env->hstatus, HSTATUS_GVA, write_gva); > > > > This does not look correct to me. > > > > The env->hstatus[GVA] should remain untouched in the 2nd and 3rd > > branch. It only needs to be set in the first branch. > > The RISC-V spec says: > > ""' > Field GVA (Guest Virtual Address) is written by the implementation > whenever a trap is taken > into HS-mode. For any trap (breakpoint, address misaligned, access > fault, page fault, or guest- > page fault) that writes a guest virtual address to stval, GVA is set > to 1. For any other trap into > HS-mode, GVA is set to 0. > """ > > So it has to be set in the second and third branches as they are > trapping into HS mode. I guess we could not touch it in the first > branch (Trap to VS mode), but that means we would need to ensure it is > updated later, so it seems easiest to just clear it here. Thanks for the info. > > In the second branch (Trap into HS mode, from virt) we set GVA based > on the trap cause, which seems correct. > > In the third case (Trap into HS mode) we are trapping from HS to HS so > we want to clear GVA. With these explanations, I feel this patch mixed up two things into one patch. The changes to 2nd and 3rd branch probably merit a separate patch with a better commit message as it *explicitely* clear or set GVA field as per the spec while current codes don't do that. A follow-up patch can be made to consolidate other "write_tval" cases. But that's up to you. FWIW: Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Regards, Bin ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction 2021-12-20 6:49 [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis 2021-12-20 6:49 ` [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext Alistair Francis 2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis @ 2021-12-20 6:49 ` Alistair Francis 2021-12-20 19:39 ` Richard Henderson 2021-12-21 7:46 ` Bin Meng 2022-01-07 3:33 ` [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis 3 siblings, 2 replies; 12+ messages in thread From: Alistair Francis @ 2021-12-20 6:49 UTC (permalink / raw) To: qemu-riscv, qemu-devel Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng From: Alistair Francis <alistair.francis@wdc.com> The stval and mtval registers can optionally contain the faulting instruction on an illegal instruction exception. This patch adds support for setting the stval and mtval registers. The RISC-V spec states that "The stval register can optionally also be used to return the faulting instruction bits on an illegal instruction exception...". In this case we are always writing the value on an illegal instruction. This doesn't match all CPUs (some CPUs won't write the data), but in QEMU let's just populate the value on illegal instructions. This won't break any guest software, but will provide more information to guests. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- target/riscv/cpu.h | 2 ++ target/riscv/cpu_helper.c | 3 +++ target/riscv/translate.c | 3 +++ 3 files changed, 8 insertions(+) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 0760c0af93..3a163b57ed 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -127,6 +127,8 @@ struct CPURISCVState { target_ulong frm; target_ulong badaddr; + uint32_t bins; + target_ulong guest_phys_fault_addr; target_ulong priv_ver; diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 9e1f5ee177..f76ba834e6 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -1007,6 +1007,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) write_gva = true; tval = env->badaddr; break; + case RISCV_EXCP_ILLEGAL_INST: + tval = env->bins; + break; default: break; } diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 24251bc8cc..921ca06bf9 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -167,6 +167,9 @@ static void generate_exception_mtval(DisasContext *ctx, int excp) static void gen_exception_illegal(DisasContext *ctx) { + tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env, + offsetof(CPURISCVState, bins)); + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction 2021-12-20 6:49 ` [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis @ 2021-12-20 19:39 ` Richard Henderson 2021-12-21 7:46 ` Bin Meng 1 sibling, 0 replies; 12+ messages in thread From: Richard Henderson @ 2021-12-20 19:39 UTC (permalink / raw) To: Alistair Francis, qemu-riscv, qemu-devel Cc: Bin Meng, Palmer Dabbelt, bmeng.cn, Alistair Francis, alistair23 On 12/19/21 10:49 PM, Alistair Francis wrote: > From: Alistair Francis<alistair.francis@wdc.com> > > The stval and mtval registers can optionally contain the faulting > instruction on an illegal instruction exception. This patch adds support > for setting the stval and mtval registers. > > The RISC-V spec states that "The stval register can optionally also be > used to return the faulting instruction bits on an illegal instruction > exception...". In this case we are always writing the value on an > illegal instruction. > > This doesn't match all CPUs (some CPUs won't write the data), but in > QEMU let's just populate the value on illegal instructions. This won't > break any guest software, but will provide more information to guests. > > Signed-off-by: Alistair Francis<alistair.francis@wdc.com> > --- > target/riscv/cpu.h | 2 ++ > target/riscv/cpu_helper.c | 3 +++ > target/riscv/translate.c | 3 +++ > 3 files changed, 8 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction 2021-12-20 6:49 ` [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis 2021-12-20 19:39 ` Richard Henderson @ 2021-12-21 7:46 ` Bin Meng 1 sibling, 0 replies; 12+ messages in thread From: Bin Meng @ 2021-12-21 7:46 UTC (permalink / raw) To: Alistair Francis Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers, Alistair Francis, Alistair Francis, Palmer Dabbelt On Mon, Dec 20, 2021 at 2:49 PM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > The stval and mtval registers can optionally contain the faulting > instruction on an illegal instruction exception. This patch adds support > for setting the stval and mtval registers. > > The RISC-V spec states that "The stval register can optionally also be > used to return the faulting instruction bits on an illegal instruction > exception...". In this case we are always writing the value on an > illegal instruction. > > This doesn't match all CPUs (some CPUs won't write the data), but in > QEMU let's just populate the value on illegal instructions. This won't > break any guest software, but will provide more information to guests. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > target/riscv/cpu.h | 2 ++ > target/riscv/cpu_helper.c | 3 +++ > target/riscv/translate.c | 3 +++ > 3 files changed, 8 insertions(+) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 0760c0af93..3a163b57ed 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -127,6 +127,8 @@ struct CPURISCVState { > target_ulong frm; > > target_ulong badaddr; > + uint32_t bins; nits: does "badins" sound a better name? I took some time to figure out what "bins" means :) > + > target_ulong guest_phys_fault_addr; > > target_ulong priv_ver; > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 9e1f5ee177..f76ba834e6 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -1007,6 +1007,9 @@ void riscv_cpu_do_interrupt(CPUState *cs) > write_gva = true; > tval = env->badaddr; > break; > + case RISCV_EXCP_ILLEGAL_INST: > + tval = env->bins; > + break; > default: > break; > } > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 24251bc8cc..921ca06bf9 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -167,6 +167,9 @@ static void generate_exception_mtval(DisasContext *ctx, int excp) > > static void gen_exception_illegal(DisasContext *ctx) > { > + tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env, > + offsetof(CPURISCVState, bins)); > + > generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); > } > Otherwise, Reviewed-by: Bin Meng <bmeng.cn@gmail.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 0/3] RISC-V: Populate mtval and stval 2021-12-20 6:49 [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis ` (2 preceding siblings ...) 2021-12-20 6:49 ` [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis @ 2022-01-07 3:33 ` Alistair Francis 3 siblings, 0 replies; 12+ messages in thread From: Alistair Francis @ 2022-01-07 3:33 UTC (permalink / raw) To: Alistair Francis Cc: open list:RISC-V, Bin Meng, qemu-devel@nongnu.org Developers, Alistair Francis, Palmer Dabbelt, Bin Meng On Mon, Dec 20, 2021 at 4:49 PM Alistair Francis <alistair.francis@opensource.wdc.com> wrote: > > From: Alistair Francis <alistair.francis@wdc.com> > > Populate mtval and stval when taking an illegal instruction exception. > > The RISC-V spec states that "The stval register can optionally also be > used to return the faulting instruction bits on an illegal instruction > exception...". In this case we are always writing the value on an > illegal instruction. > > This doesn't match all CPUs (some CPUs won't write the data), but in > QEMU let's just populate the value on illegal instructions. This won't > break any guest software, but will provide more information to guests. > > Alistair Francis (3): > target/riscv: Set the opcode in DisasContext > target/riscv: Fixup setting GVA > target/riscv: Implement the stval/mtval illegal instruction Thanks! Applied to riscv-to-apply.next Alistair > > target/riscv/cpu.h | 2 ++ > target/riscv/cpu_helper.c | 24 +++++++++--------------- > target/riscv/translate.c | 5 +++++ > 3 files changed, 16 insertions(+), 15 deletions(-) > > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-07 3:35 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-20 6:49 [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis 2021-12-20 6:49 ` [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext Alistair Francis 2021-12-21 7:08 ` Bin Meng 2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis 2021-12-20 19:38 ` Richard Henderson 2021-12-21 7:30 ` Bin Meng 2022-01-06 4:04 ` Alistair Francis 2022-01-07 2:07 ` Bin Meng 2021-12-20 6:49 ` [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction Alistair Francis 2021-12-20 19:39 ` Richard Henderson 2021-12-21 7:46 ` Bin Meng 2022-01-07 3:33 ` [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).