[Qemu-devel] [PATCH v5 0/5] ARM: Add NUMA support for machine virt

[Qemu-devel] [PATCH v5 0/5] ARM: Add NUMA support for machine virt

[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

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

[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

[Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs

[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

[Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node

[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

[Qemu-devel] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure

[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

[Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table

[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

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

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH v5 2/5] ARM: Virt: Set numa-node-id for CPUs

[Andrew Jones]

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>

Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table

[Andrew Jones]

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 

Re: [Qemu-devel] [PATCH v5 4/5] include/hw/acpi/acpi-defs: Add GICC Affinity Structure

[Andrew Jones]

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> 

Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table

[Shannon Zhao]

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

Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node

[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

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

[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

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

[Andrew Jones]

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
> 
> 

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

[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

Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH v5 3/5] ARM: Add numa-node-id for /memory node

[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

Re: [Qemu-devel] [PATCH v5 5/5] hw/arm/virt-acpi-build: Generate SRAT table

[Igor Mammedov]

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 
>