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 <address@hidden>
> 
> To support NUMA, it needs to generate SRAT ACPI table.
> 
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
>  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