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

2017-03-02 Thread Julien Grall 



On 02/03/17 16:31, Vijay Kilari wrote:

On Thu, Mar 2, 2017 at 9:00 PM, Julien Grall  wrote:

Missing emacs magic here.

You mean this at the end of the file?


Yes.



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



--
Julien Grall

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

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

2017-03-02 Thread Vijay Kilari 
On Thu, Mar 2, 2017 at 9:00 PM, Julien Grall  wrote:
> Hello Vijay,
>
> On 09/02/17 15:57, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K 
>>
>> 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> -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 000..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 
>> + *
>> + * Moved this generic code from xen/arch/x86/srat.c for other arch usage
>> + * by Vijaya Kumar K 
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +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 <

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

2017-03-02 Thread Julien Grall 

Hello Vijay,

On 09/02/17 15:57, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 

-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 000..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 
+ *
+ * Moved this generic code from xen/arch/x86/srat.c for other arch usage
+ * by Vijaya Kumar K 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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 <<

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

2017-02-09 Thread vijay . kilari 
From: Vijaya Kumar K 

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 
---
 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 
 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 
 13 files changed, 173 insertions(+), 130 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 243df96..4d7795b 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 28d1891..58de324 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 5a49796..2184d62 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 176ee74..ee7a368 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 9b390b8..6c61114 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
-static struct acpi_table_slit *__read_mostly acpi_slit;
+extern struct acpi_table_slit *__read_mostly acpi_slit;
 
 static nodemask_t memory_nodes_parsed __initdata;
 static nodemask_t processor_nodes_parsed __initdata;
 extern struct node nodes[MAX_NUMNODES] __initdata;
-
-struct pxm2node {
-   unsigned pxm;
-   nodeid_t node;
-};
-static struct pxm2node __read_mostly pxm2node[MAX_NUMNODES] =
-   { [0 ... MAX_NUMNODES - 1] = {.node = NUMA_NO_NODE} };
-
-static unsigned node_to_pxm(nodeid_t n);
-
 extern int num_node_memblks;
 extern struct node node_memblk_range[NR_NODE_MEMBLKS];
 extern nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
 
-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)
-{
-   nodeid_t node;
-   unsigned idx;
-   static bool_t warned;
-   static unsigned nodes_found;
-
-   BUILD_BUG_ON(MAX_NUMNODES >= NUMA_NO_NODE);
-
-   if (pxm < ARRAY_SIZE(pxm2node)) {
-   if (node_found(pxm, pxm))
-   return pxm2node[pxm].node;
-
-   /* Try to maintain indexing of pxm2node by pxm */
-   if (pxm2node[pxm].node == NUMA_NO_NODE) {
-   idx = pxm;
-   goto finish;
-   }
-   }
-
-   for (idx = 0; idx < ARRAY_SIZE(pxm2node); idx++)
-   if (pxm2node[idx].node == NUMA_NO_NODE)
-   goto finish;
-
-   if (!warned) {
-   printk(KERN_WARNING "SRAT: Too many proximity domains (%#x)\n",
-  pxm);
-   warned = 1;
-   }
-
-   return NUMA_NO_NODE;
-
- finish:
-   node = nodes_found++;
-   if (node >= MAX_NUMNODES)
-   return NUMA_NO_NODE;
-   pxm2node[idx].pxm = pxm;
-   pxm2node[idx].node = node;
-
-   return node;
-}
-
 static __init void bad_srat(void)
 {
int i;
@@ -115,48 +44,6 @@ static __init void bad_srat(void)
mem_hotplug = 0;
 }
 
-/*
- * A lot of BIOS fill in 10 (= no distance) everywhere. This