Re: [Xen-devel] [RFC PATCH v1 13/21] ACPI: Refactor acpi SRAT and SLIT table handling code

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Mar 2, 2017 at 9:00 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
> On 09/02/17 15:57, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> Move SRAT handling code which is common across
>> architecture is moved to new file xen/commom/srat.c
>> from xen/arch/x86/srat.c file. New header file srat.h is
>> introduced.
>>
>> Signed-off-by: Vijaya Kumar <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/domain_build.c         |   1 +
>>  xen/arch/x86/numa.c                 |   1 +
>>  xen/arch/x86/physdev.c              |   1 +
>>  xen/arch/x86/setup.c                |   1 +
>>  xen/arch/x86/smpboot.c              |   1 +
>>  xen/arch/x86/srat.c                 | 129 +------------------------------
>>  xen/arch/x86/x86_64/mm.c            |   1 +
>>  xen/common/Makefile                 |   1 +
>>  xen/common/srat.c                   | 150
>> ++++++++++++++++++++++++++++++++++++
>
>
> This new file should be created in xen/drivers/acpi/
OK
>
>>  xen/drivers/passthrough/vtd/iommu.c |   1 +
>>  xen/include/asm-x86/numa.h          |   2 -
>>  xen/include/xen/numa.h              |   1 -
>>  xen/include/xen/srat.h              |  13 ++++
>
>
> This new file should be created in xen/include/acpi/
OK
>
> [...]
>
>> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
>> index 58dee09..af12e26 100644
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -18,91 +18,20 @@
>>  #include <xen/acpi.h>
>>  #include <xen/numa.h>
>>  #include <xen/pfn.h>
>> +#include <xen/srat.h>
>>  #include <asm/e820.h>
>>  #include <asm/page.h>
>>
>> -static struct acpi_table_slit *__read_mostly acpi_slit;
>> +extern struct acpi_table_slit *__read_mostly acpi_slit;
>
>
> This should be defined in the header. However, I don't like the idea of
> exposing acpi_slit.
>
> Looking at the usage it is only to parse the distance that can be common.

node_distance() of x86 and arm is quite different as arm has DT mechanism.
I will check if possible to make ACPI node_distance part common between
x86 and arm.

>
> [...]
>
>>  /* Callback for Proximity Domain -> x2APIC mapping */
>>  void __init
>>  acpi_numa_x2apic_affinity_init(const struct acpi_srat_x2apic_cpu_affinity
>> *pa)
>> @@ -456,18 +343,6 @@ int __init acpi_scan_nodes(u64 start, u64 end)
>>         return 0;
>>  }
>
>
> The code of acpi_numa_memory_affinity_init looks pretty generic. Why didn't
> you move it in the common code?

Agreed.
>
> [...]
>
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index c1bd2ff..a668094 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -64,6 +64,7 @@ obj-bin-y += warning.init.o
>>  obj-$(CONFIG_XENOPROF) += xenoprof.o
>>  obj-y += xmalloc_tlsf.o
>>  obj-y += numa.o
>> +obj-y += srat.o
>
>
> This should be only compiled when CONFIG_ACPI is enabled.
OK
>
>
>>
>>  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo
>> unlz4 earlycpio,$(n).init.o)
>>
>> diff --git a/xen/common/srat.c b/xen/common/srat.c
>> new file mode 100644
>> index 0000000..cf50c78
>> --- /dev/null
>> +++ b/xen/common/srat.c
>> @@ -0,0 +1,150 @@
>> +/*
>> + * ACPI 3.0 based NUMA setup
>> + * Copyright 2004 Andi Kleen, SuSE Labs.
>> + *
>> + * Reads the ACPI SRAT table to figure out what memory belongs to which
>> CPUs.
>> + *
>> + * Called from acpi_numa_init while reading the SRAT and SLIT tables.
>> + * Assumes all memory regions belonging to a single proximity domain
>> + * are in one chunk. Holes between them will be included in the node.
>> + *
>> + * Adapted for Xen: Ryan Harper <ryanh@xxxxxxxxxx>
>> + *
>> + * Moved this generic code from xen/arch/x86/srat.c for other arch usage
>> + * by Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/mm.h>
>> +#include <xen/inttypes.h>
>> +#include <xen/nodemask.h>
>> +#include <xen/acpi.h>
>> +#include <xen/numa.h>
>> +#include <xen/pfn.h>
>> +#include <xen/srat.h>
>> +#include <asm/page.h>
>> +
>> +struct acpi_table_slit *__read_mostly acpi_slit;
>
>
> This should really be static.
>
>> +extern struct node nodes[MAX_NUMNODES] __initdata;
>> +
>> +struct pxm2node __read_mostly pxm2node[MAX_NUMNODES] =
>> +    { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
>
>
> So this is not only exposed because of bad_srat(). The code should be
> reworked to avoid that.

I will check.
>
>> +
>> +static inline bool_t node_found(unsigned idx, unsigned pxm)
>> +{
>> +    return ((pxm2node[idx].pxm == pxm) &&
>> +        (pxm2node[idx].node != NUMA_NO_NODE));
>> +}
>> +
>> +nodeid_t pxm_to_node(unsigned pxm)
>> +{
>> +    unsigned i;
>> +
>> +    if ( (pxm < ARRAY_SIZE(pxm2node)) && node_found(pxm, pxm) )
>> +        return pxm2node[pxm].node;
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(pxm2node); i++ )
>> +        if ( node_found(i, pxm) )
>> +            return pxm2node[i].node;
>> +
>> +    return NUMA_NO_NODE;
>> +}
>> +
>> +nodeid_t setup_node(unsigned pxm)
>
>
> This name is too generic. The name of the function should make clear it is
> an ACPI only function.
>
OK
> [...]
>
>> +unsigned node_to_pxm(nodeid_t n)
>> +{
>> +    unsigned i;
>> +
>> +    if ( (n < ARRAY_SIZE(pxm2node)) && (pxm2node[n].node == n) )
>> +        return pxm2node[n].pxm;
>> +    for ( i = 0; i < ARRAY_SIZE(pxm2node); i++ )
>> +        if ( pxm2node[i].node == n )
>> +            return pxm2node[i].pxm;
>> +    return 0;
>> +}
>
>
> Missing emacs magic here.
You mean this at the end of the file?

/*
 * Local variables:
 * mode: C
 * c-file-style: "BSD"
 * c-basic-offset: 4
 * indent-tabs-mode: nil
 * End:
 */

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel