On Mon, Nov 04, 2024 at 07:14:42PM +0800, Yong-Xuan Wang wrote:
> Hi Daniel and Andrew,
>
> When handling an external interrupt via IMSIC, we need to use the stopei CSR
> to claim the top interrupt. Even though the QEMU can emulate the IMSIC devices
> without the in-kernel irqchip, we still need to trap and emulate the stopei
> CSR. But since the host machine doesn't support the AIA extension, the guest
> OS
> will hit the illegal instruction exception instead of the virutal instruction
> exception when it accesses the stopei CSR. We can't have a chance to redirect
> this instruction to QEMU. So I think we can directly report errors when the
> user wants to use KVM AIA(MSI) without in-kernel AIA irqchip support.
Thanks for the additional info, Yong-Xuan. I think putting something like
this in the commit message, or even a comment, would be helpful.
Thanks,
drew
>
> Regards,
> Yong-Xuan
>
> On Fri, Nov 1, 2024 at 11:09 PM Andrew Jones <ajo...@ventanamicro.com> wrote:
> >
> > On Fri, Nov 01, 2024 at 08:45:13AM -0300, Daniel Henrique Barboza wrote:
> > >
> > >
> > > On 11/1/24 5:36 AM, Yong-Xuan Wang wrote:
> > > > Currently QEMU will continue to emulate the AIA MSI devices and enable
> > > > the
> > > > AIA extension for guest OS when the host kernel doesn't support the
> > > > in-kernel AIA irqchip. This will cause an illegal instruction exception
> > > > when the guest OS uses the IMSIC devices. Add additional checks to
> > > > ensure
> > > > the guest OS only uses the AIA MSI device when the host kernel supports
> > > > the in-kernel AIA chip.
> > > >
> > > > Signed-off-by: Yong-Xuan Wang <yongxuan.w...@sifive.com>
> > > > Reviewed-by: Jim Shu <jim....@sifive.com>
> > > > ---
> > > > hw/riscv/virt.c | 19 +++++++++++++------
> > > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > > index 45a8c4f8190d..0d8e047844a6 100644
> > > > --- a/hw/riscv/virt.c
> > > > +++ b/hw/riscv/virt.c
> > > > @@ -1567,12 +1567,19 @@ static void virt_machine_init(MachineState
> > > > *machine)
> > > > }
> > > > }
> > > > - if (kvm_enabled() && virt_use_kvm_aia(s)) {
> > > > - kvm_riscv_aia_create(machine, IMSIC_MMIO_GROUP_MIN_SHIFT,
> > > > - VIRT_IRQCHIP_NUM_SOURCES,
> > > > VIRT_IRQCHIP_NUM_MSIS,
> > > > - memmap[VIRT_APLIC_S].base,
> > > > - memmap[VIRT_IMSIC_S].base,
> > > > - s->aia_guests);
> > > > + if (kvm_enabled() && s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
> > > > + if (virt_use_kvm_aia(s)) {
> > > > + kvm_riscv_aia_create(machine, IMSIC_MMIO_GROUP_MIN_SHIFT,
> > > > + VIRT_IRQCHIP_NUM_SOURCES,
> > > > + VIRT_IRQCHIP_NUM_MSIS,
> > > > + memmap[VIRT_APLIC_S].base,
> > > > + memmap[VIRT_IMSIC_S].base,
> > > > + s->aia_guests);
> > > > + } else {
> > > > + error_report("Host machine doesn't support in-kernel APLIC
> > > > MSI, "
> > > > + "please use aia=none or aia=aplic");
> > > > + exit(1);
> > > > + }
> > >
> > > As you said in the commit msg it looks like we have a bug in this
> > > particular path: kvm accel,
> > > aia=aplic-imsic, no irqchip present. Erroring out is one possible
> > > solution but I wonder why we
> > > couldn't just emulate the APLIC and IMSIC controllers in this case. We
> > > have code that does
> > > that in TCG, so it would be a matter of adding the needed plumbing to
> > > treat KVM AIA without
> > > irqchip == TCG AIA.
> > >
> > > Drew, care to weight in? Thanks,
> > >
> >
> > If I understand the motivation for this patch correctly, then we'll always
> > need something like it anyway. With the proposal of supporting KVM with
> > usermode-imsic, then KVM would ultimately have three possible states:
> > inkernel-irqchip, usermode-imsic, nothing. usermode-imsic will need KVM
> > support for forwarding imsic accesses to QEMU, but when that support isn't
> > present (and the inkernel-irqchip isn't selected either), then we should
> > still want to error out before allowing the guest to try accesses that
> > can't work.
> >
> > Thanks,
> > drew
