Re: [PATCH v5 1/5] target/riscv: support the AIA device emulation with KVM enabled

[Andrew Jones]

On Thu, Jul 13, 2023 at 08:43:53AM +0000, Yong-Xuan Wang wrote:
> In this patch, we create the APLIC and IMSIC FDT helper functions and
> remove M mode AIA devices when using KVM acceleration.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 264 ++++++++++++++++++++++--------------------------
>  1 file changed, 123 insertions(+), 141 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d90286dc46..26b3aff28e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -516,79 +516,28 @@ static uint32_t imsic_num_bits(uint32_t count)
>      return ret;
>  }
>  
> -static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
> -                             uint32_t *phandle, uint32_t *intc_phandles,
> -                             uint32_t *msi_m_phandle, uint32_t 
> *msi_s_phandle)
> +static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
> +                                 uint32_t *intc_phandles, uint32_t 
> msi_phandle,
> +                                 bool m_mode, uint32_t imsic_guest_bits)
>  {
>      int cpu, socket;
>      char *imsic_name;
>      MachineState *ms = MACHINE(s);
>      int socket_count = riscv_socket_count(ms);
> -    uint32_t imsic_max_hart_per_socket, imsic_guest_bits;
> +    uint32_t imsic_max_hart_per_socket;
>      uint32_t *imsic_cells, *imsic_regs, imsic_addr, imsic_size;
>  
> -    *msi_m_phandle = (*phandle)++;
> -    *msi_s_phandle = (*phandle)++;
>      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;
> -        }
> -    }
> -    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);
> +        imsic_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : 
> IRQ_S_EXT);
>      }
> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", *msi_m_phandle);
>  
> -    g_free(imsic_name);
> -
> -    /* S-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_S_EXT);
> -    }
> -    imsic_guest_bits = imsic_num_bits(s->aia_guests + 1);
>      imsic_max_hart_per_socket = 0;
>      for (socket = 0; socket < socket_count; socket++) {
> -        imsic_addr = memmap[VIRT_IMSIC_S].base +
> -                     socket * VIRT_IMSIC_GROUP_MAX_SIZE;
> +        imsic_addr = base_addr + socket * VIRT_IMSIC_GROUP_MAX_SIZE;
>          imsic_size = IMSIC_HART_SIZE(imsic_guest_bits) *
>                       s->soc[socket].num_harts;
>          imsic_regs[socket * 4 + 0] = 0;
> @@ -599,119 +548,149 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>              imsic_max_hart_per_socket = s->soc[socket].num_harts;
>          }
>      }
> -    imsic_name = g_strdup_printf("/soc/imsics@%lx",
> -        (unsigned long)memmap[VIRT_IMSIC_S].base);
> +
> +    imsic_name = g_strdup_printf("/soc/imsics@%lx", (unsigned 
> long)base_addr);
>      qemu_fdt_add_subnode(ms->fdt, imsic_name);
> -    qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible",
> -        "riscv,imsics");
> +    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, "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);

Since we're doing a lot of reformatting, then I'd prefer these second line
parameters be aligned under the first parameters. However this patch is
tough to review with all the refactoring and formatting changes together.
It'd be best to do the refactoring without any formatting changes first,
and then create a second patch which makes all the formatting changes.

> +
>      if (imsic_guest_bits) {
>          qemu_fdt_setprop_cell(ms->fdt, imsic_name, "riscv,guest-index-bits",
>              imsic_guest_bits);
>      }
> +
>      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,
> +            "riscv,group-index-shift", IMSIC_MMIO_GROUP_MIN_SHIFT);
>      }
> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", *msi_s_phandle);
> -    g_free(imsic_name);
> +    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", msi_phandle);
>  
> +    g_free(imsic_name);
>      g_free(imsic_regs);
>      g_free(imsic_cells);
>  }
>  
> -static void create_fdt_socket_aplic(RISCVVirtState *s,
> -                                    const MemMapEntry *memmap, int socket,
> -                                    uint32_t msi_m_phandle,
> -                                    uint32_t msi_s_phandle,
> -                                    uint32_t *phandle,
> -                                    uint32_t *intc_phandles,
> -                                    uint32_t *aplic_phandles)
> +static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
> +                             uint32_t *phandle, uint32_t *intc_phandles,
> +                             uint32_t *msi_m_phandle, uint32_t 
> *msi_s_phandle)
> +{
> +    *msi_m_phandle = (*phandle)++;
> +    *msi_s_phandle = (*phandle)++;
> +
> +    if (!kvm_enabled()) {
> +        /* M-level IMSIC node */
> +        create_fdt_one_imsic(s, memmap[VIRT_IMSIC_M].base, intc_phandles,
> +                             *msi_m_phandle, true, 0);
> +    }
> +
> +    /* S-level IMSIC node */
> +    create_fdt_one_imsic(s, memmap[VIRT_IMSIC_S].base, intc_phandles,
> +                         *msi_s_phandle, false,
> +                         imsic_num_bits(s->aia_guests + 1));
> +
> +}
> +
> +static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
> +                                 unsigned long aplic_addr, uint32_t 
> aplic_size,
> +                                 uint32_t msi_phandle,
> +                                 uint32_t *intc_phandles,
> +                                 uint32_t aplic_phandle,
> +                                 uint32_t aplic_child_phandle,
> +                                 bool m_mode)
>  {
>      int cpu;
>      char *aplic_name;
>      uint32_t *aplic_cells;
> -    unsigned long aplic_addr;
>      MachineState *ms = MACHINE(s);
> -    uint32_t aplic_m_phandle, aplic_s_phandle;
>  
> -    aplic_m_phandle = (*phandle)++;
> -    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_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : 
> IRQ_S_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_cell(ms->fdt, aplic_name, "msi-parent", 
> msi_phandle);
>      }
> +
>      qemu_fdt_setprop_cells(ms->fdt, aplic_name, "reg",
> -        0x0, aplic_addr, 0x0, memmap[VIRT_APLIC_M].size);
> +        0x0, aplic_addr, 0x0, aplic_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);
> +
> +    if (aplic_child_phandle) {
> +        qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,children",
> +            aplic_child_phandle);
> +        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> +            aplic_child_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);
> +    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle", aplic_phandle);
> +
>      g_free(aplic_name);
> +    g_free(aplic_cells);
> +}
>  
> -    /* S-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_S_EXT);
> +static void create_fdt_socket_aplic(RISCVVirtState *s,
> +                                    const MemMapEntry *memmap, int socket,
> +                                    uint32_t msi_m_phandle,
> +                                    uint32_t msi_s_phandle,
> +                                    uint32_t *phandle,
> +                                    uint32_t *intc_phandles,
> +                                    uint32_t *aplic_phandles)
> +{
> +    char *aplic_name;
> +    unsigned long aplic_addr;
> +    MachineState *ms = MACHINE(s);
> +    uint32_t aplic_m_phandle, aplic_s_phandle;
> +
> +    aplic_m_phandle = (*phandle)++;
> +    aplic_s_phandle = (*phandle)++;
> +
> +    if (!kvm_enabled()) {
> +        /* M-level APLIC node */
> +        aplic_addr = memmap[VIRT_APLIC_M].base +
> +                     (memmap[VIRT_APLIC_M].size * socket);
> +        create_fdt_one_aplic(s, socket, aplic_addr, 
> memmap[VIRT_APLIC_M].size,
> +                             msi_m_phandle, intc_phandles,
> +                             aplic_m_phandle, aplic_s_phandle,
> +                             true);
>      }
> +
> +    /* S-level APLIC node */
>      aplic_addr = memmap[VIRT_APLIC_S].base +
>                   (memmap[VIRT_APLIC_S].size * socket);
> +    create_fdt_one_aplic(s, socket, aplic_addr, memmap[VIRT_APLIC_S].size,
> +                         msi_s_phandle, intc_phandles,
> +                         aplic_s_phandle, 0,
> +                         false);
> +
>      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_s_phandle);
> -    }
> -    qemu_fdt_setprop_cells(ms->fdt, aplic_name, "reg",
> -        0x0, aplic_addr, 0x0, memmap[VIRT_APLIC_S].size);
> -    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,num-sources",
> -        VIRT_IRQCHIP_NUM_SOURCES);
> -    riscv_socket_fdt_write_id(ms, aplic_name, socket);
> -    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "phandle", aplic_s_phandle);
>  
>      if (!socket) {
>          platform_bus_add_all_fdt_nodes(ms->fdt, aplic_name,
> @@ -722,7 +701,6 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>  
>      g_free(aplic_name);
>  
> -    g_free(aplic_cells);
>      aplic_phandles[socket] = aplic_s_phandle;
>  }
>  
> @@ -1163,16 +1141,20 @@ static DeviceState *virt_create_aia(RISCVVirtAIAType 
> aia_type, int aia_guests,
>      int i;
>      hwaddr addr;
>      uint32_t guest_bits;
> -    DeviceState *aplic_m;
> -    bool msimode = (aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) ? true : false;
> +    DeviceState *aplic_s = NULL;
> +    DeviceState *aplic_m = NULL;
> +    bool msimode = aia_type == VIRT_AIA_TYPE_APLIC_IMSIC;
>  
>      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);

Please align these parameters with the first parameter.

> +            }
>          }
>  
>          /* Per-socket S-level IMSICs */
> @@ -1185,29 +1167,29 @@ 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,
> +    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);

I'm not sure what the best way to indent all these parameters is, I'd
probably put them one level under the function name unless a checkpatch
policy told me otherwise. I do know that this style used for aplic_m is
inconsistent with the style below used for aplic_s. Consistency is what
really matters, so we should choose one or the other.

> +    }
> +
> +    /* 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, true, NULL);
> -
> -    if (aplic_m) {
> -        /* Per-socket S-level APLIC */
> -        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);
> -    }
> +        msimode, false, aplic_m);
>  
> -    return aplic_m;
> +    return kvm_enabled() ? aplic_s : aplic_m;
>  }
>  
>  static void create_platform_bus(RISCVVirtState *s, DeviceState *irqchip)
> -- 
> 2.17.1
>

So, as stated above, this is a bit tricky to review with all the
formatting changes intermixed and it'd probably best to do refactoring
without any other changes first and then another pass that reformats,
but at least this stuff should be easy to test. I presume the DTB was
checked before and after this patch and found to be identical?

Despite the formatting issues, LGTM

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew