diff mbox

[v5,5/5] hw/arm/virt-acpi-build: Generate SRAT table

Message ID 1461219834-10416-6-git-send-email-zhaoshenglong@huawei.com
State New
Headers show

Commit Message

Shannon Zhao April 21, 2016, 6:23 a.m. UTC 
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(+)

Comments

Andrew Jones April 22, 2016, 1:26 p.m. UTC | #1 
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
Shannon Zhao April 23, 2016, 1:08 a.m. UTC | #2 
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,
Igor Mammedov April 25, 2016, 10:44 a.m. UTC | #3 
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 
>
diff mbox

Patch

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