[1/1] target/riscv: fix VS interrupts forwarding to HS

When vs interrupts (2, 6, 10) are enabled, pending and not delegated
in hideleg, they are not always forwarded to hs mode after a return to
vs mode. This happens independently of the state of spie and sie on
the hs-level sstatus before the return. Instead, the vs-level status
sie state seems to be controlling if the interrupt is forward to hs or
not. This is both because, in riscv_cpu_local_irq_pending, vs
interrupts are ignored when checking for hs pending interrupts and
also because hs interrupts might not be considered enabled after
jumping to vs mode if the spie (which implicitly is copied to sie) is
not set when sret is executed. From what I could gather, the spec does
not preclude hs mode from receiving vs interrupts if they are not
delegated in hideleg (although this is true for m mode, but guaranteed
by hardwiring the corresponding bits in mideleg). Also, it clearly
states that: "Interrupts for higher-privilege modes, y>x, are always
globally enabled regardless of the setting of the global yIE bit for
the higher-privilege mode.", so hs_mstatus_sie must be set whenever
virt is enabled. After solving the previous issue, the problem remains
that when such interrupts are delegated in hideleg, there is still the
need to check it when calculating pending_hs_irq, otherwise, we're
still "forcing an hs except" when the interrupt should be forwarded to
vs. I believe the following patch will fix this issue:

Signed-off-by: Jose Martins <josemartins90@gmail.com>
---
 target/riscv/cpu_helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

On Sat, Apr 18, 2020 at 11:01 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> When vs interrupts (2, 6, 10) are enabled, pending and not delegated
> in hideleg, they are not always forwarded to hs mode after a return to
> vs mode. This happens independently of the state of spie and sie on
> the hs-level sstatus before the return. Instead, the vs-level status
> sie state seems to be controlling if the interrupt is forward to hs or
> not. This is both because, in riscv_cpu_local_irq_pending, vs
> interrupts are ignored when checking for hs pending interrupts and
> also because hs interrupts might not be considered enabled after
> jumping to vs mode if the spie (which implicitly is copied to sie) is
> not set when sret is executed. From what I could gather, the spec does
> not preclude hs mode from receiving vs interrupts if they are not
> delegated in hideleg (although this is true for m mode, but guaranteed
> by hardwiring the corresponding bits in mideleg). Also, it clearly
> states that: "Interrupts for higher-privilege modes, y>x, are always
> globally enabled regardless of the setting of the global yIE bit for
> the higher-privilege mode.", so hs_mstatus_sie must be set whenever
> virt is enabled. After solving the previous issue, the problem remains
> that when such interrupts are delegated in hideleg, there is still the
> need to check it when calculating pending_hs_irq, otherwise, we're
> still "forcing an hs except" when the interrupt should be forwarded to
> vs. I believe the following patch will fix this issue:
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>

Thanks for the patch!

I'm a little confused, do you mind explaining some things to me below.

> ---
>  target/riscv/cpu_helper.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d3ba9efb02..9962ee4690 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -43,8 +43,7 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>      target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
>      target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);
>
> -    target_ulong pending = env->mip & env->mie &
> -                               ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
> +    target_ulong pending = env->mip & env->mie;

If the Hypervisor sets the V* interrupts why does it then want to
receive the interrupt itself?

>      target_ulong vspending = (env->mip & env->mie &
>                                (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
>
> @@ -52,11 +51,11 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>                            (env->priv == PRV_M && mstatus_mie);
>      target_ulong sie    = env->priv < PRV_S ||
>                            (env->priv == PRV_S && mstatus_sie);
> -    target_ulong hs_sie = env->priv < PRV_S ||
> +    target_ulong hs_sie = riscv_cpu_virt_enabled(env) || env->priv < PRV_S ||
>                            (env->priv == PRV_S && hs_mstatus_sie);

Isn't hs_sie only ever accessed if riscv_cpu_virt_enabled(env)?
Doesn't this just set hs_sie to always be 1?

>
>      if (riscv_cpu_virt_enabled(env)) {
> -        target_ulong pending_hs_irq = pending & -hs_sie;
> +        target_ulong pending_hs_irq = pending & ~env->hideleg & -hs_sie;

This change looks good.

Alistair

>
>          if (pending_hs_irq) {
>              riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
> --
> 2.17.1
>

> If the Hypervisor sets the V* interrupts why does it then want to
> receive the interrupt itself?

I don't think this is a question of whether there is a use case for it
or not (I agree with you, of the top of my head I don't see why would
you forward v* interrupts to the hypervisor). However,  from what I
can understand,  the spec allows for it. If you don't set the
corresponding bits in hideleg, v* interrupts should be forwarded to HS
(as I said, they are guaranteed not to be forwarded to m mode because
these bits must be hardwired in mideleg). Otherwise, there would be no
purpose for the hideleg register, as v* interrupts bits are the only
ones that can be written in it (am I missing something?).

> Isn't hs_sie only ever accessed if riscv_cpu_virt_enabled(env)?
> Doesn't this just set hs_sie to always be 1?

I don't understand if you don't agree that hs_sie should be always set
when riscv_cpu_virt_enabled(env), or if you agree with it and don't
see the need for the hs_sie variable at all. If it is the latter, I
agree with you. So the patch would become:

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

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d3ba9efb02..a85eadb4fb 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -41,10 +41,8 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)

     target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
     target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
-    target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);

-    target_ulong pending = env->mip & env->mie &
-                               ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
+    target_ulong pending = env->mip & env->mie;
     target_ulong vspending = (env->mip & env->mie &
                               (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));

@@ -52,11 +50,9 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
                           (env->priv == PRV_M && mstatus_mie);
     target_ulong sie    = env->priv < PRV_S ||
                           (env->priv == PRV_S && mstatus_sie);
-    target_ulong hs_sie = env->priv < PRV_S ||
-                          (env->priv == PRV_S && hs_mstatus_sie);

     if (riscv_cpu_virt_enabled(env)) {
-        target_ulong pending_hs_irq = pending & -hs_sie;
+        target_ulong pending_hs_irq = pending & ~env->hideleg;

         if (pending_hs_irq) {
             riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);

On Wed, Apr 29, 2020 at 9:07 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> > If the Hypervisor sets the V* interrupts why does it then want to
> > receive the interrupt itself?
>
> I don't think this is a question of whether there is a use case for it
> or not (I agree with you, of the top of my head I don't see why would
> you forward v* interrupts to the hypervisor). However,  from what I
> can understand,  the spec allows for it. If you don't set the
> corresponding bits in hideleg, v* interrupts should be forwarded to HS
> (as I said, they are guaranteed not to be forwarded to m mode because
> these bits must be hardwired in mideleg). Otherwise, there would be no
> purpose for the hideleg register, as v* interrupts bits are the only
> ones that can be written in it (am I missing something?).

I think you are right.

"Among bits 15:0 of hideleg, only bits 10, 6, and 2 (corresponding to
the standard VS-level interrupts) shall be writable, and the others
shall be hardwired to zero."

Which means that it only controls the V* interrupts.

>
> > Isn't hs_sie only ever accessed if riscv_cpu_virt_enabled(env)?
> > Doesn't this just set hs_sie to always be 1?
>
> I don't understand if you don't agree that hs_sie should be always set
> when riscv_cpu_virt_enabled(env), or if you agree with it and don't
> see the need for the hs_sie variable at all. If it is the latter, I
> agree with you. So the patch would become:

Currently hs_sie is set:
 - When we are in U-Mode
 - If we are in S-Mode and hs_mstatus_sie is true

Then hs_sie is only accessed if virtulisation is enabled.

Your change just made it true for whenever virtulisation is enabled
(in which case we don't need it).

>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
>  target/riscv/cpu_helper.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d3ba9efb02..a85eadb4fb 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -41,10 +41,8 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>
>      target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
>      target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> -    target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);
>
> -    target_ulong pending = env->mip & env->mie &
> -                               ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
> +    target_ulong pending = env->mip & env->mie;

This looks good

>      target_ulong vspending = (env->mip & env->mie &
>                                (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
>
> @@ -52,11 +50,9 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>                            (env->priv == PRV_M && mstatus_mie);
>      target_ulong sie    = env->priv < PRV_S ||
>                            (env->priv == PRV_S && mstatus_sie);
> -    target_ulong hs_sie = env->priv < PRV_S ||
> -                          (env->priv == PRV_S && hs_mstatus_sie);
>
>      if (riscv_cpu_virt_enabled(env)) {
> -        target_ulong pending_hs_irq = pending & -hs_sie;
> +        target_ulong pending_hs_irq = pending & ~env->hideleg;

I don't see why we don't need to check the HS version of MSTATUS_SIE.
I think hs_sie should stay shouldn't it?

Alistair

>
>          if (pending_hs_irq) {
>              riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
> --
> 2.17.1
>
> Jose

> Your change just made it true for whenever virtulisation is enabled
> (in which case we don't need it).

This is exactly my point. As I said in the commit message, the spec
clearly tells us that "Interrupts for higher-privilege modes, y>x, are
always globally enabled regardless of the setting of the global yIE
bit for the higher-privilege mode.". HS is clearly a higher-privilege
mode than either VS or VU. So, if virtualization is enabled, HS level
interrupts must be considered enabled independently of the state of
the actual sie bit in mstatus_hs.

On Wed, Apr 29, 2020 at 2:08 PM Jose Martins <josemartins90@gmail.com> wrote:
>
> > Your change just made it true for whenever virtulisation is enabled
> > (in which case we don't need it).
>
> This is exactly my point. As I said in the commit message, the spec
> clearly tells us that "Interrupts for higher-privilege modes, y>x, are
> always globally enabled regardless of the setting of the global yIE
> bit for the higher-privilege mode.". HS is clearly a higher-privilege
> mode than either VS or VU. So, if virtualization is enabled, HS level

I'm not sure HS is a higher privilege mode.

HS is privilege encoding 1, which is the same as VS (VU is obviously lower).

Alistair

> interrupts must be considered enabled independently of the state of
> the actual sie bit in mstatus_hs.

> I'm not sure HS is a higher privilege mode.
>
> HS is privilege encoding 1, which is the same as VS (VU is obviously lower).

I just checked the spec and it doesn't actually, explicitly state that
HS is a higher-privilege mode than VS. I thought this was something
implicit, but you might be right. I'll try to reach out to the spec
authors to clarify this.

Jose

Reached out to Andrew Waterman. This was his response:

"I think the encoding of the privileged modes is a red herring.  HS is
inherently more privileged than VS, since it controls memory
protection and interrupt delegation for VS.
Certainly the intent is that HS-mode interrupts are always enabled
while executing in VS-mode.  Otherwise, badly behaved VS-mode software
could starve HS-mode of interrupts."

So my assumption was correct.

Jose

On Thu, 30 Apr 2020 at 22:47, Jose Martins <josemartins90@gmail.com> wrote:
>
> > I'm not sure HS is a higher privilege mode.
> >
> > HS is privilege encoding 1, which is the same as VS (VU is obviously lower).
>
> I just checked the spec and it doesn't actually, explicitly state that
> HS is a higher-privilege mode than VS. I thought this was something
> implicit, but you might be right. I'll try to reach out to the spec
> authors to clarify this.
>
> Jose

On Fri, May 1, 2020 at 11:57 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> Reached out to Andrew Waterman. This was his response:
>
> "I think the encoding of the privileged modes is a red herring.  HS is
> inherently more privileged than VS, since it controls memory
> protection and interrupt delegation for VS.
> Certainly the intent is that HS-mode interrupts are always enabled
> while executing in VS-mode.  Otherwise, badly behaved VS-mode software
> could starve HS-mode of interrupts."

Ok, so in which case the hs_sie variable should be removed.

Alistair

>
> So my assumption was correct.
>
> Jose
>
> On Thu, 30 Apr 2020 at 22:47, Jose Martins <josemartins90@gmail.com> wrote:
> >
> > > I'm not sure HS is a higher privilege mode.
> > >
> > > HS is privilege encoding 1, which is the same as VS (VU is obviously lower).
> >
> > I just checked the spec and it doesn't actually, explicitly state that
> > HS is a higher-privilege mode than VS. I thought this was something
> > implicit, but you might be right. I'll try to reach out to the spec
> > authors to clarify this.
> >
> > Jose