| Message ID | 20241101083606.5122-1-yongxuan.wang@sifive.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] hw/riscv: virt: prevent to use AIA MSI when host doesn't support it | expand |
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, Daniel > } > > if (riscv_is_32bit(&s->soc[0])) {
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
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. 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
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
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
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 3. host support AIA extension and have in-kernel AIA -> use in-kernel AIA and handle the stopei CSR in KVM 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. 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
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
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); + } } if (riscv_is_32bit(&s->soc[0])) {