On 12/9/21 10:26 PM, Alistair Francis wrote:
@@ -975,7 +975,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,9 +1003,11 @@ 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;
tval = env->badaddr;
break;
...
@@ -1041,17 +1042,7 @@ 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);
- }
+ env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
/* Trap to VS mode */
@@ -1063,7 +1054,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
cause == IRQ_VS_EXT) {
cause = cause - 1;
}
- env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
} else if (riscv_cpu_virt_enabled(env)) {
/* Trap into HS mode, from virt */
riscv_cpu_swap_hypervisor_regs(env);
@@ -1071,6 +1061,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
env->priv);
env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
riscv_cpu_virt_enabled(env));
+ if (tval) {
+ /*
+ * If we are writing a guest virtual address to stval, set
+ * this to 1.
+ */
+ env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 1);
+ }
The thing that concerns me here is the very legitimate badaddr of NULL. I think you need
to keep some out-of-band flag for that.
In addition, the out-of-band flag should probably be called write_gva, because tval from
ILLEGAL_INST is not a Guest Virtual Address, and so should not set GVA.
You could even push the setting of the bit down so that it's only done once,
e.g.
hstatus_gva = true;
tval = env->badaddr;
if (trap to vs) {
...
hstatus_gva = false;
} else if (virt enabled) {
...
} else {
trap into hs
hstatus_gva = false;
}
env->hstatus(set_field(env->hstatus, HSTATUS_GVA, hstatus_gva);
The actual handling of ILLEGAL_INST looks fine.
You may well want to split the GVA handling to a separate patch.
r~
