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

[LIU Zhiwei]

On 5/26/21 7:50 PM, Jose Martins wrote:

> Hello Zhiwei, thank you for reviewing the patch.
>
> I'll split the patch in a series as you suggest. But first can you
> help me understand what the problems are with
> riscv_cpu_local_irq_pending?
>
> > I think there are two errors in riscv_cpu_local_irq_pending.
> >
> > 1) VS interrupts can't be forwarded to hs-mode rightly . It has
> > nothing to do with delegate or not in hideleg. The reason is that
> > VS interrupts are always discarded when V=0 in
> > riscv_cpu_local_irq_pending.
> >       
> I don't see why this is the case. The way I see it, VS interrupts are
> only discarded for V=0 *iff* they are delegated in mideleg/hideleg.
>     
    First I paste the code to ensure we are talking about the same
    version.
 static int riscv_cpu_local_irq_pending(CPURISCVState *env)
 {
     target_ulong irqs;
 
     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 vspending = (env->mip & env->mie &
                               (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
 
     target_ulong mie    = env->priv < PRV_M ||
                           (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;
 
         if (pending_hs_irq) {
             riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
             return ctz64(pending_hs_irq);
         }
 
         pending = vspending;
     }
 
     irqs = (pending & ~env->mideleg & -mie) | (pending &  env->mideleg & -sie);
 
     if (irqs) {
         return ctz64(irqs); /* since non-zero */
     } else {
         return RISCV_EXCP_NONE; /* indicates no pending interrupt */
     }
 }
    Only when VS = 0, the variable vspending can transfer to
    variable pending. Any interrupt not included in variable pending
    is discarded. 
    
    That's why I say VS interrupts are always discarded
    when V=0 in riscv_cpu_local_irq_pending.
> I
> actually tested it and I see the correct forwarding of vs-mode
> interrupts to hs-mode. I tested it by running in hs-mode with all the
> needed interrupt enables set, the interrupts not delegated in hideleg,
> and forcing the trigger of the interrupt by writing hvip. But maybe
> there are some corner cases I'm not taking into account. Can you
> explain this further? Maybe walk me through an example of when this
> issue might occur.
>
> > 2) Use MSTATUS_SIE in mstatus_hs to select pending_hs_irqs.
> >       
>

I mean the second error is to misuse MSATUS_SIE in mstatus_hs to
select pending_hs_irqs.

> I don't think you need to go through mstatus_hs to get the correct sie
> state.
>     
    Agree.
> My logic behind this is: env->mstatus will have the vs-level
> sie if V=1 and hs-level sie if V=0. Due to the short-circuiting
> property of the logic operators the sie variable will only have an
> effect on hsie if V=0 and on vsie if V=1. So the value of sie is only
> used in the correct context.
>

The  swap regs funciton has done the right thing.

I think V mode and hideleg/mideleg  make it possible to process

VS interrupt or HS interrupt like other interrupts.

Zhiwei

> Again, please correct me if I'm wrong. I might be missing something.
>
> Best,
> José
>