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

[PATCH 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 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>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
---
 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 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. The relevant log will be printed out twice too and
this may confuse people.

So in this patch remove the repeated code in the 2nd part. Meanwhile
add code comment above early_acpi_boot_init() to explain why its
calling need be earlier, and print lapic override inforamtion like
other MADT entry.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de> 
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Shaohua Li <shli@fb.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Dave Young <dyoung@redhat.com>
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Toshi Kani <toshi.kani@hpe.com>
Cc: Mark Salter <msalter@redhat.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/acpi/boot.c | 17 ++---------------
 arch/x86/kernel/apic/apic.c |  2 +-
 arch/x86/kernel/setup.c     |  3 +++
 3 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 9414f84..6ef3694 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -274,6 +274,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;
@@ -990,21 +992,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 60078a6..504311c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1799,7 +1799,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();
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c4e7b39..059680d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1157,6 +1157,9 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	acpi_boot_table_init();
 
+	/*
+	 * AMD NUMA support need get boot_cpu_id earlier.
+	 */
 	early_acpi_boot_init();
 
 	initmem_init();
-- 
2.5.5

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

[Rafael J. Wysocki]

On Tuesday, June 28, 2016 07:41:36 AM Baoquan He 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. The relevant log will be printed out twice too and
> this may confuse people.
> 
> So in this patch remove the repeated code in the 2nd part. Meanwhile
> add code comment above early_acpi_boot_init() to explain why its
> calling need be earlier, and print lapic override inforamtion like
> other MADT entry.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Thomas Gleixner <tglx@linutronix.de> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Shaohua Li <shli@fb.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Lv Zheng <lv.zheng@intel.com>
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Cc: Mark Salter <msalter@redhat.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> ---
>  arch/x86/kernel/acpi/boot.c | 17 ++---------------
>  arch/x86/kernel/apic/apic.c |  2 +-
>  arch/x86/kernel/setup.c     |  3 +++
>  3 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 9414f84..6ef3694 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -274,6 +274,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;
> @@ -990,21 +992,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);
> -

I'm not really sure if this change is correct.

Does it only deal with the override?

>  	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 60078a6..504311c 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1799,7 +1799,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();
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c4e7b39..059680d 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1157,6 +1157,9 @@ void __init setup_arch(char **cmdline_p)
>  	 */
>  	acpi_boot_table_init();
>  
> +	/*
> +	 * AMD NUMA support need get boot_cpu_id earlier.
> +	 */

Surely that's not the only goal of early_acpi_boot_init(), is it?

>  	early_acpi_boot_init();
>  
>  	initmem_init();
> 

Please CC ACPI-related patches to linux-acpi@vger.kernel.org.

Thanks,
Rafael

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

[Baoquan He]

On 06/28/16 at 03:25am, Rafael J. Wysocki wrote:
 
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 9414f84..6ef3694 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -990,21 +992,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);
> > -
> 
> I'm not really sure if this change is correct.
> 
> Does it only deal with the override?

Hi Rafael,

Thanks for your comments.

Yes, it only deals with the override and it will call
register_lapic_address to set fixed mapping for lapic and get value for
boot_cpu_physical_apicid. Since this has heen done in
early_acpi_boot_init() there's no need to do it again in
acpi_boot_init().


> 
> >  	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 60078a6..504311c 100644
> > --- a/arch/x86/kernel/apic/apic.c
> > +++ b/arch/x86/kernel/apic/apic.c
> > @@ -1799,7 +1799,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();
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index c4e7b39..059680d 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -1157,6 +1157,9 @@ void __init setup_arch(char **cmdline_p)
> >  	 */
> >  	acpi_boot_table_init();
> >  
> > +	/*
> > +	 * AMD NUMA support need get boot_cpu_id earlier.
> > +	 */
> 
> Surely that's not the only goal of early_acpi_boot_init(), is it?

In commit cbf9bd60(acpi: get boot_cpu_id as early for k8_scan_nodes)
Yinghai added early_acpi_boot_init() and called it in amd numa code.
Then commit 2e42060c(x86, uv: add early detection of UV system types)
put it in arch/x86/kernel/setup.c to discover the apic type earlier.

So, you are right. This command adding is not correct.

> 
> >  	early_acpi_boot_init();
> >  
> >  	initmem_init();
> > 
> 
> Please CC ACPI-related patches to linux-acpi@vger.kernel.org.

Will do when repost.

> 
> Thanks,
> Rafael
> 

[PATCH v2 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.

Signed-off-by: Baoquan He <bhe@redhat.com>
---

v1->v2:
   -Remove the not correct code comment above early_acpi_boot_init()
    as Rafael 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 9414f84..6ef3694 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -274,6 +274,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;
@@ -990,21 +992,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 60078a6..504311c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1799,7 +1799,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 v2 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.

So this patch is not supposed to change behavior (modulo kernel messages), right? 
If so it would make sense to spell that out explicitly in the changelog.

Thanks,

	Ingo

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

[Baoquan He]

On 07/08/16 at 02:27pm, 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.
> 
> So this patch is not supposed to change behavior (modulo kernel messages), right? 
> If so it would make sense to spell that out explicitly in the changelog.

Yes, it won't. I will mention it in changelog and repost. Thanks for
your reviewing.

Thanks
Baoquan

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

[Baoquan He]

Hi Ingo,

On 07/08/16 at 02:27pm, 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.
> 
> So this patch is not supposed to change behavior (modulo kernel messages), right? 
> If so it would make sense to spell that out explicitly in the changelog.

I am not sure if I understand your question correctly. In this patch I
added the calling of acpi_table_print_madt_entry(header) in
acpi_parse_lapic_addr_ovr, it will print information related if a lapic
address override entry is provided as below:

	case ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE:                                                                  
                {                                                                                                 
                        struct acpi_madt_local_apic_override *p =                                                 
                            (struct acpi_madt_local_apic_override*)header;                                       
                        pr_info("LAPIC_ADDR_OVR (address[%p])\n",                                                 
                                (void *)(unsigned long)p->address);                                               
                }                                                                                                 
                break;

This will add one line of message to boot log if lapic addr override
entry provided:
"LAPIC_ADDR_OVR (address[0xXXXXXXXX])"

I don't know if this is the behaviour change (modulo kernel messages)
you mentioned.

Thanks
Baoquan