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

[Alistair Francis <alistair23@gmail.com>]

 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