Re: [PATCH v2 04/22] target/riscv: Improve fidelity of guest external interrupts

[Alistair Francis]

On Tue, Sep 14, 2021 at 2:33 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Sep 9, 2021 at 12:14 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Sep 2, 2021 at 9:26 PM Anup Patel <anup.patel@wdc.com> wrote:
> > >
> > > The guest external interrupts for external interrupt controller are
> > > not delivered to the guest running under hypervisor on time. This
> > > results in a guest having sluggish response to serial console input
> > > and other I/O events. To improve timely delivery of guest external
> > > interrupts, we check and inject interrupt upon every sret instruction.
> > >
> > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > ---
> > >  target/riscv/op_helper.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > > index ee7c24efe7..4c995c239e 100644
> > > --- a/target/riscv/op_helper.c
> > > +++ b/target/riscv/op_helper.c
> > > @@ -129,6 +129,15 @@ target_ulong helper_sret(CPURISCVState *env, 
> > > target_ulong cpu_pc_deb)
> > >
> > >      riscv_cpu_set_mode(env, prev_priv);
> > >
> > > +    /*
> > > +     * QEMU does not promptly deliver guest external interrupts
> > > +     * to a guest running on a hypervisor which in-turn is running
> > > +     * on QEMU. We make dummy call to riscv_cpu_update_mip() upon
> > > +     * every sret instruction so that QEMU pickup guest external
> > > +     * interrupts sooner.
> > > +     */
> > > +     riscv_cpu_update_mip(env_archcpu(env), 0, 0);
> >
> > This doesn't seem right. I don't understand why we need this?
> >
> > riscv_cpu_update_mip() is called when an interrupt is delivered to the
> > CPU, if we are missing interrupts then that is a bug somewhere else.
>
> I have finally figured out the cause of guest external interrupts being
> missed by Guest/VM.
>
> The riscv_cpu_set_irq() which handles guest external interrupt lines
> of each CPU is called asynchronously. This function in-turn calls
> riscv_cpu_update_mip() but the CPU might be in host mode (V=0)
> or in Guest/VM mode (V=1). If the CPU is in host mode (V=0) when

The IRQ being raised should just directly call riscv_cpu_update_mip()
so the IRQ should happen straight away.

Even from MTTCG I see this:

"""
Currently thanks to KVM work any access to IO memory is automatically
protected by the global iothread mutex, also known as the BQL (Big
Qemu Lock). Any IO region that doesn't use global mutex is expected to
do its own locking.

However IO memory isn't the only way emulated hardware state can be
modified. Some architectures have model specific registers that
trigger hardware emulation features. Generally any translation helper
that needs to update more than a single vCPUs of state should take the
BQL.
"""

So we should be fine here as well.

Can you supply a test case to reproduce the bug?

> the riscv_cpu_set_irq() is called, then the CPU interrupt requested by
> riscv_cpu_update_mip() has no effect because the CPU can't take
> the interrupt until it enters Guest/VM mode.
>
> This patch does the right thing by doing a dummy call to
> riscv_cpu_update_mip() upon SRET instruction so that if the CPU
> had missed a guest interrupt previously then the CPU can take it now.

This still doesn't look like the right fix.

Alistair

>
> Regards,
> Anup