x86/mm/numa: Open code function early_get_boot_cpu_id

PATCH Superseded Baoquan He 3 participants 8 messages Jun 27, 2016 – Jul 9, 2016

Subsystems (3)

x86 ACPI x86 MM

lore

Share & tools

lore

Baoquan He

author

2016-06-27 23:41 UTC #0 lore raw

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

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 <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc H. Peter Anvin <[email protected]>

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

arch/x86/mm/amdtopology.c +8 ~~-17~~

index 2ca15b59..9928dad 100644
@@ -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;

Signature · 1 line

2.5.5

Baoquan He

author

2016-06-27 23:42 UTC #1 lore raw

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

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 <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Len Brown <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Shaohua Li <[email protected]>

Cc Viresh Kumar <[email protected]>

Cc Hidehiro Kawai <[email protected]>

Cc Juergen Gross <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Dave Young <[email protected]>

Cc Lv Zheng <[email protected]>

Cc Toshi Kani <[email protected]>

Cc Mark Salter <[email protected]>

Cc Dave Hansen <[email protected]>

---
 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(-)

arch/x86/kernel/acpi/boot.c +2 ~~-15~~

index 9414f84..6ef3694 100644
@@ -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);

arch/x86/kernel/apic/apic.c +1 -1

index 60078a6..504311c 100644
@@ -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();

arch/x86/kernel/setup.c +3 -0

index c4e7b39..059680d 100644
@@ -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();

Signature · 1 line

2.5.5

Rafael J. Wysocki

2016-06-28 01:21 UTC #2 lore raw

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

Quoted 61 lines · 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 <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Shaohua Li <[email protected]>
Cc Viresh Kumar <[email protected]>
Cc Hidehiro Kawai <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Dave Young <[email protected]>
Cc Lv Zheng <[email protected]>
Cc Toshi Kani <[email protected]>
Cc Mark Salter <[email protected]>
Cc Dave Hansen <[email protected]>
---
 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(-)
arch/x86/kernel/acpi/boot.c +2 -5
index 9414f84..6ef3694 100644
@@ -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).
-	 */
-

arch/x86/kernel/acpi/boot.c static int __init acpi_parse_madt_lapic_entries(void) · line 1007

Quoted 10 lines

arch/x86/kernel/acpi/boot.c +0 ~~-10~~

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

Quoted 21 lines

arch/x86/kernel/acpi/boot.c +0 -0

 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
 				      acpi_parse_sapic, MAX_LOCAL_APIC);

arch/x86/kernel/apic/apic.c +1 -1

index 60078a6..504311c 100644
@@ -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();

arch/x86/kernel/setup.c +0 -0

index c4e7b39..059680d 100644

arch/x86/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 1162

Quoted 6 lines

arch/x86/kernel/setup.c +3 -0

@@ -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?

arch/x86/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 1165

Quoted 4 lines

arch/x86/kernel/setup.c +0 -0

 	early_acpi_boot_init();
 
 	initmem_init();

Please CC ACPI-related patches to [email protected].

Thanks,
Rafael

Baoquan He

author

2016-06-28 03:31 UTC #3 lore raw

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

Quoted 26 lines · Rafael J. Wysocki

Quoted 13 lines

arch/x86/kernel/acpi/boot.c +0 -5

index 9414f84..6ef3694 100644
@@ -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).
-	 */
-

arch/x86/kernel/acpi/boot.c static int __init acpi_parse_madt_lapic_entries(void) · line 1007

Quoted 10 lines

arch/x86/kernel/acpi/boot.c +0 ~~-10~~

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

Quoted 1 line

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

Quoted 25 lines

arch/x86/kernel/acpi/boot.c +0 -0

 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
 				      acpi_parse_sapic, MAX_LOCAL_APIC);

arch/x86/kernel/apic/apic.c +1 -1

index 60078a6..504311c 100644
@@ -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();

arch/x86/kernel/setup.c +0 -0

index c4e7b39..059680d 100644
@@ -1157,6 +1157,9 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	acpi_boot_table_init();

Quoted 5 lines

arch/x86/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 1162
Quoted 3 lines
arch/x86/kernel/setup.c +3 -0
+	/*
+	 * 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.

Quoted 7 lines

arch/x86/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 1165
Quoted 5 lines
arch/x86/kernel/setup.c +0 -0
 	early_acpi_boot_init();
 
 	initmem_init();
Please CC ACPI-related patches to [email protected].

Will do when repost.

Quoted 4 lines

Thanks,
Rafael

Baoquan He

author

2016-06-30 08:02 UTC #4 lore raw

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

In reply to Baoquan He

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 <[email protected]>

---

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(-)

arch/x86/kernel/acpi/boot.c +2 ~~-15~~

index 9414f84..6ef3694 100644
@@ -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);

arch/x86/kernel/apic/apic.c +1 -1

index 60078a6..504311c 100644
@@ -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();

Signature · 1 line

2.5.5

Ingo Molnar

2016-07-08 12:27 UTC #5 lore raw

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

Quoted 10 lines · 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.

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

Baoquan He

author

2016-07-08 12:56 UTC #6 lore raw

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

Quoted 11 lines · Ingo Molnar

Quoted 8 lines · 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.

Quoted 5 lines

Quoted 2 lines
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

Baoquan He

author

2016-07-09 02:04 UTC #7 lore raw

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

In reply to Ingo Molnar

Hi Ingo,

Quoted 11 lines · Ingo Molnar

Quoted 8 lines · 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.

Quoted 5 lines

Quoted 2 lines
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