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 <address@hidden> wrote:

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