The wfi exception trigger behavior was not taking into account the fact
that user mode is not allowed to execute wfi instructions or the effect
of the hstatus.vtw bit. It was also always generating virtual instruction
exceptions when this should only happen when the wfi instruction is
executed when the virtualization mode is enabled:

- when a wfi instruction is executed, an illegal exception should be triggered
if either the current mode is user or the mode is supervisor and mstatus.tw is
set.

- a virtual instruction exception should be raised when a wfi is executed from
virtual-user or virtual-supervisor and hstatus.vtw is set.

Signed-off-by: Jose Martins <josemartins90@gmail.com>
---
 target/riscv/cpu_bits.h  | 1 +
 target/riscv/op_helper.c | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 24b24c69c5..ed8b97c788 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -436,6 +436,7 @@
 #define HSTATUS_HU           0x00000200
 #define HSTATUS_VGEIN        0x0003F000
 #define HSTATUS_VTVM         0x00100000
+#define HSTATUS_VTW          0x00200000
 #define HSTATUS_VTSR         0x00400000
 #if defined(TARGET_RISCV64)
 #define HSTATUS_VSXL        0x300000000
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index d55def76cf..354e39ec26 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -174,9 +174,11 @@ void helper_wfi(CPURISCVState *env)
 {
     CPUState *cs = env_cpu(env);
 
-    if ((env->priv == PRV_S &&
-        get_field(env->mstatus, MSTATUS_TW)) ||
-        riscv_cpu_virt_enabled(env)) {
+    if ((!riscv_cpu_virt_enabled(env) && env->priv == PRV_U) ||
+        (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TW))) {
+        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+    } else if (riscv_cpu_virt_enabled(env) && (env->priv == PRV_U ||
+            (env->priv == PRV_S && get_field(env->hstatus, HSTATUS_VTW)))) {
         riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
     } else {
         cs->halted = 1;
-- 
2.25.1

On Sun, Apr 11, 2021 at 5:41 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> The wfi exception trigger behavior was not taking into account the fact
> that user mode is not allowed to execute wfi instructions or the effect
> of the hstatus.vtw bit. It was also always generating virtual instruction
> exceptions when this should only happen when the wfi instruction is
> executed when the virtualization mode is enabled:
>
> - when a wfi instruction is executed, an illegal exception should be triggered
> if either the current mode is user or the mode is supervisor and mstatus.tw is
> set.
>
> - a virtual instruction exception should be raised when a wfi is executed from
> virtual-user or virtual-supervisor and hstatus.vtw is set.
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
>  target/riscv/cpu_bits.h  | 1 +
>  target/riscv/op_helper.c | 8 +++++---
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 24b24c69c5..ed8b97c788 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -436,6 +436,7 @@
>  #define HSTATUS_HU           0x00000200
>  #define HSTATUS_VGEIN        0x0003F000
>  #define HSTATUS_VTVM         0x00100000
> +#define HSTATUS_VTW          0x00200000
>  #define HSTATUS_VTSR         0x00400000
>  #if defined(TARGET_RISCV64)
>  #define HSTATUS_VSXL        0x300000000
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index d55def76cf..354e39ec26 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -174,9 +174,11 @@ void helper_wfi(CPURISCVState *env)
>  {
>      CPUState *cs = env_cpu(env);
>
> -    if ((env->priv == PRV_S &&
> -        get_field(env->mstatus, MSTATUS_TW)) ||
> -        riscv_cpu_virt_enabled(env)) {
> +    if ((!riscv_cpu_virt_enabled(env) && env->priv == PRV_U) ||

> +        (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TW))) {

Shouldn't we check here that we aren't virtualised?

Alistair

> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +    } else if (riscv_cpu_virt_enabled(env) && (env->priv == PRV_U ||
> +            (env->priv == PRV_S && get_field(env->hstatus, HSTATUS_VTW)))) {
>          riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
>      } else {
>          cs->halted = 1;
> --
> 2.25.1
>
>

> > -    if ((env->priv == PRV_S &&
> > -        get_field(env->mstatus, MSTATUS_TW)) ||
> > -        riscv_cpu_virt_enabled(env)) {
> > +    if ((!riscv_cpu_virt_enabled(env) && env->priv == PRV_U) ||
>
> > +        (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TW))) {
>
> Shouldn't we check here that we aren't virtualised?
>

In section 5.4, the spec states that mstatus.tw has effect regardless
of virtualization mode: "The TW field affects execution in all modes
except M-mode.". I interpret "all modes" as being all supervisor modes
since section 3.1.6.5 states that "When S-mode is implemented, then
executing WFI in U-mode causes an illegal instruction exception" and
later chapter 5 says that a virtual instruction exception should be
generated when "in VU-mode, attempts to execute WFI (...)" regardless
of the state of any status bit. Plus, it should be an illegal
instruction exception and not a virtual instruction exception even in
VS-mode when mstatus.tw = 1 because the spec also states only "When
VTW=1 *(and assuming mstatus.TW=0)*, an attempt in VS-mode to execute
WFI raises a virtual instruction exception".

But just now I realized the patch is assuming S-mode is present and
not taking into account M/U only harts. If this is the case TW should
affect U-mode WFIs. I will fix this and submit a new version of the
patch.

José