| Message ID | 20210902112520.475901-5-anup.patel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU RISC-V AIA support | expand |
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. Alistair
On Thu, Sep 9, 2021 at 2:45 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. Agree. This change looks like a hack to me. Anup, with what configuration did you see the issue you were trying to resolve? Regards, Bin
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 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. Regards, Anup
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
On Wed, Sep 15, 2021 at 6:19 AM Alistair Francis <alistair23@gmail.com> wrote: > > 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. That's not true for guest external interrupts. The target Guest/VM might not be running on the receiving HART. Let say IMSIC injected guest external IRQ1 to HARTx which is meant for a Guest/VM, so the riscv_cpu_set_irq() will call riscv_cpu_update_mip(). If HARTx might be in HS-mode or HARTx might be running some other Guest/VM then cpu_interrupt() request queued by riscv_cpu_update_mip() will not result in any interrupt being injected. This further means that QEMU has to check and inject guest external interrupts to target Guest/VM when HARTx makes a switch from HS-mode to VS-mode. By calling riscv_cpu_update_mip() upon SRET instruction we are ensuring that if any guest external interrupt was missed then it is injected ot VS-mode. > > 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? Just boot Linux Guest using my QEMU, OpenSBI, Linux, and KVMTOOL having AIA support patches. If this patch is not there then lot of Guest external interrupts are missed and Guest gets stuck at random places. Regards, Anup > > > 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
On Thu, Sep 16, 2021 at 11:42 PM Anup Patel <anup@brainfault.org> wrote: > > On Wed, Sep 15, 2021 at 6:19 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > 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. > > That's not true for guest external interrupts. The target Guest/VM might > not be running on the receiving HART. > > Let say IMSIC injected guest external IRQ1 to HARTx which is meant > for a Guest/VM, so the riscv_cpu_set_irq() will call riscv_cpu_update_mip(). > If HARTx might be in HS-mode or HARTx might be running some > other Guest/VM then cpu_interrupt() request queued by riscv_cpu_update_mip() > will not result in any interrupt being injected. This further means that > QEMU has to check and inject guest external interrupts to target > Guest/VM when HARTx makes a switch from HS-mode to VS-mode. By > calling riscv_cpu_update_mip() upon SRET instruction we are ensuring > that if any guest external interrupt was missed then it is injected ot > VS-mode. Ah ok. So the problem is that an interrupt can occur for a guest, while that guest isn't executing. So for example a CPU is executing with V=0. `riscv_cpu_update_mip()` is called, which triggers a hard interrupt. That in turn calls `riscv_cpu_exec_interrupt()` and `riscv_cpu_local_irq_pending()`. This results in the guest Hypervisor receiving the interrupt, which it then doesn't act on? Or is MIP set but `riscv_cpu_local_irq_pending()` returns false due to the enable checks? That either seems like a guest bug or that we need some functionality in `riscv_cpu_swap_hypervisor_regs()` to trigger an interrupt on context swap. Alistair > > > > > 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? > > Just boot Linux Guest using my QEMU, OpenSBI, Linux, and KVMTOOL > having AIA support patches. If this patch is not there then lot of Guest > external interrupts are missed and Guest gets stuck at random places. > > Regards, > Anup > > > > > > 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
On Fri, Oct 15, 2021 at 11:54 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Thu, Sep 16, 2021 at 11:42 PM Anup Patel <anup@brainfault.org> wrote: > > > > On Wed, Sep 15, 2021 at 6:19 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > 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. > > > > That's not true for guest external interrupts. The target Guest/VM might > > not be running on the receiving HART. > > > > Let say IMSIC injected guest external IRQ1 to HARTx which is meant > > for a Guest/VM, so the riscv_cpu_set_irq() will call riscv_cpu_update_mip(). > > If HARTx might be in HS-mode or HARTx might be running some > > other Guest/VM then cpu_interrupt() request queued by riscv_cpu_update_mip() > > will not result in any interrupt being injected. This further means that > > QEMU has to check and inject guest external interrupts to target > > Guest/VM when HARTx makes a switch from HS-mode to VS-mode. By > > calling riscv_cpu_update_mip() upon SRET instruction we are ensuring > > that if any guest external interrupt was missed then it is injected ot > > VS-mode. > > Ah ok. > > So the problem is that an interrupt can occur for a guest, while that > guest isn't executing. Yes, that's right. > > So for example a CPU is executing with V=0. `riscv_cpu_update_mip()` > is called, which triggers a hard interrupt. That in turn calls > `riscv_cpu_exec_interrupt()` and `riscv_cpu_local_irq_pending()`. In this case, the hard interrupt is actually a guest external interrupt which is tracked via hgeip CSR. The hgeip CSR is updated immediately but `riscv_cpu_local_irq_pending()` might be called while V=0 hence no interrupt. > > This results in the guest Hypervisor receiving the interrupt, which it > then doesn't act on? Or is MIP set but `riscv_cpu_local_irq_pending()` > returns false due to the enable checks? Here, hgeip CSR will be set and it will be reflected in mip.VSEIP bit only when hypervisor schedules/runs the Guest (i.e. V=1 and hstatus.VGEIN pointing to the appropriate bit of hgeip csr). > > That either seems like a guest bug or that we need some functionality > in `riscv_cpu_swap_hypervisor_regs()` to trigger an interrupt on > context swap. This certainly is not a bug with Guest or Hypervisor but rather an issue of invoking `riscv_cpu_exec_interrupt()` and `riscv_cpu_local_irq_pending()` at the right time. Your suggestion of checking and triggering guest external interrupt in `riscv_cpu_swap_hypervisor_regs()` is a better approach. If you are okay then I will update this patch in the next revision. Regards, Anup > > Alistair > > > > > > > > > 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? > > > > Just boot Linux Guest using my QEMU, OpenSBI, Linux, and KVMTOOL > > having AIA support patches. If this patch is not there then lot of Guest > > external interrupts are missed and Guest gets stuck at random places. > > > > Regards, > > Anup > > > > > > > > > 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
On Mon, Oct 18, 2021 at 10:55 PM Anup Patel <anup@brainfault.org> wrote: > > On Fri, Oct 15, 2021 at 11:54 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Thu, Sep 16, 2021 at 11:42 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Wed, Sep 15, 2021 at 6:19 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > > > 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. > > > > > > That's not true for guest external interrupts. The target Guest/VM might > > > not be running on the receiving HART. > > > > > > Let say IMSIC injected guest external IRQ1 to HARTx which is meant > > > for a Guest/VM, so the riscv_cpu_set_irq() will call riscv_cpu_update_mip(). > > > If HARTx might be in HS-mode or HARTx might be running some > > > other Guest/VM then cpu_interrupt() request queued by riscv_cpu_update_mip() > > > will not result in any interrupt being injected. This further means that > > > QEMU has to check and inject guest external interrupts to target > > > Guest/VM when HARTx makes a switch from HS-mode to VS-mode. By > > > calling riscv_cpu_update_mip() upon SRET instruction we are ensuring > > > that if any guest external interrupt was missed then it is injected ot > > > VS-mode. > > > > Ah ok. > > > > So the problem is that an interrupt can occur for a guest, while that > > guest isn't executing. > > Yes, that's right. > > > > > So for example a CPU is executing with V=0. `riscv_cpu_update_mip()` > > is called, which triggers a hard interrupt. That in turn calls > > `riscv_cpu_exec_interrupt()` and `riscv_cpu_local_irq_pending()`. > > In this case, the hard interrupt is actually a guest external interrupt > which is tracked via hgeip CSR. The hgeip CSR is updated immediately > but `riscv_cpu_local_irq_pending()` might be called while V=0 hence > no interrupt. > > > > > This results in the guest Hypervisor receiving the interrupt, which it > > then doesn't act on? Or is MIP set but `riscv_cpu_local_irq_pending()` > > returns false due to the enable checks? > > Here, hgeip CSR will be set and it will be reflected in mip.VSEIP > bit only when hypervisor schedules/runs the Guest (i.e. V=1 and > hstatus.VGEIN pointing to the appropriate bit of hgeip csr). > > > > > That either seems like a guest bug or that we need some functionality > > in `riscv_cpu_swap_hypervisor_regs()` to trigger an interrupt on > > context swap. > > This certainly is not a bug with Guest or Hypervisor but rather an > issue of invoking `riscv_cpu_exec_interrupt()` and > `riscv_cpu_local_irq_pending()` at the right time. > > Your suggestion of checking and triggering guest external interrupt > in `riscv_cpu_swap_hypervisor_regs()` is a better approach. If you > are okay then I will update this patch in the next revision. Yeah, let's do that instead. Alistair
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); + return retpc; }
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(+)