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

[Andrew Jones <ajones@ventanamicro.com>]

On Mon, Nov 04, 2024 at 08:47:53PM +0800, Yong-Xuan Wang wrote:
> Hi Daniel and Andrew,
> 
> Sorry I found that I forgot a situation. Host kernel doesn't support
> in-kernel AIA is not equal to host machine doesn't support AIA extension.
> 
> If user specifies aia=aplic-imsic when using KVM acceleration, we have 3
> possibilities:
> 1. host doesn't support AIA extension -> report error since we can't handle
>    the stopei CSR.
> 2. host support AIA extension but doesn't have in-kernel AIA -> use usermode
>    IMSIC and handle the stopei CSR in QEMU

And also sireg for the imsic range.

> 3. host support AIA extension and have in-kernel AIA -> use in-kernel AIA
>    and handle the stopei CSR in KVM

Yes, these are the three cases I was expecting, where there's also a 1.5
which is "host supports AIA extension but KVM doesn't support
usermode-imsic". Case 1.5 should also result in reporting an error. I'm
not sure we have 1.5, though, since it looks like the KVM AIA support
already attempts to fallback to a potential usermode-imsic. So maybe
it'll work without any KVM changes.

> 
> We need to update the kvm_riscv_handle_csr() for situation 2. And it's better
> to determine the availability of AIA extension in riscv_imsic_realize().
> 
> Please ignore this patch, I will send another patchset to handle the
> above situations.

We could also take this fix now and then do the usermode-imsic on top
later.

Thanks,
drew

> 
> Regards,
> Yong-Xuan
> 
> On Mon, Nov 4, 2024 at 8:15 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> >
> >
> > On 11/4/24 8:14 AM, 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.
> >
> > Can you please add this info in the commit message? This makes it clearer
> > that there's not much we can do in QEMU aside from erroring out.
> >
> > Also, please add a:
> >
> > Fixes: 48c2c33c52 ("target/riscv: select KVM AIA in riscv virt machine")
> >
> >
> > Thanks,
> >
> > Daniel
> >
> > >
> > > 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