Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

2014-01-24 Thread Hanjun Guo

On 2014年01月23日 23:56, Tomasz Nowicki wrote:

Hi Lorenzo,

W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:

On Fri, Jan 17, 2014 at 12:24:58PM +, Hanjun Guo wrote:

[...]


diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index bd9bbd0..2210353 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -41,6 +41,7 @@
#include linux/memblock.h
#include linux/of_fdt.h
#include linux/of_platform.h
+#include linux/acpi.h

#include asm/cputype.h
#include asm/elf.h
@@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

+ /* Parse the ACPI tables for possible boot-time configuration */
+ acpi_boot_table_init();
+ early_acpi_boot_init();
+ acpi_boot_init();
+
paging_init();


Can I ask you please why we need to parse ACPI tables before
paging_init() ?
This is for future usage and because of couple of reasons. Mainly SRAT 
table parsing should be done (before paging_init()) for proper NUMA 
initialization and then paging_init().


Yes, I agree, thanks for Tomasz's clarification.



[...]


+/*
+ * __acpi_map_table() will be called before page_init(), so 
early_ioremap()

+ * or early_memremap() should be called here.


Again, why is this needed ? What's needed before paging_init() from 
ACPI ?


[...]


+/*
+ * acpi_boot_table_init() and acpi_boot_init()
+ * called from setup_arch(), always.
+ * 1. checksums all tables
+ * 2. enumerates lapics
+ * 3. enumerates io-apics
+ *
+ * acpi_table_init() is separated to allow reading SRAT without
+ * other side effects.
+ */
+void __init acpi_boot_table_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return;
+
+ /*
+ * Initialize the ACPI boot-time table parser.
+ */
+ if (acpi_table_init()) {
+ disable_acpi();
+ return;
+ }
+}
+
+int __init early_acpi_boot_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return -ENODEV;
+
+ /*
+ * Process the Multiple APIC Description Table (MADT), if present
+ */
+ early_acpi_process_madt();
+
+ return 0;
+}
+
+int __init acpi_boot_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return -ENODEV;
+
+ acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
+
+ /*
+ * Process the Multiple APIC Description Table (MADT), if present
+ */
+ acpi_process_madt();
+
+ return 0;
+}


Well, apart from having three init calls, one returning void and two
returning proper values, do not understand why, and do not understand
why we need three calls in the first place...why should we process MADT
twice in two separate calls ? What is supposed to change in between that
prevents you from merging the two together ?


Thanks for pointing this out. I can merge acpi_boot_table_init() and
early_acpi_boot_init() together, but can not merge early_acpi_boot_init()
and acpi_boot_init() together.

early_acpi_boot_init() and acpi_boot_init() was separated intentionally for
memory hotplug reasons. memory allocated in this stage can not be migrated
and cause memory hot-remove failed, in order to keep memory allocated
at base node (general NUMA node 0 in the system) at boot stage, we should
parse SRAT first before CPU is enumerated, does this make sense to you?

Thanks
Hanjun

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

2014-01-24 Thread Lorenzo Pieralisi
Hi Hanjun,

On Fri, Jan 24, 2014 at 09:09:40AM +, Hanjun Guo wrote:
 On 2014?01?23? 23:56, Tomasz Nowicki wrote:
  Hi Lorenzo,
 
  W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:
  On Fri, Jan 17, 2014 at 12:24:58PM +, Hanjun Guo wrote:
 
  [...]
 
  diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
  index bd9bbd0..2210353 100644
  --- a/arch/arm64/kernel/setup.c
  +++ b/arch/arm64/kernel/setup.c
  @@ -41,6 +41,7 @@
  #include linux/memblock.h
  #include linux/of_fdt.h
  #include linux/of_platform.h
  +#include linux/acpi.h
 
  #include asm/cputype.h
  #include asm/elf.h
  @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)
 
  arm64_memblock_init();
 
  + /* Parse the ACPI tables for possible boot-time configuration */
  + acpi_boot_table_init();
  + early_acpi_boot_init();
  + acpi_boot_init();
  +
  paging_init();
 
  Can I ask you please why we need to parse ACPI tables before
  paging_init() ?
  This is for future usage and because of couple of reasons. Mainly SRAT 
  table parsing should be done (before paging_init()) for proper NUMA 
  initialization and then paging_init().

Thank you for the explanation. I still have some questions:

1) What are the other reasons ?
2) NUMA is not supported at the moment and I reckon SRAT needs updating
   since the only way to associate a CPU to a memory node is through
   a local APIC id that is non-existent on ARM and at least deserves a
   new entry.

I am still not sure that providing a hook for parsing the ACPI tables before
paging_init() is the main focus at the moment, it is probably best, as we've
mentioned manifold times, to make sure that the infrastructure to detect
ACPI vs DT at run-time is in place in the kernel and allows us to boot
either with ACPI or DT, in a mutual exclusive way (same binary kernel
supporting both, runtime detection/decision on what data to use, ACPI tables
vs DT nodes, detection made once for all and NOT on a per property basis).

I will have a look at SRAT and how it is used on x86, and get back to you on
this.

[...]

  + * acpi_boot_table_init() and acpi_boot_init()
  + * called from setup_arch(), always.
  + * 1. checksums all tables
  + * 2. enumerates lapics
  + * 3. enumerates io-apics
  + *
  + * acpi_table_init() is separated to allow reading SRAT without
  + * other side effects.
  + */
  +void __init acpi_boot_table_init(void)
  +{
  + /*
  + * If acpi_disabled, bail out
  + */
  + if (acpi_disabled)
  + return;
  +
  + /*
  + * Initialize the ACPI boot-time table parser.
  + */
  + if (acpi_table_init()) {
  + disable_acpi();
  + return;
  + }
  +}
  +
  +int __init early_acpi_boot_init(void)
  +{
  + /*
  + * If acpi_disabled, bail out
  + */
  + if (acpi_disabled)
  + return -ENODEV;
  +
  + /*
  + * Process the Multiple APIC Description Table (MADT), if present
  + */
  + early_acpi_process_madt();
  +
  + return 0;
  +}
  +
  +int __init acpi_boot_init(void)
  +{
  + /*
  + * If acpi_disabled, bail out
  + */
  + if (acpi_disabled)
  + return -ENODEV;
  +
  + acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
  +
  + /*
  + * Process the Multiple APIC Description Table (MADT), if present
  + */
  + acpi_process_madt();
  +
  + return 0;
  +}
 
  Well, apart from having three init calls, one returning void and two
  returning proper values, do not understand why, and do not understand
  why we need three calls in the first place...why should we process MADT
  twice in two separate calls ? What is supposed to change in between that
  prevents you from merging the two together ?
 
 Thanks for pointing this out. I can merge acpi_boot_table_init() and
 early_acpi_boot_init() together, but can not merge early_acpi_boot_init()
 and acpi_boot_init() together.
 
 early_acpi_boot_init() and acpi_boot_init() was separated intentionally for
 memory hotplug reasons. memory allocated in this stage can not be migrated
 and cause memory hot-remove failed, in order to keep memory allocated
 at base node (general NUMA node 0 in the system) at boot stage, we should
 parse SRAT first before CPU is enumerated, does this make sense to you?

Are you parsing the SRAT in this series to get memory info or memory is
still initialized by DT even when system is supposed to be booted with ACPI
(ie there is a valid ACPI root table ?)

I have a hunch the latter is what's happening (and that's wrong, because
memory information when kernel is booted through ACPI must be retrieved
from UEFI - at least that's what has been defined), so I still see an early
hook to initialize ACPI tables before paging_init() that has no use as the
current patchset stands, please correct me if I am wrong.

Thank you,
Lorenzo

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

2014-01-24 Thread Tomasz Nowicki

Hi Lorenzo,

W dniu 24.01.2014 13:53, Lorenzo Pieralisi pisze:

Hi Hanjun,

On Fri, Jan 24, 2014 at 09:09:40AM +, Hanjun Guo wrote:

On 2014?01?23? 23:56, Tomasz Nowicki wrote:

Hi Lorenzo,

W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:

On Fri, Jan 17, 2014 at 12:24:58PM +, Hanjun Guo wrote:

[...]


diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index bd9bbd0..2210353 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -41,6 +41,7 @@
#include linux/memblock.h
#include linux/of_fdt.h
#include linux/of_platform.h
+#include linux/acpi.h

#include asm/cputype.h
#include asm/elf.h
@@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

+ /* Parse the ACPI tables for possible boot-time configuration */
+ acpi_boot_table_init();
+ early_acpi_boot_init();
+ acpi_boot_init();
+
paging_init();


Can I ask you please why we need to parse ACPI tables before
paging_init() ?

This is for future usage and because of couple of reasons. Mainly SRAT
table parsing should be done (before paging_init()) for proper NUMA
initialization and then paging_init().


Thank you for the explanation. I still have some questions:

1) What are the other reasons ?
If we agreed that we need SRAT parsing before paging_init() the second 
reason is all implications related to that. I mean if we want to make 
ACPI tables parseable before paging_init(), we need early_ioremap 
mechanism and __acpi_map_table() fixes. In that case, IMHO, it is better 
to please it in the right place now. early_ioremap is object of UEFI 
support patch set.

2) NUMA is not supported at the moment and I reckon SRAT needs updating
since the only way to associate a CPU to a memory node is through
a local APIC id that is non-existent on ARM and at least deserves a
new entry.

Right, that requires further work on SRAT.


I am still not sure that providing a hook for parsing the ACPI tables before
paging_init() is the main focus at the moment, it is probably best, as we've
mentioned manifold times, to make sure that the infrastructure to detect
ACPI vs DT at run-time is in place in the kernel and allows us to boot
either with ACPI or DT, in a mutual exclusive way (same binary kernel
supporting both, runtime detection/decision on what data to use, ACPI tables
vs DT nodes, detection made once for all and NOT on a per property basis).

I will have a look at SRAT and how it is used on x86, and get back to you on
this.

[...]


+ * acpi_boot_table_init() and acpi_boot_init()
+ * called from setup_arch(), always.
+ * 1. checksums all tables
+ * 2. enumerates lapics
+ * 3. enumerates io-apics
+ *
+ * acpi_table_init() is separated to allow reading SRAT without
+ * other side effects.
+ */
+void __init acpi_boot_table_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return;
+
+ /*
+ * Initialize the ACPI boot-time table parser.
+ */
+ if (acpi_table_init()) {
+ disable_acpi();
+ return;
+ }
+}
+
+int __init early_acpi_boot_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return -ENODEV;
+
+ /*
+ * Process the Multiple APIC Description Table (MADT), if present
+ */
+ early_acpi_process_madt();
+
+ return 0;
+}
+
+int __init acpi_boot_init(void)
+{
+ /*
+ * If acpi_disabled, bail out
+ */
+ if (acpi_disabled)
+ return -ENODEV;
+
+ acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
+
+ /*
+ * Process the Multiple APIC Description Table (MADT), if present
+ */
+ acpi_process_madt();
+
+ return 0;
+}


Well, apart from having three init calls, one returning void and two
returning proper values, do not understand why, and do not understand
why we need three calls in the first place...why should we process MADT
twice in two separate calls ? What is supposed to change in between that
prevents you from merging the two together ?


Thanks for pointing this out. I can merge acpi_boot_table_init() and
early_acpi_boot_init() together, but can not merge early_acpi_boot_init()
and acpi_boot_init() together.

early_acpi_boot_init() and acpi_boot_init() was separated intentionally for
memory hotplug reasons. memory allocated in this stage can not be migrated
and cause memory hot-remove failed, in order to keep memory allocated
at base node (general NUMA node 0 in the system) at boot stage, we should
parse SRAT first before CPU is enumerated, does this make sense to you?


Are you parsing the SRAT in this series to get memory info or memory is
still initialized by DT even when system is supposed to be booted with ACPI
(ie there is a valid ACPI root table ?)

I have a hunch the latter is what's happening (and that's wrong, because
memory information when kernel is booted through ACPI must be retrieved
from UEFI - at least that's what has been defined), so I still see an early
hook to initialize ACPI tables before paging_init() that has no use as the
current patchset stands, please correct me if I am wrong.
Yes, currently memory is initialized by 

Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

2014-01-23 Thread Tomasz Nowicki

Hi Lorenzo,

W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:

On Fri, Jan 17, 2014 at 12:24:58PM +, Hanjun Guo wrote:

[...]


diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index bd9bbd0..2210353 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -41,6 +41,7 @@
  #include linux/memblock.h
  #include linux/of_fdt.h
  #include linux/of_platform.h
+#include linux/acpi.h

  #include asm/cputype.h
  #include asm/elf.h
@@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

+   /* Parse the ACPI tables for possible boot-time configuration */
+   acpi_boot_table_init();
+   early_acpi_boot_init();
+   acpi_boot_init();
+
paging_init();


Can I ask you please why we need to parse ACPI tables before
paging_init() ?
This is for future usage and because of couple of reasons. Mainly SRAT 
table parsing should be done (before paging_init()) for proper NUMA 
initialization and then paging_init().


Regards,
Tomasz


[...]


+/*
+ * __acpi_map_table() will be called before page_init(), so early_ioremap()
+ * or early_memremap() should be called here.


Again, why is this needed ? What's needed before paging_init() from ACPI ?

[...]


+/*
+ * acpi_boot_table_init() and acpi_boot_init()
+ *  called from setup_arch(), always.
+ * 1. checksums all tables
+ * 2. enumerates lapics
+ * 3. enumerates io-apics
+ *
+ * acpi_table_init() is separated to allow reading SRAT without
+ * other side effects.
+ */
+void __init acpi_boot_table_init(void)
+{
+   /*
+* If acpi_disabled, bail out
+*/
+   if (acpi_disabled)
+   return;
+
+   /*
+* Initialize the ACPI boot-time table parser.
+*/
+   if (acpi_table_init()) {
+   disable_acpi();
+   return;
+   }
+}
+
+int __init early_acpi_boot_init(void)
+{
+   /*
+* If acpi_disabled, bail out
+*/
+   if (acpi_disabled)
+   return -ENODEV;
+
+   /*
+* Process the Multiple APIC Description Table (MADT), if present
+*/
+   early_acpi_process_madt();
+
+   return 0;
+}
+
+int __init acpi_boot_init(void)
+{
+   /*
+* If acpi_disabled, bail out
+*/
+   if (acpi_disabled)
+   return -ENODEV;
+
+   acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
+
+   /*
+* Process the Multiple APIC Description Table (MADT), if present
+*/
+   acpi_process_madt();
+
+   return 0;
+}


Well, apart from having three init calls, one returning void and two
returning proper values, do not understand why, and do not understand
why we need three calls in the first place...why should we process MADT
twice in two separate calls ? What is supposed to change in between that
prevents you from merging the two together ?

Thanks,
Lorenzo


___
Linaro-acpi mailing list
linaro-a...@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-acpi


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/