* [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; 20+ 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] 20+ 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; 20+ 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] 20+ 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 ` Alistair Francis
3 siblings, 2 replies; 20+ 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] 20+ 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 ` Alistair Francis
3 siblings, 2 replies; 20+ 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] 20+ 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; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA
@ 2021-12-20 19:38 ` Richard Henderson
0 siblings, 0 replies; 20+ messages in thread
From: Richard Henderson @ 2021-12-20 19:38 UTC (permalink / raw)
To: Alistair Francis, qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng
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] 20+ 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; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction
@ 2021-12-20 19:39 ` Richard Henderson
0 siblings, 0 replies; 20+ messages in thread
From: Richard Henderson @ 2021-12-20 19:39 UTC (permalink / raw)
To: Alistair Francis, qemu-riscv, qemu-devel
Cc: Alistair Francis, Palmer Dabbelt, bmeng.cn, alistair23, Bin Meng
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] 20+ 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; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 1/3] target/riscv: Set the opcode in DisasContext
@ 2021-12-21 7:08 ` Bin Meng
0 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2021-12-21 7:08 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Palmer Dabbelt, Alistair Francis, Bin Meng,
Richard Henderson
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] 20+ 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-21 7:30 ` Bin Meng
2021-12-21 7:30 ` Bin Meng
1 sibling, 0 replies; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA
@ 2021-12-21 7:30 ` Bin Meng
0 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2021-12-21 7:30 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Palmer Dabbelt, Alistair Francis, Bin Meng
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] 20+ 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-21 7:46 ` Bin Meng
2021-12-21 7:46 ` Bin Meng
1 sibling, 0 replies; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 3/3] target/riscv: Implement the stval/mtval illegal instruction
@ 2021-12-21 7:46 ` Bin Meng
0 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2021-12-21 7:46 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Palmer Dabbelt, Alistair Francis, Bin Meng
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] 20+ 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
-1 siblings, 0 replies; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA
@ 2022-01-06 4:04 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2022-01-06 4:04 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
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] 20+ 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
-1 siblings, 0 replies; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 2/3] target/riscv: Fixup setting GVA
@ 2022-01-07 2:07 ` Bin Meng
0 siblings, 0 replies; 20+ messages in thread
From: Bin Meng @ 2022-01-07 2:07 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
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] 20+ 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
@ 2022-01-07 3:33 ` Alistair Francis
2021-12-20 6:49 ` [PATCH v4 2/3] target/riscv: Fixup setting GVA Alistair Francis
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ 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] 20+ messages in thread
* Re: [PATCH v4 0/3] RISC-V: Populate mtval and stval
@ 2022-01-07 3:33 ` Alistair Francis
0 siblings, 0 replies; 20+ messages in thread
From: Alistair Francis @ 2022-01-07 3:33 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Palmer Dabbelt, Bin Meng, 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] 20+ messages in thread
end of thread, other threads:[~2022-01-07 3:35 UTC | newest]
Thread overview: 20+ 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-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-20 19:38 ` Richard Henderson
2021-12-21 7:30 ` Bin Meng
2021-12-21 7:30 ` Bin Meng
2022-01-06 4:04 ` Alistair Francis
2022-01-06 4:04 ` Alistair Francis
2022-01-07 2:07 ` Bin Meng
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-20 19:39 ` Richard Henderson
2021-12-21 7:46 ` Bin Meng
2021-12-21 7:46 ` Bin Meng
2022-01-07 3:33 ` [PATCH v4 0/3] RISC-V: Populate mtval and stval Alistair Francis
2022-01-07 3:33 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.