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

[Julien Grall <julien.grall@xxxxxxx>]

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/

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

[...]

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

[...]

>  /* 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?
[...]

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

>  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.
> +
> +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.
[...]

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

> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> index 659ff6a..79a445c 100644
> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -17,8 +17,6 @@ extern cpumask_t     node_to_cpumask[];
>  #define node_to_first_cpu(node)  (__ffs(node_to_cpumask[node]))
>  #define node_to_cpumask(node)    (node_to_cpumask[node])
>
> -extern nodeid_t pxm_to_node(unsigned int pxm);
> -
>  #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
>
>  extern void numa_init_array(void);
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index 4f04ab4..eb18380 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -73,7 +73,6 @@ extern int valid_numa_range(u64 start, u64 end, nodeid_t 
> node);
>  extern int conflicting_memblks(u64 start, u64 end);
>  extern void cutoff_node(int i, u64 start, u64 end);
>  extern void numa_add_cpu(int cpu);
> -extern nodeid_t setup_node(unsigned int pxm);
>  extern void numa_set_node(int cpu, nodeid_t node);
>  extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
>  extern int compute_hash_shift(struct node *nodes, int numnodes,
> diff --git a/xen/include/xen/srat.h b/xen/include/xen/srat.h
> new file mode 100644
> index 0000000..978f1e8
> --- /dev/null
> +++ b/xen/include/xen/srat.h
> @@ -0,0 +1,13 @@
> +#ifndef __XEN_SRAT_H__
> +#define __XEN_SRAT_H__
> +
> +struct pxm2node {
> +    unsigned pxm;
> +    nodeid_t node;
> +};
> +
> +extern struct pxm2node __read_mostly pxm2node[MAX_NUMNODES];
> +nodeid_t pxm_to_node(unsigned pxm);
> +nodeid_t setup_node(unsigned pxm);
> +unsigned node_to_pxm(nodeid_t n);
> +#endif /* __XEN_SRAT_H__ */
Missing emacs magic.

>
Regards,

--
Julien Grall

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