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

[Andrew Jones <drjones@redhat.com>]

On Sat, Apr 23, 2016 at 09:17:25AM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/4/22 20:25, Andrew Jones wrote:
> > 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);

I noticed that /distance-map is an optional node by the latest version
of the spec. In its absence default values will be used. Do we plan on
putting anything other then the 10s and 20s here? If not, then we can
leave it to Linux to determine what the defaults should be, and it'll
use them by itself if we leave this node out.

> >         }
> >     }
> >     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);
> >   }
> > 
> Ok, will update this.

Thanks,
drew

> 
> Thanks,
> -- 
> Shannon
> 
>