Re: [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA

[Andrew Jones <drjones@redhat.com>]

On Thu, Apr 21, 2016 at 02:23:50PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> This /distance-map node is used to describe the accessing distance
> between NUMA nodes.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 56d35c7..814a1eb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -40,6 +40,7 @@
>  #include "sysemu/device_tree.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/numa.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
>  #include "exec/address-spaces.h"
> @@ -203,6 +204,9 @@ static VirtBoardInfo *find_machine_info(const char *cpu)
>  
>  static void create_fdt(VirtBoardInfo *vbi)
>  {
> +    unsigned int i, j, number, count;

s/count/index/ ?

> +    uint64_t *matrix;
> +
>      void *fdt = create_device_tree(&vbi->fdt_size);
>  
>      if (!fdt) {
> @@ -239,6 +243,32 @@ static void create_fdt(VirtBoardInfo *vbi)
>                                  "clk24mhz");
>      qemu_fdt_setprop_cell(fdt, "/apb-pclk", "phandle", vbi->clock_phandle);
>  
> +    if (nb_numa_nodes <= 0) {
> +        return;
> +    }
> +
> +    /* Add /distance-map node for NUMA */
> +    qemu_fdt_add_subnode(fdt, "/distance-map");
> +    qemu_fdt_setprop_string(fdt, "/distance-map", "compatible",
> +                            "numa-distance-map-v1");
> +
> +    number = nb_numa_nodes * nb_numa_nodes * 6;
> +    matrix = g_malloc0(number * sizeof(uint64_t));
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        for (j = 0; j < nb_numa_nodes; j++) {
> +            count = (i * nb_numa_nodes + j) * 6;
> +            matrix[count++] = 1;
> +            matrix[count++] = i;
> +            matrix[count++] = 1;
> +            matrix[count++] = j;
> +            matrix[count++] = 1;
> +            matrix[count++] = (i == j) ? 10 : 20;
> +        }
> +    }
> +    qemu_fdt_setprop_sized_cells_from_array(fdt, "/distance-map",
> +                                            "distance-matrix", number / 2,
> +                                            matrix);

I had to read qemu_fdt_setprop_sized_cells_from_array to understand why
above we're using 6 instead of 3, and then placing all the 1's in every
other slot, and then dividing number by 2 here. Is using this function
worth the confusion?

I think the following would greatly improve reviewability, and shave off
a bit of boot time (by not having to alloc more mem and copy the matrix).

    uint32_t *matrix;

    number = nb_numa_nodes * nb_numa_nodes * 3;
    matrix = g_malloc0(number * sizeof(uint32_t));
    for (i = 0; i < nb_numa_nodes; i++) {
        for (j = 0; j < nb_numa_nodes; j++) {
            count = (i * nb_numa_nodes + j) * 3;
            matrix[count++] = cpu_to_be32(i);
            matrix[count++] = cpu_to_be32(j);
            matrix[count++] = cpu_to_be32(i == j ? 10 : 20);
        }
    }
    qemu_fdt_setprop(fdt, "/distance-map", "distance-matrix",
                     matrix, number * sizeof(uint32_t));


> +    g_free(matrix);

Also, I think it would nicer if all this was put in its own function, and
then just add the following to create_fdt.

  if (nb_numa_nodes) {
      virt_fdt_create_distance_map(fdt);
  }

>  }
>  
>  static void fdt_add_psci_node(const VirtBoardInfo *vbi)
> -- 
> 2.0.4

Otherwise per https://lkml.org/lkml/2016/4/8/572 it looks good.

Thanks,
drew