| Message ID | 1461219834-10416-2-git-send-email-zhaoshenglong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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); > } > } > 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,
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 > >
On 2016/4/23 15:03, Andrew Jones wrote: > 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. > Agree. I thought this before. It could not provide this /distance-map node as well as it doesn't provide SLIT table for ACPI. So I'll drop this patch. Thanks,
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; + 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); + g_free(matrix); } static void fdt_add_psci_node(const VirtBoardInfo *vbi)