Re: [PATCH v2 4/6] hw/riscv/virt.c: calculate socket count once in create_fdt_imsic()

[Alistair Francis]

On Tue, Jan 17, 2023 at 3:36 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> riscv_socket_count() returns either ms->numa_state->num_nodes or 1
> depending on NUMA support. In any case the value can be retrieved only
> once and used in the rest of the function.
>
> This will also alleviate the rename we're going to do next by reducing
> the instances of MachineState 'mc' inside hw/riscv/virt.c.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index cbba0b8930..8ff89c217f 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -505,13 +505,14 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      int cpu, socket;
>      char *imsic_name;
>      MachineState *mc = MACHINE(s);
> +    int socket_count = riscv_socket_count(mc);
>      uint32_t imsic_max_hart_per_socket, imsic_guest_bits;
>      uint32_t *imsic_cells, *imsic_regs, imsic_addr, imsic_size;
>
>      *msi_m_phandle = (*phandle)++;
>      *msi_s_phandle = (*phandle)++;
>      imsic_cells = g_new0(uint32_t, mc->smp.cpus * 2);
> -    imsic_regs = g_new0(uint32_t, riscv_socket_count(mc) * 4);
> +    imsic_regs = g_new0(uint32_t, socket_count * 4);
>
>      /* M-level IMSIC node */
>      for (cpu = 0; cpu < mc->smp.cpus; cpu++) {
> @@ -519,7 +520,7 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>          imsic_cells[cpu * 2 + 1] = cpu_to_be32(IRQ_M_EXT);
>      }
>      imsic_max_hart_per_socket = 0;
> -    for (socket = 0; socket < riscv_socket_count(mc); socket++) {
> +    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;
> @@ -545,14 +546,14 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      qemu_fdt_setprop(mc->fdt, imsic_name, "interrupts-extended",
>          imsic_cells, mc->smp.cpus * sizeof(uint32_t) * 2);
>      qemu_fdt_setprop(mc->fdt, imsic_name, "reg", imsic_regs,
> -        riscv_socket_count(mc) * sizeof(uint32_t) * 4);
> +        socket_count * sizeof(uint32_t) * 4);
>      qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,num-ids",
>          VIRT_IRQCHIP_NUM_MSIS);
> -    if (riscv_socket_count(mc) > 1) {
> +    if (socket_count > 1) {
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,hart-index-bits",
>              imsic_num_bits(imsic_max_hart_per_socket));
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,group-index-bits",
> -            imsic_num_bits(riscv_socket_count(mc)));
> +            imsic_num_bits(socket_count));
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,group-index-shift",
>              IMSIC_MMIO_GROUP_MIN_SHIFT);
>      }
> @@ -567,7 +568,7 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      }
>      imsic_guest_bits = imsic_num_bits(s->aia_guests + 1);
>      imsic_max_hart_per_socket = 0;
> -    for (socket = 0; socket < riscv_socket_count(mc); socket++) {
> +    for (socket = 0; socket < socket_count; socket++) {
>          imsic_addr = memmap[VIRT_IMSIC_S].base +
>                       socket * VIRT_IMSIC_GROUP_MAX_SIZE;
>          imsic_size = IMSIC_HART_SIZE(imsic_guest_bits) *
> @@ -594,18 +595,18 @@ static void create_fdt_imsic(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      qemu_fdt_setprop(mc->fdt, imsic_name, "interrupts-extended",
>          imsic_cells, mc->smp.cpus * sizeof(uint32_t) * 2);
>      qemu_fdt_setprop(mc->fdt, imsic_name, "reg", imsic_regs,
> -        riscv_socket_count(mc) * sizeof(uint32_t) * 4);
> +        socket_count * sizeof(uint32_t) * 4);
>      qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,num-ids",
>          VIRT_IRQCHIP_NUM_MSIS);
>      if (imsic_guest_bits) {
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,guest-index-bits",
>              imsic_guest_bits);
>      }
> -    if (riscv_socket_count(mc) > 1) {
> +    if (socket_count > 1) {
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,hart-index-bits",
>              imsic_num_bits(imsic_max_hart_per_socket));
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,group-index-bits",
> -            imsic_num_bits(riscv_socket_count(mc)));
> +            imsic_num_bits(socket_count));
>          qemu_fdt_setprop_cell(mc->fdt, imsic_name, "riscv,group-index-shift",
>              IMSIC_MMIO_GROUP_MIN_SHIFT);
>      }
> @@ -733,6 +734,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      MachineState *mc = MACHINE(s);
>      uint32_t msi_m_phandle = 0, msi_s_phandle = 0;
>      uint32_t *intc_phandles, xplic_phandles[MAX_NODES];
> +    int socket_count = riscv_socket_count(mc);
>
>      qemu_fdt_add_subnode(mc->fdt, "/cpus");
>      qemu_fdt_setprop_cell(mc->fdt, "/cpus", "timebase-frequency",
> @@ -744,7 +746,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      intc_phandles = g_new0(uint32_t, mc->smp.cpus);
>
>      phandle_pos = mc->smp.cpus;
> -    for (socket = (riscv_socket_count(mc) - 1); socket >= 0; socket--) {
> +    for (socket = (socket_count - 1); socket >= 0; socket--) {
>          phandle_pos -= s->soc[socket].num_harts;
>
>          clust_name = g_strdup_printf("/cpus/cpu-map/cluster%d", socket);
> @@ -775,7 +777,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>      }
>
>      phandle_pos = mc->smp.cpus;
> -    for (socket = (riscv_socket_count(mc) - 1); socket >= 0; socket--) {
> +    for (socket = (socket_count - 1); socket >= 0; socket--) {
>          phandle_pos -= s->soc[socket].num_harts;
>
>          if (s->aia_type == VIRT_AIA_TYPE_NONE) {
> @@ -790,7 +792,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const 
> MemMapEntry *memmap,
>
>      g_free(intc_phandles);
>
> -    for (socket = 0; socket < riscv_socket_count(mc); socket++) {
> +    for (socket = 0; socket < socket_count; socket++) {
>          if (socket == 0) {
>              *irq_mmio_phandle = xplic_phandles[socket];
>              *irq_virtio_phandle = xplic_phandles[socket];
> @@ -1051,7 +1053,8 @@ static void create_fdt(RISCVVirtState *s, const 
> MemMapEntry *memmap)
>
>      /* Pass seed to RNG */
>      qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> -    qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed", rng_seed, 
> sizeof(rng_seed));
> +    qemu_fdt_setprop(mc->fdt, "/chosen", "rng-seed",
> +                     rng_seed, sizeof(rng_seed));
>  }
>
>  static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
> @@ -1335,9 +1338,10 @@ static void virt_machine_init(MachineState *machine)
>      char *soc_name;
>      DeviceState *mmio_irqchip, *virtio_irqchip, *pcie_irqchip;
>      int i, base_hartid, hart_count;
> +    int socket_count = riscv_socket_count(machine);
>
>      /* Check socket count limit */
> -    if (VIRT_SOCKETS_MAX < riscv_socket_count(machine)) {
> +    if (VIRT_SOCKETS_MAX < socket_count) {
>          error_report("number of sockets/nodes should be less than %d",
>              VIRT_SOCKETS_MAX);
>          exit(1);
> @@ -1345,7 +1349,7 @@ static void virt_machine_init(MachineState *machine)
>
>      /* Initialize sockets */
>      mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
> -    for (i = 0; i < riscv_socket_count(machine); i++) {
> +    for (i = 0; i < socket_count; i++) {
>          if (!riscv_socket_check_hartids(machine, i)) {
>              error_report("discontinuous hartids in socket%d", i);
>              exit(1);
> --
> 2.39.0
>
>