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

[Alistair Francis]

On Sat, Apr 18, 2020 at 11:01 AM Jose Martins <address@hidden> 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 <address@hidden>

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
>