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

[Andrew Jones]

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