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
> <dbarb...@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 <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
