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

[LIU Zhiwei <zhiwei_liu@c-sky.com>]

[-- Attachment #1: Type: text/plain, Size: 4018 bytes --]


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é

[-- Attachment #2: Type: text/html, Size: 6059 bytes --]