Re: [PATCH 1/1] target/riscv: Fix VS mode interrupts forwarding.

From: Jose Martins <josemartins90@gmail.com>
To: Rajnesh Kanwal <rajnesh.kanwal49@gmail.com>
Cc: alistair.francis@wdc.com, palmerdabbelt@google.com,
	qemu-riscv@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH 1/1] target/riscv: Fix VS mode interrupts forwarding.
Date: Sun, 23 Feb 2020 15:39:55 +0000	[thread overview]
Message-ID: <CAC41xo0V_F4ExJjoYx4AWQXViX-WwgfFx219EyCb8mk-okKUAQ@mail.gmail.com> (raw)
In-Reply-To: <CAHfcQ+GGCXj6f+PMMvqNhg=Lzpi1Ke+OaEO6-pb5gjSngm3Upg@mail.gmail.com>

No problem. But I'm failing to see what you mean. My reasoning was:
the specification mandates that only VS mode interrupt bits are
writable in hideleg, all the others must be hardwired to zero. This
means the hypervisor can't really delegate S mode interrupts as you
are saying. So, if this is implemented correctly, you will never get
inside that if condition because of an HS interrupt. And all
delegatable asynchronous exception values must be decremented. So,
checking if this is an async exception should do the job.

Jose

On Sun, 23 Feb 2020 at 15:10, Rajnesh Kanwal <rajnesh.kanwal49@gmail.com> wrote:
>
> Hello Jose,
>
> Sorry I didn't see that as it hadn't became a part of the port. I don't know how
> they proceed with same patches.
>
> Just to add, there is a minor problem with your patch. The cause value should
> only be decremented by one for VS mode interrupts. In case if hypervisor has
> delegated S mode interrupts then we should not decrement cause for those
> interrupts.
>
> Regards,
> Rajnesh
>
>
> On Sun, Feb 23, 2020 at 7:41 PM Jose Martins <josemartins90@gmail.com> wrote:
>>
>> Hello rajnesh,
>>
>> I had already submitted almost this exact patch a few weeks ago.
>>
>> Jose
>>
>> On Sun, 23 Feb 2020 at 13:51, <rajnesh.kanwal49@gmail.com> wrote:
>> >
>> > From: Rajnesh Kanwal <rajnesh.kanwal49@gmail.com>
>> >
>> > Currently riscv_cpu_local_irq_pending is used to find out pending
>> > interrupt and VS mode interrupts are being shifted to represent
>> > S mode interrupts in this function. So when the cause returned by
>> > this function is passed to riscv_cpu_do_interrupt to actually
>> > forward the interrupt, the VS mode forwarding check does not work
>> > as intended and interrupt is actually forwarded to hypervisor. This
>> > patch fixes this issue.
>> >
>> > Signed-off-by: Rajnesh Kanwal <rajnesh.kanwal49@gmail.com>
>> > ---
>> >  target/riscv/cpu_helper.c | 9 ++++++++-
>> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > index b9e90dfd9a..59535ecba6 100644
>> > --- a/target/riscv/cpu_helper.c
>> > +++ b/target/riscv/cpu_helper.c
>> > @@ -46,7 +46,7 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
>> >      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)) >> 1;
>> > +                              (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
>> >
>> >      target_ulong mie    = env->priv < PRV_M ||
>> >                            (env->priv == PRV_M && mstatus_mie);
>> > @@ -900,6 +900,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>> >
>> >              if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) &&
>> >                  !force_hs_execp) {
>> > +                /*
>> > +                 * See if we need to adjust cause. Yes if its VS mode interrupt
>> > +                 * no if hypervisor has delegated one of hs mode's interrupt
>> > +                 */
>> > +                if (cause == IRQ_VS_TIMER || cause == IRQ_VS_SOFT ||
>> > +                    cause == IRQ_VS_EXT)
>> > +                    cause = cause - 1;
>> >                  /* Trap to VS mode */
>> >              } else if (riscv_cpu_virt_enabled(env)) {
>> >                  /* Trap into HS mode, from virt */
>> > --
>> > 2.17.1
>> >
>> >