| Message ID | 20250217081927.10613-2-yongxuan.wang@sifive.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: AIA: refinement for KVM acceleration | expand |
On 2/17/25 5:19 AM, Yong-Xuan Wang wrote: > KVM AIA is only needed to be set when the virt machine use the AIA MSI. > So we can move the KVM AIA configuration into virt_create_aia() to reduce > the condition checking. > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> > --- Unfortunately this doesn't work. The reason is that kvm_riscv_aia_create(), as it is now, is called only once during virt_machine_init() and it's already handling initialization for each socket: for (socket = 0; socket < socket_count; socket++) { socket_imsic_base = imsic_base + socket * (1U << group_shift); hart_count = riscv_socket_hart_count(machine, socket); base_hart = riscv_socket_first_hartid(machine, socket); if (max_hart_per_socket < hart_count) { max_hart_per_socket = hart_count; } for (i = 0; i < hart_count; i++) { imsic_addr = socket_imsic_base + i * IMSIC_HART_SIZE(guest_bits); ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_ADDR, KVM_DEV_RISCV_AIA_ADDR_IMSIC(i + base_hart), &imsic_addr, true, NULL); if (ret < 0) { error_report("KVM AIA: failed to set the IMSIC address for hart %d", i); exit(1); } } } After this change, kvm_riscv_aia_create() is being called once for each socket since it's now being called inside virt_create_aia(). And this will cause errors when running qemu-kvm with more than one socket: ./qemu-system-riscv64 \ -machine virt,accel=kvm,aia=aplic-imsic -m 2G \ -object memory-backend-ram,size=1G,id=m0 \ -object memory-backend-ram,size=1G,id=m1 \ -smp 2,sockets=2,cores=1,threads=1 \ -numa node,memdev=m0,cpus=0,nodeid=0 \ -numa node,memdev=m1,cpus=1,nodeid=1 \ (...) qemu-system-riscv64: KVM AIA: failed to set the IMSIC address for hart 0 To make this patch work we would need changes in kvm_riscv_aia_create() to handle just the current socket. The loop I mentioned above is one place, and there's another place where we set group_bits and group_shift if socket_count > 1. To be honest I'm not sure if all these extra required changes are worth the simplification this patch is proposing. Thanks, Daniel > hw/riscv/virt.c | 79 +++++++++++++++++++++++-------------------------- > 1 file changed, 37 insertions(+), 42 deletions(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index dae46f4733cd..a52117ef71ee 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -58,14 +58,6 @@ > #include "qapi/qapi-visit-common.h" > #include "hw/virtio/virtio-iommu.h" > > -/* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */ > -static bool virt_use_kvm_aia_aplic_imsic(RISCVVirtAIAType aia_type) > -{ > - bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > - > - return riscv_is_kvm_aia_aplic_imsic(msimode); > -} > - > static bool virt_use_emulated_aplic(RISCVVirtAIAType aia_type) > { > bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > @@ -1298,10 +1290,12 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket, > return ret; > } > > -static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > +static DeviceState *virt_create_aia(RISCVVirtState *s, > const MemMapEntry *memmap, int socket, > int base_hartid, int hart_count) > { > + RISCVVirtAIAType aia_type = s->aia_type; > + int aia_guests = s->aia_guests; > int i; > hwaddr addr = 0; > uint32_t guest_bits; > @@ -1309,6 +1303,28 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > DeviceState *aplic_m = NULL; > bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > > + if (!kvm_enabled()) { > + /* Per-socket M-level APLIC */ > + aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + > + socket * memmap[VIRT_APLIC_M].size, > + memmap[VIRT_APLIC_M].size, > + (msimode) ? 0 : base_hartid, > + (msimode) ? 0 : hart_count, > + VIRT_IRQCHIP_NUM_SOURCES, > + VIRT_IRQCHIP_NUM_PRIO_BITS, > + msimode, true, NULL); > + } > + > + /* Per-socket S-level APLIC */ > + aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + > + socket * memmap[VIRT_APLIC_S].size, > + memmap[VIRT_APLIC_S].size, > + (msimode) ? 0 : base_hartid, > + (msimode) ? 0 : hart_count, > + VIRT_IRQCHIP_NUM_SOURCES, > + VIRT_IRQCHIP_NUM_PRIO_BITS, > + msimode, false, aplic_m); > + > if (msimode) { > if (!kvm_enabled()) { > /* Per-socket M-level IMSICs */ > @@ -1329,32 +1345,20 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > base_hartid + i, false, 1 + aia_guests, > VIRT_IRQCHIP_NUM_MSIS); > } > - } > > - if (!kvm_enabled()) { > - /* Per-socket M-level APLIC */ > - aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + > - socket * memmap[VIRT_APLIC_M].size, > - memmap[VIRT_APLIC_M].size, > - (msimode) ? 0 : base_hartid, > - (msimode) ? 0 : hart_count, > - VIRT_IRQCHIP_NUM_SOURCES, > - VIRT_IRQCHIP_NUM_PRIO_BITS, > - msimode, true, NULL); > - } > > - /* Per-socket S-level APLIC */ > - aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + > - socket * memmap[VIRT_APLIC_S].size, > - memmap[VIRT_APLIC_S].size, > - (msimode) ? 0 : base_hartid, > - (msimode) ? 0 : hart_count, > + if (kvm_irqchip_in_kernel()) { > + kvm_riscv_aia_create(MACHINE(s), IMSIC_MMIO_GROUP_MIN_SHIFT, > VIRT_IRQCHIP_NUM_SOURCES, > - VIRT_IRQCHIP_NUM_PRIO_BITS, > - msimode, false, aplic_m); > + VIRT_IRQCHIP_NUM_MSIS, > + memmap[VIRT_APLIC_S].base, > + memmap[VIRT_IMSIC_S].base, > + aia_guests); > + } > > - if (kvm_enabled() && msimode) { > - riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); > + if (kvm_enabled()) { > + riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); > + } > } > > return kvm_enabled() ? aplic_s : aplic_m; > @@ -1621,9 +1625,8 @@ static void virt_machine_init(MachineState *machine) > s->irqchip[i] = virt_create_plic(memmap, i, > base_hartid, hart_count); > } else { > - s->irqchip[i] = virt_create_aia(s->aia_type, s->aia_guests, > - memmap, i, base_hartid, > - hart_count); > + s->irqchip[i] = virt_create_aia(s, memmap, i, > + base_hartid, hart_count); > } > > /* Try to use different IRQCHIP instance based device type */ > @@ -1641,14 +1644,6 @@ static void virt_machine_init(MachineState *machine) > } > } > > - if (kvm_enabled() && virt_use_kvm_aia_aplic_imsic(s->aia_type)) { > - 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 (riscv_is_32bit(&s->soc[0])) { > #if HOST_LONG_BITS == 64 > /* limit RAM size in a 32-bit system */
Hi Daniel, On Tue, Feb 18, 2025 at 3:24 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > > > On 2/17/25 5:19 AM, Yong-Xuan Wang wrote: > > KVM AIA is only needed to be set when the virt machine use the AIA MSI. > > So we can move the KVM AIA configuration into virt_create_aia() to reduce > > the condition checking. > > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> > > --- > > Unfortunately this doesn't work. > > The reason is that kvm_riscv_aia_create(), as it is now, is called only once > during virt_machine_init() and it's already handling initialization for each socket: > > > for (socket = 0; socket < socket_count; socket++) { > socket_imsic_base = imsic_base + socket * (1U << group_shift); > hart_count = riscv_socket_hart_count(machine, socket); > base_hart = riscv_socket_first_hartid(machine, socket); > > if (max_hart_per_socket < hart_count) { > max_hart_per_socket = hart_count; > } > > for (i = 0; i < hart_count; i++) { > imsic_addr = socket_imsic_base + i * IMSIC_HART_SIZE(guest_bits); > ret = kvm_device_access(aia_fd, KVM_DEV_RISCV_AIA_GRP_ADDR, > KVM_DEV_RISCV_AIA_ADDR_IMSIC(i + base_hart), > &imsic_addr, true, NULL); > if (ret < 0) { > error_report("KVM AIA: failed to set the IMSIC address for hart %d", i); > exit(1); > } > } > } > > After this change, kvm_riscv_aia_create() is being called once for each socket since it's > now being called inside virt_create_aia(). And this will cause errors when running qemu-kvm > with more than one socket: > > ./qemu-system-riscv64 \ > -machine virt,accel=kvm,aia=aplic-imsic -m 2G \ > -object memory-backend-ram,size=1G,id=m0 \ > -object memory-backend-ram,size=1G,id=m1 \ > -smp 2,sockets=2,cores=1,threads=1 \ > -numa node,memdev=m0,cpus=0,nodeid=0 \ > -numa node,memdev=m1,cpus=1,nodeid=1 \ > (...) > qemu-system-riscv64: KVM AIA: failed to set the IMSIC address for hart 0 > Oh I forgot to test the NUMA config. Sorry. > > To make this patch work we would need changes in kvm_riscv_aia_create() to handle just the > current socket. The loop I mentioned above is one place, and there's another place where > we set group_bits and group_shift if socket_count > 1. > Also we need to find a place to initialize the in-kernel AIA after setting up all the IMSICs among sockets. This would make things more complicated. I will remove this patch in the next version. Thank you! Regards, Yong-Xuan > To be honest I'm not sure if all these extra required changes are worth the simplification > this patch is proposing. > > > Thanks, > > Daniel > > > > > > > hw/riscv/virt.c | 79 +++++++++++++++++++++++-------------------------- > > 1 file changed, 37 insertions(+), 42 deletions(-) > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > index dae46f4733cd..a52117ef71ee 100644 > > --- a/hw/riscv/virt.c > > +++ b/hw/riscv/virt.c > > @@ -58,14 +58,6 @@ > > #include "qapi/qapi-visit-common.h" > > #include "hw/virtio/virtio-iommu.h" > > > > -/* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */ > > -static bool virt_use_kvm_aia_aplic_imsic(RISCVVirtAIAType aia_type) > > -{ > > - bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > > - > > - return riscv_is_kvm_aia_aplic_imsic(msimode); > > -} > > - > > static bool virt_use_emulated_aplic(RISCVVirtAIAType aia_type) > > { > > bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > > @@ -1298,10 +1290,12 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket, > > return ret; > > } > > > > -static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > > +static DeviceState *virt_create_aia(RISCVVirtState *s, > > const MemMapEntry *memmap, int socket, > > int base_hartid, int hart_count) > > { > > + RISCVVirtAIAType aia_type = s->aia_type; > > + int aia_guests = s->aia_guests; > > int i; > > hwaddr addr = 0; > > uint32_t guest_bits; > > @@ -1309,6 +1303,28 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > > DeviceState *aplic_m = NULL; > > bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; > > > > + if (!kvm_enabled()) { > > + /* Per-socket M-level APLIC */ > > + aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + > > + socket * memmap[VIRT_APLIC_M].size, > > + memmap[VIRT_APLIC_M].size, > > + (msimode) ? 0 : base_hartid, > > + (msimode) ? 0 : hart_count, > > + VIRT_IRQCHIP_NUM_SOURCES, > > + VIRT_IRQCHIP_NUM_PRIO_BITS, > > + msimode, true, NULL); > > + } > > + > > + /* Per-socket S-level APLIC */ > > + aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + > > + socket * memmap[VIRT_APLIC_S].size, > > + memmap[VIRT_APLIC_S].size, > > + (msimode) ? 0 : base_hartid, > > + (msimode) ? 0 : hart_count, > > + VIRT_IRQCHIP_NUM_SOURCES, > > + VIRT_IRQCHIP_NUM_PRIO_BITS, > > + msimode, false, aplic_m); > > + > > if (msimode) { > > if (!kvm_enabled()) { > > /* Per-socket M-level IMSICs */ > > @@ -1329,32 +1345,20 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, > > base_hartid + i, false, 1 + aia_guests, > > VIRT_IRQCHIP_NUM_MSIS); > > } > > - } > > > > - if (!kvm_enabled()) { > > - /* Per-socket M-level APLIC */ > > - aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + > > - socket * memmap[VIRT_APLIC_M].size, > > - memmap[VIRT_APLIC_M].size, > > - (msimode) ? 0 : base_hartid, > > - (msimode) ? 0 : hart_count, > > - VIRT_IRQCHIP_NUM_SOURCES, > > - VIRT_IRQCHIP_NUM_PRIO_BITS, > > - msimode, true, NULL); > > - } > > > > - /* Per-socket S-level APLIC */ > > - aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + > > - socket * memmap[VIRT_APLIC_S].size, > > - memmap[VIRT_APLIC_S].size, > > - (msimode) ? 0 : base_hartid, > > - (msimode) ? 0 : hart_count, > > + if (kvm_irqchip_in_kernel()) { > > + kvm_riscv_aia_create(MACHINE(s), IMSIC_MMIO_GROUP_MIN_SHIFT, > > VIRT_IRQCHIP_NUM_SOURCES, > > - VIRT_IRQCHIP_NUM_PRIO_BITS, > > - msimode, false, aplic_m); > > + VIRT_IRQCHIP_NUM_MSIS, > > + memmap[VIRT_APLIC_S].base, > > + memmap[VIRT_IMSIC_S].base, > > + aia_guests); > > + } > > > > - if (kvm_enabled() && msimode) { > > - riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); > > + if (kvm_enabled()) { > > + riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); > > + } > > } > > > > return kvm_enabled() ? aplic_s : aplic_m; > > @@ -1621,9 +1625,8 @@ static void virt_machine_init(MachineState *machine) > > s->irqchip[i] = virt_create_plic(memmap, i, > > base_hartid, hart_count); > > } else { > > - s->irqchip[i] = virt_create_aia(s->aia_type, s->aia_guests, > > - memmap, i, base_hartid, > > - hart_count); > > + s->irqchip[i] = virt_create_aia(s, memmap, i, > > + base_hartid, hart_count); > > } > > > > /* Try to use different IRQCHIP instance based device type */ > > @@ -1641,14 +1644,6 @@ static void virt_machine_init(MachineState *machine) > > } > > } > > > > - if (kvm_enabled() && virt_use_kvm_aia_aplic_imsic(s->aia_type)) { > > - 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 (riscv_is_32bit(&s->soc[0])) { > > #if HOST_LONG_BITS == 64 > > /* limit RAM size in a 32-bit system */ >
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index dae46f4733cd..a52117ef71ee 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -58,14 +58,6 @@ #include "qapi/qapi-visit-common.h" #include "hw/virtio/virtio-iommu.h" -/* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */ -static bool virt_use_kvm_aia_aplic_imsic(RISCVVirtAIAType aia_type) -{ - bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; - - return riscv_is_kvm_aia_aplic_imsic(msimode); -} - static bool virt_use_emulated_aplic(RISCVVirtAIAType aia_type) { bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; @@ -1298,10 +1290,12 @@ static DeviceState *virt_create_plic(const MemMapEntry *memmap, int socket, return ret; } -static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, +static DeviceState *virt_create_aia(RISCVVirtState *s, const MemMapEntry *memmap, int socket, int base_hartid, int hart_count) { + RISCVVirtAIAType aia_type = s->aia_type; + int aia_guests = s->aia_guests; int i; hwaddr addr = 0; uint32_t guest_bits; @@ -1309,6 +1303,28 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, DeviceState *aplic_m = NULL; bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC; + if (!kvm_enabled()) { + /* Per-socket M-level APLIC */ + aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + + socket * memmap[VIRT_APLIC_M].size, + memmap[VIRT_APLIC_M].size, + (msimode) ? 0 : base_hartid, + (msimode) ? 0 : hart_count, + VIRT_IRQCHIP_NUM_SOURCES, + VIRT_IRQCHIP_NUM_PRIO_BITS, + msimode, true, NULL); + } + + /* Per-socket S-level APLIC */ + aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + + socket * memmap[VIRT_APLIC_S].size, + memmap[VIRT_APLIC_S].size, + (msimode) ? 0 : base_hartid, + (msimode) ? 0 : hart_count, + VIRT_IRQCHIP_NUM_SOURCES, + VIRT_IRQCHIP_NUM_PRIO_BITS, + msimode, false, aplic_m); + if (msimode) { if (!kvm_enabled()) { /* Per-socket M-level IMSICs */ @@ -1329,32 +1345,20 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests, base_hartid + i, false, 1 + aia_guests, VIRT_IRQCHIP_NUM_MSIS); } - } - if (!kvm_enabled()) { - /* Per-socket M-level APLIC */ - aplic_m = riscv_aplic_create(memmap[VIRT_APLIC_M].base + - socket * memmap[VIRT_APLIC_M].size, - memmap[VIRT_APLIC_M].size, - (msimode) ? 0 : base_hartid, - (msimode) ? 0 : hart_count, - VIRT_IRQCHIP_NUM_SOURCES, - VIRT_IRQCHIP_NUM_PRIO_BITS, - msimode, true, NULL); - } - /* Per-socket S-level APLIC */ - aplic_s = riscv_aplic_create(memmap[VIRT_APLIC_S].base + - socket * memmap[VIRT_APLIC_S].size, - memmap[VIRT_APLIC_S].size, - (msimode) ? 0 : base_hartid, - (msimode) ? 0 : hart_count, + if (kvm_irqchip_in_kernel()) { + kvm_riscv_aia_create(MACHINE(s), IMSIC_MMIO_GROUP_MIN_SHIFT, VIRT_IRQCHIP_NUM_SOURCES, - VIRT_IRQCHIP_NUM_PRIO_BITS, - msimode, false, aplic_m); + VIRT_IRQCHIP_NUM_MSIS, + memmap[VIRT_APLIC_S].base, + memmap[VIRT_IMSIC_S].base, + aia_guests); + } - if (kvm_enabled() && msimode) { - riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); + if (kvm_enabled()) { + riscv_aplic_set_kvm_msicfgaddr(RISCV_APLIC(aplic_s), addr); + } } return kvm_enabled() ? aplic_s : aplic_m; @@ -1621,9 +1625,8 @@ static void virt_machine_init(MachineState *machine) s->irqchip[i] = virt_create_plic(memmap, i, base_hartid, hart_count); } else { - s->irqchip[i] = virt_create_aia(s->aia_type, s->aia_guests, - memmap, i, base_hartid, - hart_count); + s->irqchip[i] = virt_create_aia(s, memmap, i, + base_hartid, hart_count); } /* Try to use different IRQCHIP instance based device type */ @@ -1641,14 +1644,6 @@ static void virt_machine_init(MachineState *machine) } } - if (kvm_enabled() && virt_use_kvm_aia_aplic_imsic(s->aia_type)) { - 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 (riscv_is_32bit(&s->soc[0])) { #if HOST_LONG_BITS == 64 /* limit RAM size in a 32-bit system */
KVM AIA is only needed to be set when the virt machine use the AIA MSI. So we can move the KVM AIA configuration into virt_create_aia() to reduce the condition checking. Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com> --- hw/riscv/virt.c | 79 +++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 42 deletions(-)