Hi Daniel,
I think this checking can be removed too. Would you send a patch to
fix it? Or I can remove it in this patch.
Regards,
Yong-Xuan
On Tue, Jun 6, 2023 at 2:45 AM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
>
>
> On 5/26/23 03:25, Yong-Xuan Wang wrote:
> > Remove M mode AIA devices when using KVM acceleration
> >
> > Signed-off-by: Yong-Xuan Wang <yongxuan.w...@sifive.com>
> > Reviewed-by: Jim Shu <jim....@sifive.com>
> > ---
> > hw/riscv/virt.c | 199 +++++++++++++++++++++++++-----------------------
> > 1 file changed, 105 insertions(+), 94 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 4e3efbee16..18b94888ab 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -531,52 +531,54 @@ static void create_fdt_imsic(RISCVVirtState *s, const
> > MemMapEntry *memmap,
> > imsic_cells = g_new0(uint32_t, ms->smp.cpus * 2);
> > imsic_regs = g_new0(uint32_t, socket_count * 4);
> >
> > - /* M-level IMSIC node */
> > - for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
> > - imsic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > - imsic_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_EXT);
> > - }
> > - imsic_max_hart_per_socket = 0;
> > - for (socket = 0; socket < socket_count; socket++) {
> > - imsic_addr = memmap[VIRT_IMSIC_M].base +
> > - socket * VIRT_IMSIC_GROUP_MAX_SIZE;
> > - imsic_size = IMSIC_HART_SIZE(0) * s->soc[socket].num_harts;
> > - imsic_regs[socket * 4 + 0] = 0;
> > - imsic_regs[socket * 4 + 1] = cpu_to_be32(imsic_addr);
> > - imsic_regs[socket * 4 + 2] = 0;
> > - imsic_regs[socket * 4 + 3] = cpu_to_be32(imsic_size);
> > - if (imsic_max_hart_per_socket < s->soc[socket].num_harts) {
> > - imsic_max_hart_per_socket = s->soc[socket].num_harts;
> > + if (!kvm_enabled()) {
> > + /* M-level IMSIC node */
> > + for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
> > + imsic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > + imsic_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_EXT);
> > }
> > - }
> > - imsic_name = g_strdup_printf("/soc/imsics@%lx",
> > - (unsigned long)memmap[VIRT_IMSIC_M].base);
> > - qemu_fdt_add_subnode(ms->fdt, imsic_name);
> > - qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible",
> > - "riscv,imsics");
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
> > - FDT_IMSIC_INT_CELLS);
> > - qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller",
> > - NULL, 0);
> > - qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller",
> > - NULL, 0);
> > - qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
> > - imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
> > - qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
> > - socket_count * sizeof(uint32_t) * 4);
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name, "riscv,num-ids",
> > - VIRT_IRQCHIP_NUM_MSIS);
> > - if (socket_count > 1) {
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name, "riscv,hart-index-bits",
> > - imsic_num_bits(imsic_max_hart_per_socket));
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name,
> > "riscv,group-index-bits",
> > - imsic_num_bits(socket_count));
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name,
> > "riscv,group-index-shift",
> > - IMSIC_MMIO_GROUP_MIN_SHIFT);
> > - }
> > - qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", *msi_m_phandle);
> > + imsic_max_hart_per_socket = 0;
> > + for (socket = 0; socket < socket_count; socket++) {
> > + imsic_addr = memmap[VIRT_IMSIC_M].base +
> > + socket * VIRT_IMSIC_GROUP_MAX_SIZE;
> > + imsic_size = IMSIC_HART_SIZE(0) * s->soc[socket].num_harts;
> > + imsic_regs[socket * 4 + 0] = 0;
> > + imsic_regs[socket * 4 + 1] = cpu_to_be32(imsic_addr);
> > + imsic_regs[socket * 4 + 2] = 0;
> > + imsic_regs[socket * 4 + 3] = cpu_to_be32(imsic_size);
> > + if (imsic_max_hart_per_socket < s->soc[socket].num_harts) {
> > + imsic_max_hart_per_socket = s->soc[socket].num_harts;
> > + }
> > + }
> > + imsic_name = g_strdup_printf("/soc/imsics@%lx",
> > + (unsigned long)memmap[VIRT_IMSIC_M].base);
> > + qemu_fdt_add_subnode(ms->fdt, imsic_name);
> > + qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible",
> > + "riscv,imsics");
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
> > + FDT_IMSIC_INT_CELLS);
> > + qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller",
> > + NULL, 0);
> > + qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller",
> > + NULL, 0);
> > + qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
> > + imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
> > + qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
> > + socket_count * sizeof(uint32_t) * 4);
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name, "riscv,num-ids",
> > + VIRT_IRQCHIP_NUM_MSIS);
> > + if (socket_count > 1) {
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name,
> > "riscv,hart-index-bits",
> > + imsic_num_bits(imsic_max_hart_per_socket));
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name,
> > "riscv,group-index-bits",
> > + imsic_num_bits(socket_count));
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name,
> > + "riscv,group-index-shift", IMSIC_MMIO_GROUP_MIN_SHIFT);
> > + }
> > + qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle",
> > *msi_m_phandle);
> >
> > - g_free(imsic_name);
> > + g_free(imsic_name);
> > + }
> >
> > /* S-level IMSIC node */
> > for (cpu = 0; cpu < ms->smp.cpus; cpu++) {
> > @@ -653,37 +655,40 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
> > aplic_s_phandle = (*phandle)++;
> > aplic_cells = g_new0(uint32_t, s->soc[socket].num_harts * 2);
> >
> > - /* M-level APLIC node */
> > - for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) {
> > - aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > - aplic_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_EXT);
> > - }
> > - aplic_addr = memmap[VIRT_APLIC_M].base +
> > - (memmap[VIRT_APLIC_M].size * socket);
> > - aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> > - qemu_fdt_add_subnode(ms->fdt, aplic_name);
> > - qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible",
> > "riscv,aplic");
> > - qemu_fdt_setprop_cell(ms->fdt, aplic_name,
> > - "#interrupt-cells", FDT_APLIC_INT_CELLS);
> > - qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
> > - if (s->aia_type == VIRT_AIA_TYPE_APLIC) {
> > - qemu_fdt_setprop(ms->fdt, aplic_name, "interrupts-extended",
> > - aplic_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 2);
> > - } else {
> > - qemu_fdt_setprop_cell(ms->fdt, aplic_name, "msi-parent",
> > - msi_m_phandle);
> > + if (!kvm_enabled()) {
> > + /* M-level APLIC node */
> > + for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) {
> > + aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> > + aplic_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_EXT);
> > + }
> > + aplic_addr = memmap[VIRT_APLIC_M].base +
> > + (memmap[VIRT_APLIC_M].size * socket);
> > + aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> > + qemu_fdt_add_subnode(ms->fdt, aplic_name);
> > + qemu_fdt_setprop_string(ms->fdt, aplic_name,
> > + "compatible", "riscv,aplic");
> > + qemu_fdt_setprop_cell(ms->fdt, aplic_name,
> > + "#interrupt-cells", FDT_APLIC_INT_CELLS);
> > + qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller",
> > NULL, 0);
> > + if (s->aia_type == VIRT_AIA_TYPE_APLIC) {
> > + qemu_fdt_setprop(ms->fdt, aplic_name, "interrupts-extended",
> > + aplic_cells, s->soc[socket].num_harts * sizeof(uint32_t) *
> > 2);
> > + } else {
> > + qemu_fdt_setprop_cell(ms->fdt, aplic_name, "msi-parent",
> > + msi_m_phandle);
> > + }
> > + qemu_fdt_setprop_cells(ms->fdt, aplic_name, "reg",
> > + 0x0, aplic_addr, 0x0, memmap[VIRT_APLIC_M].size);
> > + qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,num-sources",
> > + VIRT_IRQCHIP_NUM_SOURCES);
> > + qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,children",
> > + aplic_s_phandle);
> > + qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> > + aplic_s_phandle, 0x1, VIRT_IRQCHIP_NUM_SOURCES);
> > + riscv_socket_fdt_write_id(ms, aplic_name, socket);
> > + qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle",
> > aplic_m_phandle);
> > + g_free(aplic_name);
> > }
> > - qemu_fdt_setprop_cells(ms->fdt, aplic_name, "reg",
> > - 0x0, aplic_addr, 0x0, memmap[VIRT_APLIC_M].size);
> > - qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,num-sources",
> > - VIRT_IRQCHIP_NUM_SOURCES);
> > - qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,children",
> > - aplic_s_phandle);
> > - qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> > - aplic_s_phandle, 0x1, VIRT_IRQCHIP_NUM_SOURCES);
> > - riscv_socket_fdt_write_id(ms, aplic_name, socket);
> > - qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle", aplic_m_phandle);
> > - g_free(aplic_name);
> >
> > /* S-level APLIC node */
> > for (cpu = 0; cpu < s->soc[socket].num_harts; cpu++) {
> > @@ -1162,16 +1167,20 @@ static DeviceState
> > *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests,
> > int i;
> > hwaddr addr;
> > uint32_t guest_bits;
> > - DeviceState *aplic_m;
> > + DeviceState *aplic_s = NULL;
> > + DeviceState *aplic_m = NULL;
> > bool msimode = (aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) ? true : false;
> >
> > if (msimode) {
> > - /* Per-socket M-level IMSICs */
> > - addr = memmap[VIRT_IMSIC_M].base + socket *
> > VIRT_IMSIC_GROUP_MAX_SIZE;
> > - for (i = 0; i < hart_count; i++) {
> > - riscv_imsic_create(addr + i * IMSIC_HART_SIZE(0),
> > - base_hartid + i, true, 1,
> > - VIRT_IRQCHIP_NUM_MSIS);
> > + if (!kvm_enabled()) {
> > + /* Per-socket M-level IMSICs */
> > + addr = memmap[VIRT_IMSIC_M].base +
> > + socket * VIRT_IMSIC_GROUP_MAX_SIZE;
> > + for (i = 0; i < hart_count; i++) {
> > + riscv_imsic_create(addr + i * IMSIC_HART_SIZE(0),
> > + base_hartid + i, true, 1,
> > + VIRT_IRQCHIP_NUM_MSIS);
> > + }
> > }
> >
> > /* Per-socket S-level IMSICs */
> > @@ -1184,19 +1193,21 @@ static DeviceState
> > *virt_create_aia(RISCVVirtAIAType aia_type, int aia_guests,
> > }
> > }
> >
> > - /* 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);
> > -
> > - if (aplic_m) {
> > + 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);
> > + }
> > +
> > + if (aplic_m || kvm_enabled()) {
>
> I am struggling to understand why we're checking for "aplic_m" here given that
> riscv_aplic_create() will either returning something not NULL or error out via
> &error_fatal.
>
> This behavior predates this patch though.
>
>
>
> Reviewed-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
>
> > /* Per-socket S-level APLIC */
> > - riscv_aplic_create(
> > + 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,
> > @@ -1206,7 +1217,7 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType
> > aia_type, int aia_guests,
> > msimode, false, aplic_m);
> > }
> >
> > - return aplic_m;
> > + return kvm_enabled() ? aplic_s : aplic_m;
> > }
> >
> > static void create_platform_bus(RISCVVirtState *s, DeviceState *irqchip)
