* [Qemu-devel] [PATCH v5 0/5] ARM: Add NUMA support for machine virt
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
From: Shannon Zhao <shannon.zhao@linaro.org>
Add NUMA support for machine virt. Tested successfully running a guest
Linux kernel with the following patch applied:
- [PATCH v16 0/6] arm64, numa: Add numa support for arm64 platforms
https://lkml.org/lkml/2016/4/8/571
- [PATCH v5 00/14] ACPI NUMA support for ARM64
https://lkml.org/lkml/2016/4/19/852
Example qemu command line:
qemu-system-aarch64 \
-enable-kvm -smp 4\
-kernel Image \
-m 512 -machine virt,kernel_irqchip=on \
-initrd guestfs.cpio.gz \
-cpu host -nographic \
-numa node,mem=256M,cpus=0-1,nodeid=0 \
-numa node,mem=256M,cpus=2-3,nodeid=1 \
-append "console=ttyAMA0 root=/dev/ram"
Changes since v4:
* rebased on new kernel driver and device bindings, especially the
compatible string "numa-distance-map-v1" of /distance-map node
* set the numa-node-id for first /memory node
Changes since v3:
* based on new kernel driver and device bindings
* add ACPI part
Changes since v2:
* update to use NUMA node property arm,associativity.
Changes since v1:
Take into account Peter's comments:
* rename virt_memory_init to arm_generate_memory_dtb
* move arm_generate_memory_dtb to boot.c and make it a common func
* use a struct numa_map to generate numa dtb
Shannon Zhao (5):
ARM: Virt: Add /distance-map node for NUMA
ARM: Virt: Set numa-node-id for CPUs
ARM: Add numa-node-id for /memory node
include/hw/acpi/acpi-defs: Add GICC Affinity Structure
hw/arm/virt-acpi-build: Generate SRAT table
hw/arm/boot.c | 31 +++++++++++++++++++++++-
hw/arm/virt-acpi-build.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
hw/arm/virt.c | 37 +++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 2 +-
include/hw/acpi/acpi-defs.h | 15 +++++++++++-
5 files changed, 140 insertions(+), 3 deletions(-)
--
2.0.4
^ permalink raw reply [flat|nested] 38+ messages in thread* [Qemu-arm] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-21 6:23 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: peter.huangpeng, drjones, shannon.zhao, qemu-devel, david.daney
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;
+ 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)
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
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;
+ 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)
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
2016-04-21 6:23 ` [Qemu-devel] " Shannon Zhao
@ 2016-04-22 12:25 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:25 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
@ 2016-04-22 12:25 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:25 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, peter.maydell, peter.huangpeng, shannon.zhao,
qemu-devel, david.daney
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
2016-04-22 12:25 ` Andrew Jones
@ 2016-04-23 1:17 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:17 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
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,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
@ 2016-04-23 1:17 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:17 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-arm, peter.maydell, peter.huangpeng, shannon.zhao,
qemu-devel, david.daney
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,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
2016-04-23 1:17 ` Shannon Zhao
@ 2016-04-23 7:03 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-23 7:03 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao
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
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
@ 2016-04-23 7:03 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-23 7:03 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
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
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
2016-04-23 7:03 ` Andrew Jones
@ 2016-04-23 7:27 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 7:27 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao
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,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/5] ARM: Virt: Add /distance-map node for NUMA
@ 2016-04-23 7:27 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 7:27 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
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,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-21 6:23 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: peter.huangpeng, drjones, shannon.zhao, qemu-devel, david.daney
From: Shannon Zhao <shannon.zhao@linaro.org>
Add a numa-node-id property to specify NUMA information for CPUs.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/virt.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 814a1eb..d21e9a0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -359,6 +359,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
{
int cpu;
int addr_cells = 1;
+ unsigned int i;
/*
* From Documentation/devicetree/bindings/arm/cpus.txt
@@ -408,6 +409,12 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
armcpu->mp_affinity);
}
+ for (i = 0; i < nb_numa_nodes; i++) {
+ if (test_bit(cpu, numa_info[i].node_cpu)) {
+ qemu_fdt_setprop_cell(vbi->fdt, nodename, "numa-node-id", i);
+ }
+ }
+
g_free(nodename);
}
}
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
From: Shannon Zhao <shannon.zhao@linaro.org>
Add a numa-node-id property to specify NUMA information for CPUs.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/virt.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 814a1eb..d21e9a0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -359,6 +359,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
{
int cpu;
int addr_cells = 1;
+ unsigned int i;
/*
* From Documentation/devicetree/bindings/arm/cpus.txt
@@ -408,6 +409,12 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
armcpu->mp_affinity);
}
+ for (i = 0; i < nb_numa_nodes; i++) {
+ if (test_bit(cpu, numa_info[i].node_cpu)) {
+ qemu_fdt_setprop_cell(vbi->fdt, nodename, "numa-node-id", i);
+ }
+ }
+
g_free(nodename);
}
}
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-22 12:34 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:34 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
On Thu, Apr 21, 2016 at 02:23:51PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Add a numa-node-id property to specify NUMA information for CPUs.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/virt.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 814a1eb..d21e9a0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -359,6 +359,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> {
> int cpu;
> int addr_cells = 1;
> + unsigned int i;
>
> /*
> * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -408,6 +409,12 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> armcpu->mp_affinity);
> }
>
> + for (i = 0; i < nb_numa_nodes; i++) {
> + if (test_bit(cpu, numa_info[i].node_cpu)) {
> + qemu_fdt_setprop_cell(vbi->fdt, nodename, "numa-node-id", i);
> + }
> + }
> +
> g_free(nodename);
> }
> }
> --
> 2.0.4
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs
@ 2016-04-22 12:34 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:34 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, peter.maydell, peter.huangpeng, shannon.zhao,
qemu-devel, david.daney
On Thu, Apr 21, 2016 at 02:23:51PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Add a numa-node-id property to specify NUMA information for CPUs.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/virt.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 814a1eb..d21e9a0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -359,6 +359,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> {
> int cpu;
> int addr_cells = 1;
> + unsigned int i;
>
> /*
> * From Documentation/devicetree/bindings/arm/cpus.txt
> @@ -408,6 +409,12 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
> armcpu->mp_affinity);
> }
>
> + for (i = 0; i < nb_numa_nodes; i++) {
> + if (test_bit(cpu, numa_info[i].node_cpu)) {
> + qemu_fdt_setprop_cell(vbi->fdt, nodename, "numa-node-id", i);
> + }
> + }
> +
> g_free(nodename);
> }
> }
> --
> 2.0.4
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-arm] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-21 6:23 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: peter.huangpeng, drjones, shannon.zhao, qemu-devel, david.daney
From: Shannon Zhao <shannon.zhao@linaro.org>
When specifying NUMA for ARM machine, generate /memory node according to
NUMA topology.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 5975fbf..3770235 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -14,6 +14,7 @@
#include "hw/arm/linux-boot-if.h"
#include "sysemu/kvm.h"
#include "sysemu/sysemu.h"
+#include "sysemu/numa.h"
#include "hw/boards.h"
#include "hw/loader.h"
#include "elf.h"
@@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
void *fdt = NULL;
int size, rc;
uint32_t acells, scells;
+ char *nodename;
+ unsigned int i;
+ hwaddr mem_base, mem_len;
if (binfo->dtb_filename) {
char *filename;
@@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
goto fail;
}
+ mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
acells, binfo->loader_start,
- scells, binfo->ram_size);
+ scells, mem_len);
if (rc < 0) {
fprintf(stderr, "couldn't set /memory/reg\n");
goto fail;
}
+ if (nb_numa_nodes > 0) {
+ /* Set the numa-node-id for the first /memory node. */
+ qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
+ /* create /memory node and set properties for other memory numa nodes */
+ mem_base = binfo->loader_start + mem_len;
+ for (i = 1; i < nb_numa_nodes; i++) {
+ mem_len = numa_info[i].node_mem;
+ nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
+ qemu_fdt_add_subnode(fdt, nodename);
+ qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
+ rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
+ acells, mem_base,
+ scells, mem_len);
+ if (rc < 0) {
+ fprintf(stderr, "couldn't set /memory/reg\n");
+ goto fail;
+ }
+
+ qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
+ mem_base += mem_len;
+ g_free(nodename);
+ }
+ }
+
if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
binfo->kernel_cmdline);
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
From: Shannon Zhao <shannon.zhao@linaro.org>
When specifying NUMA for ARM machine, generate /memory node according to
NUMA topology.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 5975fbf..3770235 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -14,6 +14,7 @@
#include "hw/arm/linux-boot-if.h"
#include "sysemu/kvm.h"
#include "sysemu/sysemu.h"
+#include "sysemu/numa.h"
#include "hw/boards.h"
#include "hw/loader.h"
#include "elf.h"
@@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
void *fdt = NULL;
int size, rc;
uint32_t acells, scells;
+ char *nodename;
+ unsigned int i;
+ hwaddr mem_base, mem_len;
if (binfo->dtb_filename) {
char *filename;
@@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
goto fail;
}
+ mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
acells, binfo->loader_start,
- scells, binfo->ram_size);
+ scells, mem_len);
if (rc < 0) {
fprintf(stderr, "couldn't set /memory/reg\n");
goto fail;
}
+ if (nb_numa_nodes > 0) {
+ /* Set the numa-node-id for the first /memory node. */
+ qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
+ /* create /memory node and set properties for other memory numa nodes */
+ mem_base = binfo->loader_start + mem_len;
+ for (i = 1; i < nb_numa_nodes; i++) {
+ mem_len = numa_info[i].node_mem;
+ nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
+ qemu_fdt_add_subnode(fdt, nodename);
+ qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
+ rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
+ acells, mem_base,
+ scells, mem_len);
+ if (rc < 0) {
+ fprintf(stderr, "couldn't set /memory/reg\n");
+ goto fail;
+ }
+
+ qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
+ mem_base += mem_len;
+ g_free(nodename);
+ }
+ }
+
if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
binfo->kernel_cmdline);
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
2016-04-21 6:23 ` [Qemu-devel] " Shannon Zhao
@ 2016-04-22 12:48 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:48 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> When specifying NUMA for ARM machine, generate /memory node according to
> NUMA topology.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 5975fbf..3770235 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -14,6 +14,7 @@
> #include "hw/arm/linux-boot-if.h"
> #include "sysemu/kvm.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/numa.h"
> #include "hw/boards.h"
> #include "hw/loader.h"
> #include "elf.h"
> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> void *fdt = NULL;
> int size, rc;
> uint32_t acells, scells;
> + char *nodename;
> + unsigned int i;
> + hwaddr mem_base, mem_len;
>
> if (binfo->dtb_filename) {
> char *filename;
> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> goto fail;
> }
>
> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
So node0's memory node will still be called '/memory' instead of
'/memory@addr' like the other nodes? Shouldn't we change it too?
> acells, binfo->loader_start,
> - scells, binfo->ram_size);
> + scells, mem_len);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /memory/reg\n");
> goto fail;
> }
>
> + if (nb_numa_nodes > 0) {
> + /* Set the numa-node-id for the first /memory node. */
> + qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
> + /* create /memory node and set properties for other memory numa nodes */
> + mem_base = binfo->loader_start + mem_len;
> + for (i = 1; i < nb_numa_nodes; i++) {
> + mem_len = numa_info[i].node_mem;
> + nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
> + qemu_fdt_add_subnode(fdt, nodename);
> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> + rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
> + acells, mem_base,
> + scells, mem_len);
> + if (rc < 0) {
> + fprintf(stderr, "couldn't set /memory/reg\n");
I'd extend this error message with either the full memory node name,
i.e. '@addr', or something like "couldn't set... for node %d\n"
> + goto fail;
> + }
> +
> + qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
> + mem_base += mem_len;
> + g_free(nodename);
> + }
> + }
> +
> if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
> rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
> binfo->kernel_cmdline);
> --
> 2.0.4
Thanks,
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
@ 2016-04-22 12:48 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 12:48 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, peter.maydell, peter.huangpeng, shannon.zhao,
qemu-devel, david.daney
On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> When specifying NUMA for ARM machine, generate /memory node according to
> NUMA topology.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 5975fbf..3770235 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -14,6 +14,7 @@
> #include "hw/arm/linux-boot-if.h"
> #include "sysemu/kvm.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/numa.h"
> #include "hw/boards.h"
> #include "hw/loader.h"
> #include "elf.h"
> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> void *fdt = NULL;
> int size, rc;
> uint32_t acells, scells;
> + char *nodename;
> + unsigned int i;
> + hwaddr mem_base, mem_len;
>
> if (binfo->dtb_filename) {
> char *filename;
> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> goto fail;
> }
>
> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
So node0's memory node will still be called '/memory' instead of
'/memory@addr' like the other nodes? Shouldn't we change it too?
> acells, binfo->loader_start,
> - scells, binfo->ram_size);
> + scells, mem_len);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /memory/reg\n");
> goto fail;
> }
>
> + if (nb_numa_nodes > 0) {
> + /* Set the numa-node-id for the first /memory node. */
> + qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
> + /* create /memory node and set properties for other memory numa nodes */
> + mem_base = binfo->loader_start + mem_len;
> + for (i = 1; i < nb_numa_nodes; i++) {
> + mem_len = numa_info[i].node_mem;
> + nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
> + qemu_fdt_add_subnode(fdt, nodename);
> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> + rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
> + acells, mem_base,
> + scells, mem_len);
> + if (rc < 0) {
> + fprintf(stderr, "couldn't set /memory/reg\n");
I'd extend this error message with either the full memory node name,
i.e. '@addr', or something like "couldn't set... for node %d\n"
> + goto fail;
> + }
> +
> + qemu_fdt_setprop_cell(fdt, nodename, "numa-node-id", i);
> + mem_base += mem_len;
> + g_free(nodename);
> + }
> + }
> +
> if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
> rc = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",
> binfo->kernel_cmdline);
> --
> 2.0.4
Thanks,
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
2016-04-22 12:48 ` Andrew Jones
@ 2016-04-23 1:16 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:16 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
On 2016/4/22 20:48, Andrew Jones wrote:
> On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> When specifying NUMA for ARM machine, generate /memory node according to
>> NUMA topology.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 5975fbf..3770235 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -14,6 +14,7 @@
>> #include "hw/arm/linux-boot-if.h"
>> #include "sysemu/kvm.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/numa.h"
>> #include "hw/boards.h"
>> #include "hw/loader.h"
>> #include "elf.h"
>> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> void *fdt = NULL;
>> int size, rc;
>> uint32_t acells, scells;
>> + char *nodename;
>> + unsigned int i;
>> + hwaddr mem_base, mem_len;
>>
>> if (binfo->dtb_filename) {
>> char *filename;
>> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> goto fail;
>> }
>>
>> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
>> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
>
> So node0's memory node will still be called '/memory' instead of
> '/memory@addr' like the other nodes? Shouldn't we change it too?
>
Previously I deleted the /memory node creation codes in virt.c and
create here, but that will cause other boards booting fail since
load_dtb() is a common function. So to avoid more changes to other
files, I just use current way. So is there any way to change the node
name after it's created in qemu?
>> acells, binfo->loader_start,
>> - scells, binfo->ram_size);
>> + scells, mem_len);
>> if (rc < 0) {
>> fprintf(stderr, "couldn't set /memory/reg\n");
>> goto fail;
>> }
>>
>> + if (nb_numa_nodes > 0) {
>> + /* Set the numa-node-id for the first /memory node. */
>> + qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
>> + /* create /memory node and set properties for other memory numa nodes */
>> + mem_base = binfo->loader_start + mem_len;
>> + for (i = 1; i < nb_numa_nodes; i++) {
>> + mem_len = numa_info[i].node_mem;
>> + nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
>> + qemu_fdt_add_subnode(fdt, nodename);
>> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>> + rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
>> + acells, mem_base,
>> + scells, mem_len);
>> + if (rc < 0) {
>> + fprintf(stderr, "couldn't set /memory/reg\n");
>
> I'd extend this error message with either the full memory node name,
> i.e. '@addr', or something like "couldn't set... for node %d\n"
>
Sure. Thanks.
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
@ 2016-04-23 1:16 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:16 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-arm, peter.maydell, peter.huangpeng, shannon.zhao,
qemu-devel, david.daney
On 2016/4/22 20:48, Andrew Jones wrote:
> On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> When specifying NUMA for ARM machine, generate /memory node according to
>> NUMA topology.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 5975fbf..3770235 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -14,6 +14,7 @@
>> #include "hw/arm/linux-boot-if.h"
>> #include "sysemu/kvm.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/numa.h"
>> #include "hw/boards.h"
>> #include "hw/loader.h"
>> #include "elf.h"
>> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> void *fdt = NULL;
>> int size, rc;
>> uint32_t acells, scells;
>> + char *nodename;
>> + unsigned int i;
>> + hwaddr mem_base, mem_len;
>>
>> if (binfo->dtb_filename) {
>> char *filename;
>> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>> goto fail;
>> }
>>
>> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
>> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
>
> So node0's memory node will still be called '/memory' instead of
> '/memory@addr' like the other nodes? Shouldn't we change it too?
>
Previously I deleted the /memory node creation codes in virt.c and
create here, but that will cause other boards booting fail since
load_dtb() is a common function. So to avoid more changes to other
files, I just use current way. So is there any way to change the node
name after it's created in qemu?
>> acells, binfo->loader_start,
>> - scells, binfo->ram_size);
>> + scells, mem_len);
>> if (rc < 0) {
>> fprintf(stderr, "couldn't set /memory/reg\n");
>> goto fail;
>> }
>>
>> + if (nb_numa_nodes > 0) {
>> + /* Set the numa-node-id for the first /memory node. */
>> + qemu_fdt_setprop_cell(fdt, "/memory", "numa-node-id", 0);
>> + /* create /memory node and set properties for other memory numa nodes */
>> + mem_base = binfo->loader_start + mem_len;
>> + for (i = 1; i < nb_numa_nodes; i++) {
>> + mem_len = numa_info[i].node_mem;
>> + nodename = g_strdup_printf("/memory@%" PRIx64, mem_base);
>> + qemu_fdt_add_subnode(fdt, nodename);
>> + qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
>> + rc = qemu_fdt_setprop_sized_cells(fdt, nodename, "reg",
>> + acells, mem_base,
>> + scells, mem_len);
>> + if (rc < 0) {
>> + fprintf(stderr, "couldn't set /memory/reg\n");
>
> I'd extend this error message with either the full memory node name,
> i.e. '@addr', or something like "couldn't set... for node %d\n"
>
Sure. Thanks.
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
2016-04-23 1:16 ` Shannon Zhao
@ 2016-04-23 7:45 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-23 7:45 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao
On Sat, Apr 23, 2016 at 09:16:11AM +0800, Shannon Zhao wrote:
>
>
> On 2016/4/22 20:48, Andrew Jones wrote:
> > On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> When specifying NUMA for ARM machine, generate /memory node according to
> >> NUMA topology.
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
> >> 1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> >> index 5975fbf..3770235 100644
> >> --- a/hw/arm/boot.c
> >> +++ b/hw/arm/boot.c
> >> @@ -14,6 +14,7 @@
> >> #include "hw/arm/linux-boot-if.h"
> >> #include "sysemu/kvm.h"
> >> #include "sysemu/sysemu.h"
> >> +#include "sysemu/numa.h"
> >> #include "hw/boards.h"
> >> #include "hw/loader.h"
> >> #include "elf.h"
> >> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >> void *fdt = NULL;
> >> int size, rc;
> >> uint32_t acells, scells;
> >> + char *nodename;
> >> + unsigned int i;
> >> + hwaddr mem_base, mem_len;
> >>
> >> if (binfo->dtb_filename) {
> >> char *filename;
> >> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >> goto fail;
> >> }
> >>
> >> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
> >> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
> >
> > So node0's memory node will still be called '/memory' instead of
> > '/memory@addr' like the other nodes? Shouldn't we change it too?
> >
> Previously I deleted the /memory node creation codes in virt.c and
> create here, but that will cause other boards booting fail since
> load_dtb() is a common function. So to avoid more changes to other
> files, I just use current way. So is there any way to change the node
> name after it's created in qemu?
I'm not sure if that's possible, but we could maybe use qemu_fdt_nop_node
to turn /memory into a NOP node, and then add a new one?
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
@ 2016-04-23 7:45 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-23 7:45 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
On Sat, Apr 23, 2016 at 09:16:11AM +0800, Shannon Zhao wrote:
>
>
> On 2016/4/22 20:48, Andrew Jones wrote:
> > On Thu, Apr 21, 2016 at 02:23:52PM +0800, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> When specifying NUMA for ARM machine, generate /memory node according to
> >> NUMA topology.
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >> hw/arm/boot.c | 31 ++++++++++++++++++++++++++++++-
> >> 1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> >> index 5975fbf..3770235 100644
> >> --- a/hw/arm/boot.c
> >> +++ b/hw/arm/boot.c
> >> @@ -14,6 +14,7 @@
> >> #include "hw/arm/linux-boot-if.h"
> >> #include "sysemu/kvm.h"
> >> #include "sysemu/sysemu.h"
> >> +#include "sysemu/numa.h"
> >> #include "hw/boards.h"
> >> #include "hw/loader.h"
> >> #include "elf.h"
> >> @@ -405,6 +406,9 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >> void *fdt = NULL;
> >> int size, rc;
> >> uint32_t acells, scells;
> >> + char *nodename;
> >> + unsigned int i;
> >> + hwaddr mem_base, mem_len;
> >>
> >> if (binfo->dtb_filename) {
> >> char *filename;
> >> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
> >> goto fail;
> >> }
> >>
> >> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
> >> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
> >
> > So node0's memory node will still be called '/memory' instead of
> > '/memory@addr' like the other nodes? Shouldn't we change it too?
> >
> Previously I deleted the /memory node creation codes in virt.c and
> create here, but that will cause other boards booting fail since
> load_dtb() is a common function. So to avoid more changes to other
> files, I just use current way. So is there any way to change the node
> name after it's created in qemu?
I'm not sure if that's possible, but we could maybe use qemu_fdt_nop_node
to turn /memory into a NOP node, and then add a new one?
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
2016-04-23 7:45 ` Andrew Jones
@ 2016-04-23 8:02 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 8:02 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao
On 2016/4/23 15:45, Andrew Jones wrote:
>>>> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>> > >> goto fail;
>>>> > >> }
>>>> > >>
>>>> > >> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
>>>> > >> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
>>> > >
>>> > > So node0's memory node will still be called '/memory' instead of
>>> > > '/memory@addr' like the other nodes? Shouldn't we change it too?
>>> > >
>> > Previously I deleted the /memory node creation codes in virt.c and
>> > create here, but that will cause other boards booting fail since
>> > load_dtb() is a common function. So to avoid more changes to other
>> > files, I just use current way. So is there any way to change the node
>> > name after it's created in qemu?
> I'm not sure if that's possible, but we could maybe use qemu_fdt_nop_node
> to turn /memory into a NOP node, and then add a new one?
This would be a good solution, I think. I'll update it using
qemu_fdt_nop_node.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node
@ 2016-04-23 8:02 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 8:02 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao
On 2016/4/23 15:45, Andrew Jones wrote:
>>>> @@ -456,14 +460,39 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>>>> > >> goto fail;
>>>> > >> }
>>>> > >>
>>>> > >> + mem_len = (nb_numa_nodes > 0) ? numa_info[0].node_mem : binfo->ram_size;
>>>> > >> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg",
>>> > >
>>> > > So node0's memory node will still be called '/memory' instead of
>>> > > '/memory@addr' like the other nodes? Shouldn't we change it too?
>>> > >
>> > Previously I deleted the /memory node creation codes in virt.c and
>> > create here, but that will cause other boards booting fail since
>> > load_dtb() is a common function. So to avoid more changes to other
>> > files, I just use current way. So is there any way to change the node
>> > name after it's created in qemu?
> I'm not sure if that's possible, but we could maybe use qemu_fdt_nop_node
> to turn /memory into a NOP node, and then add a new one?
This would be a good solution, I think. I'll update it using
qemu_fdt_nop_node.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-21 6:23 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: peter.huangpeng, drjones, shannon.zhao, qemu-devel, david.daney
From: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/i386/acpi-build.c | 2 +-
include/hw/acpi/acpi-defs.h | 15 ++++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6477003..9ae4c0d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2474,7 +2474,7 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
int apic_id = apic_ids->cpus[i].arch_id;
core = acpi_data_push(table_data, sizeof *core);
- core->type = ACPI_SRAT_PROCESSOR;
+ core->type = ACPI_SRAT_PROCESSOR_APIC;
core->length = sizeof(*core);
core->local_apic_id = apic_id;
curnode = pcms->node_cpu[apic_id];
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c7a03d4..bcf5c3f 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -455,8 +455,10 @@ struct AcpiSystemResourceAffinityTable
} QEMU_PACKED;
typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
-#define ACPI_SRAT_PROCESSOR 0
+#define ACPI_SRAT_PROCESSOR_APIC 0
#define ACPI_SRAT_MEMORY 1
+#define ACPI_SRAT_PROCESSOR_x2APIC 2
+#define ACPI_SRAT_PROCESSOR_GICC 3
struct AcpiSratProcessorAffinity
{
@@ -483,6 +485,17 @@ struct AcpiSratMemoryAffinity
} QEMU_PACKED;
typedef struct AcpiSratMemoryAffinity AcpiSratMemoryAffinity;
+struct AcpiSratProcessorGiccAffinity
+{
+ ACPI_SUB_HEADER_DEF
+ uint32_t proximity;
+ uint32_t acpi_processor_uid;
+ uint32_t flags;
+ uint32_t clock_domain;
+} QEMU_PACKED;
+
+typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity;
+
/* PCI fw r3.0 MCFG table. */
/* Subtable */
struct AcpiMcfgAllocation {
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
From: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/i386/acpi-build.c | 2 +-
include/hw/acpi/acpi-defs.h | 15 ++++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6477003..9ae4c0d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2474,7 +2474,7 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
int apic_id = apic_ids->cpus[i].arch_id;
core = acpi_data_push(table_data, sizeof *core);
- core->type = ACPI_SRAT_PROCESSOR;
+ core->type = ACPI_SRAT_PROCESSOR_APIC;
core->length = sizeof(*core);
core->local_apic_id = apic_id;
curnode = pcms->node_cpu[apic_id];
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c7a03d4..bcf5c3f 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -455,8 +455,10 @@ struct AcpiSystemResourceAffinityTable
} QEMU_PACKED;
typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
-#define ACPI_SRAT_PROCESSOR 0
+#define ACPI_SRAT_PROCESSOR_APIC 0
#define ACPI_SRAT_MEMORY 1
+#define ACPI_SRAT_PROCESSOR_x2APIC 2
+#define ACPI_SRAT_PROCESSOR_GICC 3
struct AcpiSratProcessorAffinity
{
@@ -483,6 +485,17 @@ struct AcpiSratMemoryAffinity
} QEMU_PACKED;
typedef struct AcpiSratMemoryAffinity AcpiSratMemoryAffinity;
+struct AcpiSratProcessorGiccAffinity
+{
+ ACPI_SUB_HEADER_DEF
+ uint32_t proximity;
+ uint32_t acpi_processor_uid;
+ uint32_t flags;
+ uint32_t clock_domain;
+} QEMU_PACKED;
+
+typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity;
+
/* PCI fw r3.0 MCFG table. */
/* Subtable */
struct AcpiMcfgAllocation {
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-22 13:26 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 13:26 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao
On Thu, Apr 21, 2016 at 02:23:53PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/i386/acpi-build.c | 2 +-
> include/hw/acpi/acpi-defs.h | 15 ++++++++++++++-
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6477003..9ae4c0d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2474,7 +2474,7 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
> int apic_id = apic_ids->cpus[i].arch_id;
>
> core = acpi_data_push(table_data, sizeof *core);
> - core->type = ACPI_SRAT_PROCESSOR;
> + core->type = ACPI_SRAT_PROCESSOR_APIC;
> core->length = sizeof(*core);
> core->local_apic_id = apic_id;
> curnode = pcms->node_cpu[apic_id];
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index c7a03d4..bcf5c3f 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -455,8 +455,10 @@ struct AcpiSystemResourceAffinityTable
> } QEMU_PACKED;
> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>
> -#define ACPI_SRAT_PROCESSOR 0
> +#define ACPI_SRAT_PROCESSOR_APIC 0
> #define ACPI_SRAT_MEMORY 1
> +#define ACPI_SRAT_PROCESSOR_x2APIC 2
> +#define ACPI_SRAT_PROCESSOR_GICC 3
>
> struct AcpiSratProcessorAffinity
> {
> @@ -483,6 +485,17 @@ struct AcpiSratMemoryAffinity
> } QEMU_PACKED;
> typedef struct AcpiSratMemoryAffinity AcpiSratMemoryAffinity;
>
> +struct AcpiSratProcessorGiccAffinity
> +{
> + ACPI_SUB_HEADER_DEF
> + uint32_t proximity;
> + uint32_t acpi_processor_uid;
> + uint32_t flags;
> + uint32_t clock_domain;
> +} QEMU_PACKED;
> +
> +typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity;
> +
> /* PCI fw r3.0 MCFG table. */
> /* Subtable */
> struct AcpiMcfgAllocation {
> --
> 2.0.4
>
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure
@ 2016-04-22 13:26 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 13:26 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, peter.maydell, qemu-devel, david.daney, peter.huangpeng,
shannon.zhao
On Thu, Apr 21, 2016 at 02:23:53PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/i386/acpi-build.c | 2 +-
> include/hw/acpi/acpi-defs.h | 15 ++++++++++++++-
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6477003..9ae4c0d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2474,7 +2474,7 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
> int apic_id = apic_ids->cpus[i].arch_id;
>
> core = acpi_data_push(table_data, sizeof *core);
> - core->type = ACPI_SRAT_PROCESSOR;
> + core->type = ACPI_SRAT_PROCESSOR_APIC;
> core->length = sizeof(*core);
> core->local_apic_id = apic_id;
> curnode = pcms->node_cpu[apic_id];
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index c7a03d4..bcf5c3f 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -455,8 +455,10 @@ struct AcpiSystemResourceAffinityTable
> } QEMU_PACKED;
> typedef struct AcpiSystemResourceAffinityTable AcpiSystemResourceAffinityTable;
>
> -#define ACPI_SRAT_PROCESSOR 0
> +#define ACPI_SRAT_PROCESSOR_APIC 0
> #define ACPI_SRAT_MEMORY 1
> +#define ACPI_SRAT_PROCESSOR_x2APIC 2
> +#define ACPI_SRAT_PROCESSOR_GICC 3
>
> struct AcpiSratProcessorAffinity
> {
> @@ -483,6 +485,17 @@ struct AcpiSratMemoryAffinity
> } QEMU_PACKED;
> typedef struct AcpiSratMemoryAffinity AcpiSratMemoryAffinity;
>
> +struct AcpiSratProcessorGiccAffinity
> +{
> + ACPI_SUB_HEADER_DEF
> + uint32_t proximity;
> + uint32_t acpi_processor_uid;
> + uint32_t flags;
> + uint32_t clock_domain;
> +} QEMU_PACKED;
> +
> +typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity;
> +
> /* PCI fw r3.0 MCFG table. */
> /* Subtable */
> struct AcpiMcfgAllocation {
> --
> 2.0.4
>
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-arm] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
2016-04-21 6:23 ` Shannon Zhao
@ 2016-04-21 6:23 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: peter.huangpeng, drjones, shannon.zhao, qemu-devel, david.daney
From: Shannon Zhao <shannon.zhao@linaro.org>
To support NUMA, it needs to generate SRAT ACPI table.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f51fe39..a618210 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -43,6 +43,7 @@
#include "hw/acpi/aml-build.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
+#include "sysemu/numa.h"
#define ARM_SPI_BASE 32
#define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
}
static void
+build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+ AcpiSystemResourceAffinityTable *srat;
+ AcpiSratProcessorGiccAffinity *core;
+ AcpiSratMemoryAffinity *numamem;
+ int i, j, srat_start;
+ uint64_t mem_len, mem_base;
+ uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
+
+ for (i = 0; i < guest_info->smp_cpus; i++) {
+ for (j = 0; j < nb_numa_nodes; j++) {
+ if (test_bit(i, numa_info[j].node_cpu)) {
+ cpu_node[i] = j;
+ break;
+ }
+ }
+ }
+
+ srat_start = table_data->len;
+ srat = acpi_data_push(table_data, sizeof *srat);
+ srat->reserved1 = cpu_to_le32(1);
+
+ for (i = 0; i < guest_info->smp_cpus; ++i) {
+ core = acpi_data_push(table_data, sizeof *core);
+ core->type = ACPI_SRAT_PROCESSOR_GICC;
+ core->length = sizeof(*core);
+ core->proximity = cpu_node[i];
+ core->acpi_processor_uid = i;
+ core->flags = cpu_to_le32(1);
+ }
+ g_free(cpu_node);
+
+ mem_base = guest_info->memmap[VIRT_MEM].base;
+ for (i = 0; i < nb_numa_nodes; ++i) {
+ mem_len = numa_info[i].node_mem;
+ numamem = acpi_data_push(table_data, sizeof *numamem);
+ numamem->type = ACPI_SRAT_MEMORY;
+ numamem->length = sizeof(*numamem);
+ memset(numamem->proximity, 0, 4);
+ numamem->proximity[0] = i;
+ numamem->flags = cpu_to_le32(1);
+ numamem->base_addr = cpu_to_le64(mem_base);
+ numamem->range_length = cpu_to_le64(mem_len);
+ mem_base += mem_len;
+ }
+
+ build_header(linker, table_data,
+ (void *)(table_data->data + srat_start), "SRAT",
+ table_data->len - srat_start, 3, NULL, NULL);
+}
+
+static void
build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
{
AcpiTableMcfg *mcfg;
@@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_spcr(tables_blob, tables->linker, guest_info);
+ if (nb_numa_nodes > 0) {
+ acpi_add_table(table_offsets, tables_blob);
+ build_srat(tables_blob, tables->linker, guest_info);
+ }
+
/* RSDT is pointed to by RSDP */
rsdt = tables_blob->len;
build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
@ 2016-04-21 6:23 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-21 6:23 UTC (permalink / raw)
To: qemu-arm, peter.maydell
Cc: qemu-devel, drjones, david.daney, peter.huangpeng, shannon.zhao
From: Shannon Zhao <shannon.zhao@linaro.org>
To support NUMA, it needs to generate SRAT ACPI table.
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f51fe39..a618210 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -43,6 +43,7 @@
#include "hw/acpi/aml-build.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
+#include "sysemu/numa.h"
#define ARM_SPI_BASE 32
#define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
}
static void
+build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+ AcpiSystemResourceAffinityTable *srat;
+ AcpiSratProcessorGiccAffinity *core;
+ AcpiSratMemoryAffinity *numamem;
+ int i, j, srat_start;
+ uint64_t mem_len, mem_base;
+ uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
+
+ for (i = 0; i < guest_info->smp_cpus; i++) {
+ for (j = 0; j < nb_numa_nodes; j++) {
+ if (test_bit(i, numa_info[j].node_cpu)) {
+ cpu_node[i] = j;
+ break;
+ }
+ }
+ }
+
+ srat_start = table_data->len;
+ srat = acpi_data_push(table_data, sizeof *srat);
+ srat->reserved1 = cpu_to_le32(1);
+
+ for (i = 0; i < guest_info->smp_cpus; ++i) {
+ core = acpi_data_push(table_data, sizeof *core);
+ core->type = ACPI_SRAT_PROCESSOR_GICC;
+ core->length = sizeof(*core);
+ core->proximity = cpu_node[i];
+ core->acpi_processor_uid = i;
+ core->flags = cpu_to_le32(1);
+ }
+ g_free(cpu_node);
+
+ mem_base = guest_info->memmap[VIRT_MEM].base;
+ for (i = 0; i < nb_numa_nodes; ++i) {
+ mem_len = numa_info[i].node_mem;
+ numamem = acpi_data_push(table_data, sizeof *numamem);
+ numamem->type = ACPI_SRAT_MEMORY;
+ numamem->length = sizeof(*numamem);
+ memset(numamem->proximity, 0, 4);
+ numamem->proximity[0] = i;
+ numamem->flags = cpu_to_le32(1);
+ numamem->base_addr = cpu_to_le64(mem_base);
+ numamem->range_length = cpu_to_le64(mem_len);
+ mem_base += mem_len;
+ }
+
+ build_header(linker, table_data,
+ (void *)(table_data->data + srat_start), "SRAT",
+ table_data->len - srat_start, 3, NULL, NULL);
+}
+
+static void
build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
{
AcpiTableMcfg *mcfg;
@@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_spcr(tables_blob, tables->linker, guest_info);
+ if (nb_numa_nodes > 0) {
+ acpi_add_table(table_offsets, tables_blob);
+ build_srat(tables_blob, tables->linker, guest_info);
+ }
+
/* RSDT is pointed to by RSDP */
rsdt = tables_blob->len;
build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
--
2.0.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
2016-04-21 6:23 ` [Qemu-devel] " Shannon Zhao
@ 2016-04-22 13:26 ` Andrew Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 13:26 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao, imammedo
On Thu, Apr 21, 2016 at 02:23:54PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> To support NUMA, it needs to generate SRAT ACPI table.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f51fe39..a618210 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -43,6 +43,7 @@
> #include "hw/acpi/aml-build.h"
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> +#include "sysemu/numa.h"
>
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> }
>
> static void
> +build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> + AcpiSystemResourceAffinityTable *srat;
> + AcpiSratProcessorGiccAffinity *core;
> + AcpiSratMemoryAffinity *numamem;
> + int i, j, srat_start;
> + uint64_t mem_len, mem_base;
> + uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
nit1: the majority of qemu code uses () with sizeof
nit2: sizeof(uint32_t) might be nicer than the cyclic reference
> +
> + for (i = 0; i < guest_info->smp_cpus; i++) {
> + for (j = 0; j < nb_numa_nodes; j++) {
> + if (test_bit(i, numa_info[j].node_cpu)) {
> + cpu_node[i] = j;
> + break;
> + }
> + }
> + }
> +
> + srat_start = table_data->len;
> + srat = acpi_data_push(table_data, sizeof *srat);
> + srat->reserved1 = cpu_to_le32(1);
> +
> + for (i = 0; i < guest_info->smp_cpus; ++i) {
> + core = acpi_data_push(table_data, sizeof *core);
> + core->type = ACPI_SRAT_PROCESSOR_GICC;
> + core->length = sizeof(*core);
> + core->proximity = cpu_node[i];
> + core->acpi_processor_uid = i;
We'll want to use get_arch_id() here (after I implement it as part of
the cpu topology work I keep promising).
> + core->flags = cpu_to_le32(1);
> + }
> + g_free(cpu_node);
> +
> + mem_base = guest_info->memmap[VIRT_MEM].base;
> + for (i = 0; i < nb_numa_nodes; ++i) {
> + mem_len = numa_info[i].node_mem;
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + numamem->type = ACPI_SRAT_MEMORY;
> + numamem->length = sizeof(*numamem);
> + memset(numamem->proximity, 0, 4);
> + numamem->proximity[0] = i;
This is weird (but I see x86 does it too). The spec says proximity is
"Integer that represents the proximity domain to which the processor
belongs", and its 4 bytes. So why doesn't the structure define it as
a uint32_t and then we'd just do
numamem->proximity = cpu_to_le32(i);
(adding Igor)
> + numamem->flags = cpu_to_le32(1);
> + numamem->base_addr = cpu_to_le64(mem_base);
> + numamem->range_length = cpu_to_le64(mem_len);
How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
somewhere in hw/acpi and reusing it?
> + mem_base += mem_len;
> + }
> +
> + build_header(linker, table_data,
> + (void *)(table_data->data + srat_start), "SRAT",
> + table_data->len - srat_start, 3, NULL, NULL);
> +}
> +
> +static void
> build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> {
> AcpiTableMcfg *mcfg;
> @@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> acpi_add_table(table_offsets, tables_blob);
> build_spcr(tables_blob, tables->linker, guest_info);
>
> + if (nb_numa_nodes > 0) {
> + acpi_add_table(table_offsets, tables_blob);
> + build_srat(tables_blob, tables->linker, guest_info);
> + }
> +
> /* RSDT is pointed to by RSDP */
> rsdt = tables_blob->len;
> build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> --
> 2.0.4
>
>
Thanks,
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
@ 2016-04-22 13:26 ` Andrew Jones
0 siblings, 0 replies; 38+ messages in thread
From: Andrew Jones @ 2016-04-22 13:26 UTC (permalink / raw)
To: Shannon Zhao
Cc: qemu-arm, peter.maydell, qemu-devel, david.daney, peter.huangpeng,
shannon.zhao, imammedo
On Thu, Apr 21, 2016 at 02:23:54PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> To support NUMA, it needs to generate SRAT ACPI table.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f51fe39..a618210 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -43,6 +43,7 @@
> #include "hw/acpi/aml-build.h"
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> +#include "sysemu/numa.h"
>
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> }
>
> static void
> +build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> + AcpiSystemResourceAffinityTable *srat;
> + AcpiSratProcessorGiccAffinity *core;
> + AcpiSratMemoryAffinity *numamem;
> + int i, j, srat_start;
> + uint64_t mem_len, mem_base;
> + uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
nit1: the majority of qemu code uses () with sizeof
nit2: sizeof(uint32_t) might be nicer than the cyclic reference
> +
> + for (i = 0; i < guest_info->smp_cpus; i++) {
> + for (j = 0; j < nb_numa_nodes; j++) {
> + if (test_bit(i, numa_info[j].node_cpu)) {
> + cpu_node[i] = j;
> + break;
> + }
> + }
> + }
> +
> + srat_start = table_data->len;
> + srat = acpi_data_push(table_data, sizeof *srat);
> + srat->reserved1 = cpu_to_le32(1);
> +
> + for (i = 0; i < guest_info->smp_cpus; ++i) {
> + core = acpi_data_push(table_data, sizeof *core);
> + core->type = ACPI_SRAT_PROCESSOR_GICC;
> + core->length = sizeof(*core);
> + core->proximity = cpu_node[i];
> + core->acpi_processor_uid = i;
We'll want to use get_arch_id() here (after I implement it as part of
the cpu topology work I keep promising).
> + core->flags = cpu_to_le32(1);
> + }
> + g_free(cpu_node);
> +
> + mem_base = guest_info->memmap[VIRT_MEM].base;
> + for (i = 0; i < nb_numa_nodes; ++i) {
> + mem_len = numa_info[i].node_mem;
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + numamem->type = ACPI_SRAT_MEMORY;
> + numamem->length = sizeof(*numamem);
> + memset(numamem->proximity, 0, 4);
> + numamem->proximity[0] = i;
This is weird (but I see x86 does it too). The spec says proximity is
"Integer that represents the proximity domain to which the processor
belongs", and its 4 bytes. So why doesn't the structure define it as
a uint32_t and then we'd just do
numamem->proximity = cpu_to_le32(i);
(adding Igor)
> + numamem->flags = cpu_to_le32(1);
> + numamem->base_addr = cpu_to_le64(mem_base);
> + numamem->range_length = cpu_to_le64(mem_len);
How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
somewhere in hw/acpi and reusing it?
> + mem_base += mem_len;
> + }
> +
> + build_header(linker, table_data,
> + (void *)(table_data->data + srat_start), "SRAT",
> + table_data->len - srat_start, 3, NULL, NULL);
> +}
> +
> +static void
> build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> {
> AcpiTableMcfg *mcfg;
> @@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> acpi_add_table(table_offsets, tables_blob);
> build_spcr(tables_blob, tables->linker, guest_info);
>
> + if (nb_numa_nodes > 0) {
> + acpi_add_table(table_offsets, tables_blob);
> + build_srat(tables_blob, tables->linker, guest_info);
> + }
> +
> /* RSDT is pointed to by RSDP */
> rsdt = tables_blob->len;
> build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> --
> 2.0.4
>
>
Thanks,
drew
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-arm] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
2016-04-22 13:26 ` [Qemu-devel] " Andrew Jones
@ 2016-04-23 1:08 ` Shannon Zhao
-1 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:08 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, peter.huangpeng, qemu-devel, qemu-arm,
shannon.zhao, imammedo
On 2016/4/22 21:26, Andrew Jones wrote:
>> + core->flags = cpu_to_le32(1);
>> > + }
>> > + g_free(cpu_node);
>> > +
>> > + mem_base = guest_info->memmap[VIRT_MEM].base;
>> > + for (i = 0; i < nb_numa_nodes; ++i) {
>> > + mem_len = numa_info[i].node_mem;
>> > + numamem = acpi_data_push(table_data, sizeof *numamem);
>> > + numamem->type = ACPI_SRAT_MEMORY;
>> > + numamem->length = sizeof(*numamem);
>> > + memset(numamem->proximity, 0, 4);
>> > + numamem->proximity[0] = i;
> This is weird (but I see x86 does it too). The spec says proximity is
> "Integer that represents the proximity domain to which the processor
> belongs", and its 4 bytes. So why doesn't the structure define it as
> a uint32_t and then we'd just do
>
> numamem->proximity = cpu_to_le32(i);
>
> (adding Igor)
>
>> > + numamem->flags = cpu_to_le32(1);
>> > + numamem->base_addr = cpu_to_le64(mem_base);
>> > + numamem->range_length = cpu_to_le64(mem_len);
> How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
> somewhere in hw/acpi and reusing it?
>
Good point! Will do that.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
@ 2016-04-23 1:08 ` Shannon Zhao
0 siblings, 0 replies; 38+ messages in thread
From: Shannon Zhao @ 2016-04-23 1:08 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-arm, peter.maydell, qemu-devel, david.daney, peter.huangpeng,
shannon.zhao, imammedo
On 2016/4/22 21:26, Andrew Jones wrote:
>> + core->flags = cpu_to_le32(1);
>> > + }
>> > + g_free(cpu_node);
>> > +
>> > + mem_base = guest_info->memmap[VIRT_MEM].base;
>> > + for (i = 0; i < nb_numa_nodes; ++i) {
>> > + mem_len = numa_info[i].node_mem;
>> > + numamem = acpi_data_push(table_data, sizeof *numamem);
>> > + numamem->type = ACPI_SRAT_MEMORY;
>> > + numamem->length = sizeof(*numamem);
>> > + memset(numamem->proximity, 0, 4);
>> > + numamem->proximity[0] = i;
> This is weird (but I see x86 does it too). The spec says proximity is
> "Integer that represents the proximity domain to which the processor
> belongs", and its 4 bytes. So why doesn't the structure define it as
> a uint32_t and then we'd just do
>
> numamem->proximity = cpu_to_le32(i);
>
> (adding Igor)
>
>> > + numamem->flags = cpu_to_le32(1);
>> > + numamem->base_addr = cpu_to_le64(mem_base);
>> > + numamem->range_length = cpu_to_le64(mem_len);
> How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
> somewhere in hw/acpi and reusing it?
>
Good point! Will do that.
Thanks,
--
Shannon
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
2016-04-22 13:26 ` [Qemu-devel] " Andrew Jones
@ 2016-04-25 10:44 ` Igor Mammedov
-1 siblings, 0 replies; 38+ messages in thread
From: Igor Mammedov @ 2016-04-25 10:44 UTC (permalink / raw)
To: Andrew Jones
Cc: peter.maydell, david.daney, qemu-devel, peter.huangpeng, qemu-arm,
shannon.zhao, Shannon Zhao
On Fri, 22 Apr 2016 15:26:05 +0200
Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Apr 21, 2016 at 02:23:54PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >
> > To support NUMA, it needs to generate SRAT ACPI table.
> >
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> > hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f51fe39..a618210 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -43,6 +43,7 @@
> > #include "hw/acpi/aml-build.h"
> > #include "hw/pci/pcie_host.h"
> > #include "hw/pci/pci.h"
> > +#include "sysemu/numa.h"
> >
> > #define ARM_SPI_BASE 32
> > #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> > @@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > }
> >
> > static void
> > +build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > +{
> > + AcpiSystemResourceAffinityTable *srat;
> > + AcpiSratProcessorGiccAffinity *core;
> > + AcpiSratMemoryAffinity *numamem;
> > + int i, j, srat_start;
> > + uint64_t mem_len, mem_base;
> > + uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
>
> nit1: the majority of qemu code uses () with sizeof
> nit2: sizeof(uint32_t) might be nicer than the cyclic reference
>
> > +
> > + for (i = 0; i < guest_info->smp_cpus; i++) {
> > + for (j = 0; j < nb_numa_nodes; j++) {
> > + if (test_bit(i, numa_info[j].node_cpu)) {
> > + cpu_node[i] = j;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + srat_start = table_data->len;
> > + srat = acpi_data_push(table_data, sizeof *srat);
> > + srat->reserved1 = cpu_to_le32(1);
> > +
> > + for (i = 0; i < guest_info->smp_cpus; ++i) {
> > + core = acpi_data_push(table_data, sizeof *core);
> > + core->type = ACPI_SRAT_PROCESSOR_GICC;
> > + core->length = sizeof(*core);
> > + core->proximity = cpu_node[i];
> > + core->acpi_processor_uid = i;
>
> We'll want to use get_arch_id() here (after I implement it as part of
> the cpu topology work I keep promising).
>
> > + core->flags = cpu_to_le32(1);
> > + }
> > + g_free(cpu_node);
> > +
> > + mem_base = guest_info->memmap[VIRT_MEM].base;
> > + for (i = 0; i < nb_numa_nodes; ++i) {
> > + mem_len = numa_info[i].node_mem;
> > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > + numamem->type = ACPI_SRAT_MEMORY;
> > + numamem->length = sizeof(*numamem);
> > + memset(numamem->proximity, 0, 4);
> > + numamem->proximity[0] = i;
>
> This is weird (but I see x86 does it too). The spec says proximity is
> "Integer that represents the proximity domain to which the processor
> belongs", and its 4 bytes. So why doesn't the structure define it as
> a uint32_t and then we'd just do
>
> numamem->proximity = cpu_to_le32(i);
It's probably just some legacy code and should be fixed as you're suggesting.
>
> (adding Igor)
>
> > + numamem->flags = cpu_to_le32(1);
> > + numamem->base_addr = cpu_to_le64(mem_base);
> > + numamem->range_length = cpu_to_le64(mem_len);
>
> How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
> somewhere in hw/acpi and reusing it?
>
> > + mem_base += mem_len;
> > + }
> > +
> > + build_header(linker, table_data,
> > + (void *)(table_data->data + srat_start), "SRAT",
> > + table_data->len - srat_start, 3, NULL, NULL);
> > +}
> > +
> > +static void
> > build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > {
> > AcpiTableMcfg *mcfg;
> > @@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> > acpi_add_table(table_offsets, tables_blob);
> > build_spcr(tables_blob, tables->linker, guest_info);
> >
> > + if (nb_numa_nodes > 0) {
> > + acpi_add_table(table_offsets, tables_blob);
> > + build_srat(tables_blob, tables->linker, guest_info);
> > + }
> > +
> > /* RSDT is pointed to by RSDP */
> > rsdt = tables_blob->len;
> > build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > --
> > 2.0.4
> >
> >
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table
@ 2016-04-25 10:44 ` Igor Mammedov
0 siblings, 0 replies; 38+ messages in thread
From: Igor Mammedov @ 2016-04-25 10:44 UTC (permalink / raw)
To: Andrew Jones
Cc: Shannon Zhao, peter.maydell, david.daney, peter.huangpeng,
qemu-devel, qemu-arm, shannon.zhao
On Fri, 22 Apr 2016 15:26:05 +0200
Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Apr 21, 2016 at 02:23:54PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >
> > To support NUMA, it needs to generate SRAT ACPI table.
> >
> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> > hw/arm/virt-acpi-build.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index f51fe39..a618210 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -43,6 +43,7 @@
> > #include "hw/acpi/aml-build.h"
> > #include "hw/pci/pcie_host.h"
> > #include "hw/pci/pci.h"
> > +#include "sysemu/numa.h"
> >
> > #define ARM_SPI_BASE 32
> > #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> > @@ -414,6 +415,58 @@ build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > }
> >
> > static void
> > +build_srat(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > +{
> > + AcpiSystemResourceAffinityTable *srat;
> > + AcpiSratProcessorGiccAffinity *core;
> > + AcpiSratMemoryAffinity *numamem;
> > + int i, j, srat_start;
> > + uint64_t mem_len, mem_base;
> > + uint32_t *cpu_node = g_malloc0(guest_info->smp_cpus * sizeof *cpu_node);
>
> nit1: the majority of qemu code uses () with sizeof
> nit2: sizeof(uint32_t) might be nicer than the cyclic reference
>
> > +
> > + for (i = 0; i < guest_info->smp_cpus; i++) {
> > + for (j = 0; j < nb_numa_nodes; j++) {
> > + if (test_bit(i, numa_info[j].node_cpu)) {
> > + cpu_node[i] = j;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + srat_start = table_data->len;
> > + srat = acpi_data_push(table_data, sizeof *srat);
> > + srat->reserved1 = cpu_to_le32(1);
> > +
> > + for (i = 0; i < guest_info->smp_cpus; ++i) {
> > + core = acpi_data_push(table_data, sizeof *core);
> > + core->type = ACPI_SRAT_PROCESSOR_GICC;
> > + core->length = sizeof(*core);
> > + core->proximity = cpu_node[i];
> > + core->acpi_processor_uid = i;
>
> We'll want to use get_arch_id() here (after I implement it as part of
> the cpu topology work I keep promising).
>
> > + core->flags = cpu_to_le32(1);
> > + }
> > + g_free(cpu_node);
> > +
> > + mem_base = guest_info->memmap[VIRT_MEM].base;
> > + for (i = 0; i < nb_numa_nodes; ++i) {
> > + mem_len = numa_info[i].node_mem;
> > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > + numamem->type = ACPI_SRAT_MEMORY;
> > + numamem->length = sizeof(*numamem);
> > + memset(numamem->proximity, 0, 4);
> > + numamem->proximity[0] = i;
>
> This is weird (but I see x86 does it too). The spec says proximity is
> "Integer that represents the proximity domain to which the processor
> belongs", and its 4 bytes. So why doesn't the structure define it as
> a uint32_t and then we'd just do
>
> numamem->proximity = cpu_to_le32(i);
It's probably just some legacy code and should be fixed as you're suggesting.
>
> (adding Igor)
>
> > + numamem->flags = cpu_to_le32(1);
> > + numamem->base_addr = cpu_to_le64(mem_base);
> > + numamem->range_length = cpu_to_le64(mem_len);
>
> How about moving acpi_build_srat_memory from hw/i386/acpi-build.c to
> somewhere in hw/acpi and reusing it?
>
> > + mem_base += mem_len;
> > + }
> > +
> > + build_header(linker, table_data,
> > + (void *)(table_data->data + srat_start), "SRAT",
> > + table_data->len - srat_start, 3, NULL, NULL);
> > +}
> > +
> > +static void
> > build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > {
> > AcpiTableMcfg *mcfg;
> > @@ -638,6 +691,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> > acpi_add_table(table_offsets, tables_blob);
> > build_spcr(tables_blob, tables->linker, guest_info);
> >
> > + if (nb_numa_nodes > 0) {
> > + acpi_add_table(table_offsets, tables_blob);
> > + build_srat(tables_blob, tables->linker, guest_info);
> > + }
> > +
> > /* RSDT is pointed to by RSDP */
> > rsdt = tables_blob->len;
> > build_rsdt(tables_blob, tables->linker, table_offsets, NULL, NULL);
> > --
> > 2.0.4
> >
> >
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 38+ messages in thread