[Patch v3 1/2] x86/mm/numa: Open code function early_get_boot_cpu_id

[Patch v3 1/2] x86/mm/numa: Open code function early_get_boot_cpu_id

[Baoquan He]

Previously early_acpi_boot_init is called in early_get_boot_cpu_id()
to get value for boot_cpu_physical_apicid. Now early_acpi_boot_init()
has been taken out and moved to setup_arch(), the name of
early_get_boot_cpu_id doesn't match its implementation. And only the
getting boot-time SMP configuration code is left. So in this patch
open code it.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1->v3:
  No change from v1.

 arch/x86/mm/amdtopology.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
index 2ca15b59..9928dad 100644
--- a/arch/x86/mm/amdtopology.c
+++ b/arch/x86/mm/amdtopology.c
@@ -53,21 +53,6 @@ static __init int find_northbridge(void)
 	return -ENOENT;
 }
 
-static __init void early_get_boot_cpu_id(void)
-{
-	/*
-	 * need to get the APIC ID of the BSP so can use that to
-	 * create apicid_to_node in amd_scan_nodes()
-	 */
-#ifdef CONFIG_X86_MPPARSE
-	/*
-	 * get boot-time SMP configuration:
-	 */
-	if (smp_found_config)
-		early_get_smp_config();
-#endif
-}
-
 int __init amd_numa_init(void)
 {
 	u64 start = PFN_PHYS(0);
@@ -181,8 +166,14 @@ int __init amd_numa_init(void)
 	cores = 1 << bits;
 	apicid_base = 0;
 
-	/* get the APIC ID of the BSP early for systems with apicid lifting */
-	early_get_boot_cpu_id();
+#ifdef CONFIG_X86_MPPARSE
+	/*
+	 * get boot-time SMP configuration:
+	 */
+	if (smp_found_config)
+		early_get_smp_config();
+#endif
+
 	if (boot_cpu_physical_apicid > 0) {
 		pr_info("BSP APIC ID: %02x\n", boot_cpu_physical_apicid);
 		apicid_base = boot_cpu_physical_apicid;
-- 
2.5.5

[Patch v3 2/2] x86/acpi: Remove the repeated lapic address override entry parsing

[Baoquan He]

ACPI MADT has a 32-bit field providing lapic address at which
each processor can access its lapic information. MADT also contains
an optional entry to provide a 64-bit address to override the 32-bit
one. However the current code does the lapic address override entry
parsing twice. One is in early_acpi_boot_init() because AMD NUMA need
get boot_cpu_id earlier. The other is in acpi_boot_init() which parses
all MADT entries.

So in this patch remove the repeated code in the 2nd part. Meanwhile
print lapic override entry information like other MADT entry, this
will be added to boot log.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1->v2:
  -Remove the incorrect code comment above early_acpi_boot_init()
   as Rafael suggested.

v2->v3:
  -Spell out that lapic override entry information is added to boot
   log as Ingo suggested.

 arch/x86/kernel/acpi/boot.c | 17 ++---------------
 arch/x86/kernel/apic/apic.c |  2 +-
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 6738e5c..9f5b332 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -282,6 +282,8 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 	if (BAD_MADT_ENTRY(lapic_addr_ovr, end))
 		return -EINVAL;
 
+	acpi_table_print_madt_entry(header);
+
 	acpi_lapic_addr = lapic_addr_ovr->address;
 
 	return 0;
@@ -998,21 +1000,6 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	if (!boot_cpu_has(X86_FEATURE_APIC))
 		return -ENODEV;
 
-	/*
-	 * Note that the LAPIC address is obtained from the MADT (32-bit value)
-	 * and (optionally) overridden by a LAPIC_ADDR_OVR entry (64-bit value).
-	 */
-
-	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE,
-				      acpi_parse_lapic_addr_ovr, 0);
-	if (count < 0) {
-		printk(KERN_ERR PREFIX
-		       "Error parsing LAPIC address override entry\n");
-		return count;
-	}
-
-	register_lapic_address(acpi_lapic_addr);
-
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
 				      acpi_parse_sapic, MAX_LOCAL_APIC);
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index ac8d8ad..346ec91 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1801,7 +1801,7 @@ void __init register_lapic_address(unsigned long address)
 	if (!x2apic_mode) {
 		set_fixmap_nocache(FIX_APIC_BASE, address);
 		apic_printk(APIC_VERBOSE, "mapped APIC to %16lx (%16lx)\n",
-			    APIC_BASE, mp_lapic_addr);
+			    APIC_BASE, address);
 	}
 	if (boot_cpu_physical_apicid == -1U) {
 		boot_cpu_physical_apicid  = read_apic_id();
-- 
2.5.5

Re: [Patch v3 2/2] x86/acpi: Remove the repeated lapic address override entry parsing

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> ACPI MADT has a 32-bit field providing lapic address at which
> each processor can access its lapic information. MADT also contains
> an optional entry to provide a 64-bit address to override the 32-bit
> one. However the current code does the lapic address override entry
> parsing twice. One is in early_acpi_boot_init() because AMD NUMA need
> get boot_cpu_id earlier. The other is in acpi_boot_init() which parses
> all MADT entries.
> 
> So in this patch remove the repeated code in the 2nd part. Meanwhile
> print lapic override entry information like other MADT entry, this
> will be added to boot log.

it is not at all clear to me from this changelog whether the change is supposed to 
change anything. If not then please spell it out explicitly:

  "This patch is not supposed to change any behavior."

Or so.

Thanks,

	Ingo

Re: [Patch v3 2/2] x86/acpi: Remove the repeated lapic address override entry parsing

[Baoquan He]

On 08/10/16 at 04:02pm, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > ACPI MADT has a 32-bit field providing lapic address at which
> > each processor can access its lapic information. MADT also contains
> > an optional entry to provide a 64-bit address to override the 32-bit
> > one. However the current code does the lapic address override entry
> > parsing twice. One is in early_acpi_boot_init() because AMD NUMA need
> > get boot_cpu_id earlier. The other is in acpi_boot_init() which parses
> > all MADT entries.
> > 
> > So in this patch remove the repeated code in the 2nd part. Meanwhile
> > print lapic override entry information like other MADT entry, this
> > will be added to boot log.
> 
> it is not at all clear to me from this changelog whether the change is supposed to 
> change anything. If not then please spell it out explicitly:
> 
>   "This patch is not supposed to change any behavior."

I don't know if adding new information to boot log can be seen as
behavior change. If lapic override entry exist, the code change will
add one line of message to boot log:

	LAPIC_ADDR_OVR (address[0xXXXXXXXX])

If this is not behavior change, I will add the sentence you suggested.

Thanks
Baoquan

Re: [Patch v3 2/2] x86/acpi: Remove the repeated lapic address override entry parsing

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> On 08/10/16 at 04:02pm, Ingo Molnar wrote:
> > 
> > * Baoquan He <bhe@redhat.com> wrote:
> > 
> > > ACPI MADT has a 32-bit field providing lapic address at which
> > > each processor can access its lapic information. MADT also contains
> > > an optional entry to provide a 64-bit address to override the 32-bit
> > > one. However the current code does the lapic address override entry
> > > parsing twice. One is in early_acpi_boot_init() because AMD NUMA need
> > > get boot_cpu_id earlier. The other is in acpi_boot_init() which parses
> > > all MADT entries.
> > > 
> > > So in this patch remove the repeated code in the 2nd part. Meanwhile
> > > print lapic override entry information like other MADT entry, this
> > > will be added to boot log.
> > 
> > it is not at all clear to me from this changelog whether the change is supposed to 
> > change anything. If not then please spell it out explicitly:
> > 
> >   "This patch is not supposed to change any behavior."
> 
> I don't know if adding new information to boot log can be seen as
> behavior change. If lapic override entry exist, the code change will
> add one line of message to boot log:
> 
> 	LAPIC_ADDR_OVR (address[0xXXXXXXXX])
> 
> If this is not behavior change, I will add the sentence you suggested.

Yeah, you can write it:

	"This patch is not supposed to change any runtime behavior, other than 
	 improving kernel messages."

Thanks,

	Ingo

Re: [Patch v3 2/2] x86/acpi: Remove the repeated lapic address override entry parsing

[Baoquan He]

On 08/11/16 at 10:37am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > On 08/10/16 at 04:02pm, Ingo Molnar wrote:
> > > 
> > > * Baoquan He <bhe@redhat.com> wrote:
> > > 
> > > > ACPI MADT has a 32-bit field providing lapic address at which
> > > > each processor can access its lapic information. MADT also contains
> > > > an optional entry to provide a 64-bit address to override the 32-bit
> > > > one. However the current code does the lapic address override entry
> > > > parsing twice. One is in early_acpi_boot_init() because AMD NUMA need
> > > > get boot_cpu_id earlier. The other is in acpi_boot_init() which parses
> > > > all MADT entries.
> > > > 
> > > > So in this patch remove the repeated code in the 2nd part. Meanwhile
> > > > print lapic override entry information like other MADT entry, this
> > > > will be added to boot log.
> > > 
> > > it is not at all clear to me from this changelog whether the change is supposed to 
> > > change anything. If not then please spell it out explicitly:
> > > 
> > >   "This patch is not supposed to change any behavior."
> > 
> > I don't know if adding new information to boot log can be seen as
> > behavior change. If lapic override entry exist, the code change will
> > add one line of message to boot log:
> > 
> > 	LAPIC_ADDR_OVR (address[0xXXXXXXXX])
> > 
> > If this is not behavior change, I will add the sentence you suggested.
> 
> Yeah, you can write it:
> 
> 	"This patch is not supposed to change any runtime behavior, other than 
> 	 improving kernel messages."

Got it, will copy this to patch log and repost. Thanks a lot!

Thanks
Baoquan

Re: [Patch v3 1/2] x86/mm/numa: Open code function early_get_boot_cpu_id

[Baoquan He]

Hi Ingo,

Ping,

Do you have another comment to this two patches?

Thanks
Baoquan

On 07/30/16 at 07:10pm, Baoquan He wrote:
> Previously early_acpi_boot_init is called in early_get_boot_cpu_id()
> to get value for boot_cpu_physical_apicid. Now early_acpi_boot_init()
> has been taken out and moved to setup_arch(), the name of
> early_get_boot_cpu_id doesn't match its implementation. And only the
> getting boot-time SMP configuration code is left. So in this patch
> open code it.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> v1->v3:
>   No change from v1.
> 
>  arch/x86/mm/amdtopology.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
> index 2ca15b59..9928dad 100644
> --- a/arch/x86/mm/amdtopology.c
> +++ b/arch/x86/mm/amdtopology.c
> @@ -53,21 +53,6 @@ static __init int find_northbridge(void)
>  	return -ENOENT;
>  }
>  
> -static __init void early_get_boot_cpu_id(void)
> -{
> -	/*
> -	 * need to get the APIC ID of the BSP so can use that to
> -	 * create apicid_to_node in amd_scan_nodes()
> -	 */
> -#ifdef CONFIG_X86_MPPARSE
> -	/*
> -	 * get boot-time SMP configuration:
> -	 */
> -	if (smp_found_config)
> -		early_get_smp_config();
> -#endif
> -}
> -
>  int __init amd_numa_init(void)
>  {
>  	u64 start = PFN_PHYS(0);
> @@ -181,8 +166,14 @@ int __init amd_numa_init(void)
>  	cores = 1 << bits;
>  	apicid_base = 0;
>  
> -	/* get the APIC ID of the BSP early for systems with apicid lifting */
> -	early_get_boot_cpu_id();
> +#ifdef CONFIG_X86_MPPARSE
> +	/*
> +	 * get boot-time SMP configuration:
> +	 */
> +	if (smp_found_config)
> +		early_get_smp_config();
> +#endif
> +
>  	if (boot_cpu_physical_apicid > 0) {
>  		pr_info("BSP APIC ID: %02x\n", boot_cpu_physical_apicid);
>  		apicid_base = boot_cpu_physical_apicid;
> -- 
> 2.5.5
> 

Re: [Patch v3 1/2] x86/mm/numa: Open code function early_get_boot_cpu_id

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> Previously early_acpi_boot_init is called in early_get_boot_cpu_id()
> to get value for boot_cpu_physical_apicid. Now early_acpi_boot_init()
> has been taken out and moved to setup_arch(), the name of
> early_get_boot_cpu_id doesn't match its implementation. And only the
> getting boot-time SMP configuration code is left. So in this patch
> open code it.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> v1->v3:
>   No change from v1.
> 
>  arch/x86/mm/amdtopology.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
> index 2ca15b59..9928dad 100644
> --- a/arch/x86/mm/amdtopology.c
> +++ b/arch/x86/mm/amdtopology.c
> @@ -53,21 +53,6 @@ static __init int find_northbridge(void)
>  	return -ENOENT;
>  }
>  
> -static __init void early_get_boot_cpu_id(void)
> -{
> -	/*
> -	 * need to get the APIC ID of the BSP so can use that to
> -	 * create apicid_to_node in amd_scan_nodes()
> -	 */
> -#ifdef CONFIG_X86_MPPARSE
> -	/*
> -	 * get boot-time SMP configuration:
> -	 */
> -	if (smp_found_config)
> -		early_get_smp_config();
> -#endif
> -}
> -
>  int __init amd_numa_init(void)
>  {
>  	u64 start = PFN_PHYS(0);
> @@ -181,8 +166,14 @@ int __init amd_numa_init(void)
>  	cores = 1 << bits;
>  	apicid_base = 0;
>  
> -	/* get the APIC ID of the BSP early for systems with apicid lifting */
> -	early_get_boot_cpu_id();
> +#ifdef CONFIG_X86_MPPARSE
> +	/*
> +	 * get boot-time SMP configuration:
> +	 */
> +	if (smp_found_config)
> +		early_get_smp_config();
> +#endif

So I think this cleanup could be improved: any reason why the smp_found_config 
check is not done in early_get_smp_config()?

Plus it appears there's already a !CONFIG_X86_MPPARSE version of 
early_get_smp_config() [but please double check that it's available under all 
legitimate Kconfig variants] so we could drop the ugly #ifdef as well.

Thanks,

	Ingo

Re: [Patch v3 1/2] x86/mm/numa: Open code function early_get_boot_cpu_id

[Baoquan He]

On 08/10/16 at 04:01pm, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > Previously early_acpi_boot_init is called in early_get_boot_cpu_id()
> > to get value for boot_cpu_physical_apicid. Now early_acpi_boot_init()
> > has been taken out and moved to setup_arch(), the name of
> > early_get_boot_cpu_id doesn't match its implementation. And only the
> > getting boot-time SMP configuration code is left. So in this patch
> > open code it.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > v1->v3:
> >   No change from v1.
> > 
> >  arch/x86/mm/amdtopology.c | 25 ++++++++-----------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/x86/mm/amdtopology.c b/arch/x86/mm/amdtopology.c
> > index 2ca15b59..9928dad 100644
> > --- a/arch/x86/mm/amdtopology.c
> > +++ b/arch/x86/mm/amdtopology.c
> > @@ -53,21 +53,6 @@ static __init int find_northbridge(void)
> >  	return -ENOENT;
> >  }
> >  
> > -static __init void early_get_boot_cpu_id(void)
> > -{
> > -	/*
> > -	 * need to get the APIC ID of the BSP so can use that to
> > -	 * create apicid_to_node in amd_scan_nodes()
> > -	 */
> > -#ifdef CONFIG_X86_MPPARSE
> > -	/*
> > -	 * get boot-time SMP configuration:
> > -	 */
> > -	if (smp_found_config)
> > -		early_get_smp_config();
> > -#endif
> > -}
> > -
> >  int __init amd_numa_init(void)
> >  {
> >  	u64 start = PFN_PHYS(0);
> > @@ -181,8 +166,14 @@ int __init amd_numa_init(void)
> >  	cores = 1 << bits;
> >  	apicid_base = 0;
> >  
> > -	/* get the APIC ID of the BSP early for systems with apicid lifting */
> > -	early_get_boot_cpu_id();
> > +#ifdef CONFIG_X86_MPPARSE
> > +	/*
> > +	 * get boot-time SMP configuration:
> > +	 */
> > +	if (smp_found_config)
> > +		early_get_smp_config();
> > +#endif
> 
> So I think this cleanup could be improved: any reason why the smp_found_config 
> check is not done in early_get_smp_config()?

Yes, you are quite right. Both early_get_smp_config and get_smp_config
call default_get_smp_config() if CONFIG_X86_MPPARSE is defined. The
smp_found_config check should be done inside default_get_smp_config to
simplify code.

> 
> Plus it appears there's already a !CONFIG_X86_MPPARSE version of 
> early_get_smp_config() [but please double check that it's available under all 
> legitimate Kconfig variants] so we could drop the ugly #ifdef as well.

Yes, just as you said, if !CONFIG_X86_MPPARSE default_get_smp_config()
will be defined as x86_init_noop in arch/x86/include/asm/mpspec.h. So
the #ifdef checking can be removed.

I will repost with the change you suggested. Thanks for your comments.

Thanks
Baoquan