Re: [PATCH 1/1] hw/riscv: virt: prevent to use AIA MSI when host doesn't support it

[Andrew Jones]

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 <ajones@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.wang@sifive.com>
> > > > Reviewed-by: Jim Shu <jim.shu@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