x86, AMD: cleanup: merge common code in early microcode loading

PATCH In review Torsten Kaiser 4 participants 10 messages Jul 23, 2013 – Aug 9, 2013 x86

Share & tools

Torsten Kaiser

author

2013-07-23 21:00 UTC #0 lore raw

PATCH 3/5 x86, AMD: cleanup: merge common code in early microcode loading

Extract common checks and initialisations from load_ucode_ap() and save_microcode_in_initrd_amd() to load_microcode_amd_early(). load_ucode_ap() gets a quick exit for !cpu, because for the BSP there is already a different function dealing with its update.

The original code already didn't anything, because without load_microcode_amd() getting called apply_microcode_amd() could not do anything.

Signed-off-by Torsten Kaiser <[email protected]>

arch/x86/kernel/microcode_amd_early.c +24 ~~-22~~

@@ -196,6 +196,23 @@ void __init load_ucode_amd_bsp(void)
 	apply_ucode_in_initrd(cd.data, cd.size);
 }
 
+static int load_microcode_amd_early(void)
+{
+	enum ucode_state ret;
+	void *ucode;
+
+	if (ucode_loaded || !ucode_size || !initrd_start)
+		return 0;
+
+	ucode = (void *)(initrd_start + ucode_offset);
+	ret = load_microcode_amd(0, ucode, ucode_size);
+	if (ret != UCODE_OK)
+		return -EINVAL;
+
+	ucode_loaded = true;
+	return 0;
+}
+
 #ifdef CONFIG_X86_32
 u8 amd_bsp_mpb[MPB_MAX_SIZE];

@@ -258,17 +275,13 @@ void load_ucode_amd_ap(void)
 
 	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
 
-	if (cpu && !ucode_loaded) {
-		void *ucode;
-
-		if (!ucode_size || !initrd_start)
-			return;
+	/* BSP via load_ucode_amd_bsp() */
+	if (!cpu)
+		return;
 
-		ucode = (void *)(initrd_start + ucode_offset);
-		if (load_microcode_amd(0, ucode, ucode_size) != UCODE_OK)
-			return;
-		ucode_loaded = true;
-	}
+	load_microcode_amd_early();
+	if (!ucode_loaded)
+		return;
 
 	apply_microcode_amd(cpu);
 }

@@ -276,8 +289,6 @@ void load_ucode_amd_ap(void)
 
 int __init save_microcode_in_initrd_amd(void)
 {
-	enum ucode_state ret;
-	void *ucode;
 #ifdef CONFIG_X86_32
 	unsigned int bsp = boot_cpu_data.cpu_index;
 	struct ucode_cpu_info *uci = ucode_cpu_info + bsp;

@@ -289,14 +300,5 @@ int __init save_microcode_in_initrd_amd(
 		pr_info("microcode: updated early to new patch_level=0x%08x\n",
 			ucode_new_rev);
 
-	if (ucode_loaded || !ucode_size || !initrd_start)
-		return 0;
-
-	ucode = (void *)(initrd_start + ucode_offset);
-	ret = load_microcode_amd(0, ucode, ucode_size);
-	if (ret != UCODE_OK)
-		return -EINVAL;
-
-	ucode_loaded = true;
-	return 0;
+	return load_microcode_amd_early();
 }

Borislav Petkov

maintainer

2013-07-24 13:33 UTC #1 lore raw

Quoted 38 lines · Torsten Kaiser

Extract common checks and initialisations from load_ucode_ap() and save_microcode_in_initrd_amd() to load_microcode_amd_early(). load_ucode_ap() gets a quick exit for !cpu, because for the BSP there is already a different function dealing with its update.
The original code already didn't anything, because without load_microcode_amd() getting called apply_microcode_amd() could not do anything.
Signed-off-by Torsten Kaiser <[email protected]>
arch/x86/kernel/microcode_amd_early.c +17 -0
@@ -196,6 +196,23 @@ void __init load_ucode_amd_bsp(void)
 	apply_ucode_in_initrd(cd.data, cd.size);
 }
 
+static int load_microcode_amd_early(void)
+{
+	enum ucode_state ret;
+	void *ucode;
+
+	if (ucode_loaded || !ucode_size || !initrd_start)
+		return 0;
+
+	ucode = (void *)(initrd_start + ucode_offset);
+	ret = load_microcode_amd(0, ucode, ucode_size);
+	if (ret != UCODE_OK)
+		return -EINVAL;
+
+	ucode_loaded = true;
+	return 0;
+}
+
 #ifdef CONFIG_X86_32
 u8 amd_bsp_mpb[MPB_MAX_SIZE];
 
@@ -258,17 +275,13 @@ void load_ucode_amd_ap(void)
 

arch/x86/kernel/microcode_amd_early.c void load_ucode_amd_ap(void) · line 280

Quoted 10 lines

arch/x86/kernel/microcode_amd_early.c +3 -5

 	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
 
-	if (cpu && !ucode_loaded) {
-		void *ucode;
-
-		if (!ucode_size || !initrd_start)
-			return;
+	/* BSP via load_ucode_amd_bsp() */
+	if (!cpu)
+		return;

Ok, this is really misleading. Fenghua, what's the reason for calling load_ucode_ap() on the BSP too?

We have on the one hand:

x86_64_start_kernel
|->load_ucode_bsp

and on the other:

x86_64_start_kernel
|-> x86_64_start_reservations

    |-> start_kernel
        |-> trap_init
	    |-> cpu_init
	        |-> load_ucode_ap()

so we attempt to load the ucode twice on the BSP.

IMO, we should do this in cpu_init:

	if (cpu)
		load_ucode_ap();

no?

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Yu, Fenghua

2013-08-07 22:46 UTC #2 lore raw

Quoted 66 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, July 24, 2013 6:33 AM
Quoted 3 lines · Torsten Kaiser
Extract common checks and initialisations from load_ucode_ap() and save_microcode_in_initrd_amd() to load_microcode_amd_early(). load_ucode_ap() gets a quick exit for !cpu, because for the BSP there
is
Quoted 3 lines
already a different function dealing with its update.
The original code already didn't anything, because without
load_microcode_amd()
Quoted 5 lines
getting called apply_microcode_amd() could not do anything.
Signed-off-by Torsten Kaiser <[email protected]>
--- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
06:22:32.000000000 +0200
Quoted 1 line
+++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
20:00:04.889508712 +0200
Quoted 26 lines
arch/x86/kernel/microcode_amd_early.c +17 -0
@@ -196,6 +196,23 @@ void __init load_ucode_amd_bsp(void)
 	apply_ucode_in_initrd(cd.data, cd.size);
 }
 
+static int load_microcode_amd_early(void)
+{
+	enum ucode_state ret;
+	void *ucode;
+
+	if (ucode_loaded || !ucode_size || !initrd_start)
+		return 0;
+
+	ucode = (void *)(initrd_start + ucode_offset);
+	ret = load_microcode_amd(0, ucode, ucode_size);
+	if (ret != UCODE_OK)
+		return -EINVAL;
+
+	ucode_loaded = true;
+	return 0;
+}
+
 #ifdef CONFIG_X86_32
 u8 amd_bsp_mpb[MPB_MAX_SIZE];
 
@@ -258,17 +275,13 @@ void load_ucode_amd_ap(void)
 
arch/x86/kernel/microcode_amd_early.c void load_ucode_amd_ap(void) · line 280
Quoted 10 lines
arch/x86/kernel/microcode_amd_early.c +3 -5
 	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
 
-	if (cpu && !ucode_loaded) {
-		void *ucode;
-
-		if (!ucode_size || !initrd_start)
-			return;
+	/* BSP via load_ucode_amd_bsp() */
+	if (!cpu)
+		return;
Ok, this is really misleading. Fenghua, what's the reason for calling load_ucode_ap() on the BSP too?
We have on the one hand:
x86_64_start_kernel
|->load_ucode_bsp
and on the other:

Quoted 9 lines

x86_64_start_kernel
|-> x86_64_start_reservations
    |-> start_kernel
        |-> trap_init
	    |-> cpu_init
	        |-> load_ucode_ap()
so we attempt to load the ucode twice on the BSP.

You are right. Though the second time loading attempt will not find a valid/newer ucode patch to update. So this won't cause a real/serious problem. But we had better to fix this anyway.

Quoted 6 lines

IMO, we should do this in cpu_init:
	if (cpu)
		load_ucode_ap();
no?

This check won't work when CPU0 is hot added. So we need to find a better way to fix this.

Thanks.

-Fenghua ��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Yu, Fenghua

2013-08-07 23:34 UTC #3 lore raw

In reply to Borislav Petkov

Quoted 86 lines

From: Yu, Fenghua
Sent: Wednesday, August 07, 2013 3:46 PM
Quoted 6 lines
From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, July 24, 2013 6:33 AM
Quoted 3 lines · Torsten Kaiser
Extract common checks and initialisations from load_ucode_ap() and save_microcode_in_initrd_amd() to load_microcode_amd_early(). load_ucode_ap() gets a quick exit for !cpu, because for the BSP
there
Quoted 60 lines
is
Quoted 3 lines
already a different function dealing with its update.
The original code already didn't anything, because without
load_microcode_amd()
Quoted 5 lines
getting called apply_microcode_amd() could not do anything.
Signed-off-by Torsten Kaiser <[email protected]>
--- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
06:22:32.000000000 +0200
Quoted 1 line
+++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
20:00:04.889508712 +0200
Quoted 26 lines
arch/x86/kernel/microcode_amd_early.c +17 -0
@@ -196,6 +196,23 @@ void __init load_ucode_amd_bsp(void)
 	apply_ucode_in_initrd(cd.data, cd.size);
 }
 
+static int load_microcode_amd_early(void)
+{
+	enum ucode_state ret;
+	void *ucode;
+
+	if (ucode_loaded || !ucode_size || !initrd_start)
+		return 0;
+
+	ucode = (void *)(initrd_start + ucode_offset);
+	ret = load_microcode_amd(0, ucode, ucode_size);
+	if (ret != UCODE_OK)
+		return -EINVAL;
+
+	ucode_loaded = true;
+	return 0;
+}
+
 #ifdef CONFIG_X86_32
 u8 amd_bsp_mpb[MPB_MAX_SIZE];
 
@@ -258,17 +275,13 @@ void load_ucode_amd_ap(void)
 
arch/x86/kernel/microcode_amd_early.c void load_ucode_amd_ap(void) · line 280
Quoted 10 lines
arch/x86/kernel/microcode_amd_early.c +3 -5
 	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
 
-	if (cpu && !ucode_loaded) {
-		void *ucode;
-
-		if (!ucode_size || !initrd_start)
-			return;
+	/* BSP via load_ucode_amd_bsp() */
+	if (!cpu)
+		return;
Ok, this is really misleading. Fenghua, what's the reason for calling load_ucode_ap() on the BSP too?
We have on the one hand:
x86_64_start_kernel
|->load_ucode_bsp
and on the other:
Quoted 9 lines
x86_64_start_kernel
|-> x86_64_start_reservations
    |-> start_kernel
        |-> trap_init
	    |-> cpu_init
	        |-> load_ucode_ap()
so we attempt to load the ucode twice on the BSP.
You are right. Though the second time loading attempt will not find a valid/newer ucode patch to update. So this won't cause a real/serious problem. But we had better to fix this anyway.
Quoted 2 lines
IMO, we should do this in cpu_init:

Quoted 8 lines

Quoted 4 lines
	if (cpu)
		load_ucode_ap();
no?
This check won't work when CPU0 is hot added. So we need to find a better way to fix this.

Maybe need to change the check as follows to take care of CPU0 hot add case?

if ((cpu && system_state == SYSTEM_BOOTING) || (system_state == SYSTEM_RUNNING))

	load_ucode_ap();

Thanks.

H. Peter Anvin

reviewer

2013-08-08 02:25 UTC #4 lore raw

How much does this matter?

Quoted 108 lines · Fenghua

Quoted 7 lines
From: Yu, Fenghua
Sent: Wednesday, August 07, 2013 3:46 PM
Quoted 4 lines
From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, July 24, 2013 6:33 AM
Quoted 1 line · Torsten Kaiser
Extract common checks and initialisations from load_ucode_ap()
and
Quoted 40 lines
Quoted 2 lines
Quoted 2 lines
save_microcode_in_initrd_amd() to load_microcode_amd_early(). load_ucode_ap() gets a quick exit for !cpu, because for the BSP
there
Quoted 37 lines
is
Quoted 3 lines
already a different function dealing with its update.
The original code already didn't anything, because without
load_microcode_amd()
Quoted 5 lines
getting called apply_microcode_amd() could not do anything.
Signed-off-by Torsten Kaiser <[email protected]>
--- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
06:22:32.000000000 +0200
Quoted 1 line
+++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
20:00:04.889508712 +0200
Quoted 24 lines
arch/x86/kernel/microcode_amd_early.c +17 -0
@@ -196,6 +196,23 @@ void __init load_ucode_amd_bsp(void)
 	apply_ucode_in_initrd(cd.data, cd.size);
 }
 
+static int load_microcode_amd_early(void)
+{
+	enum ucode_state ret;
+	void *ucode;
+
+	if (ucode_loaded || !ucode_size || !initrd_start)
+		return 0;
+
+	ucode = (void *)(initrd_start + ucode_offset);
+	ret = load_microcode_amd(0, ucode, ucode_size);
+	if (ret != UCODE_OK)
+		return -EINVAL;
+
+	ucode_loaded = true;
+	return 0;
+}
+
 #ifdef CONFIG_X86_32
 u8 amd_bsp_mpb[MPB_MAX_SIZE];
 
Quoted 3 lines
Quoted 3 lines
Review void load_ucode_amd_ap(void) · line 276
Quoted 3 lines
(quoted diff) +0 -0
@@ -258,17 +275,13 @@ void load_ucode_amd_ap(void)
 
 	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info +
cpu);
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/microcode_amd_early.c void load_ucode_amd_ap(void) · line 280
Quoted 9 lines
arch/x86/kernel/microcode_amd_early.c +3 -5
-	if (cpu && !ucode_loaded) {
-		void *ucode;
-
-		if (!ucode_size || !initrd_start)
-			return;
+	/* BSP via load_ucode_amd_bsp() */
+	if (!cpu)
+		return;
Ok, this is really misleading. Fenghua, what's the reason for
calling
Quoted 25 lines
Quoted 9 lines
load_ucode_ap() on the BSP too?
We have on the one hand:
x86_64_start_kernel
|->load_ucode_bsp
and on the other:
Quoted 9 lines
x86_64_start_kernel
|-> x86_64_start_reservations
    |-> start_kernel
        |-> trap_init
	    |-> cpu_init
	        |-> load_ucode_ap()
so we attempt to load the ucode twice on the BSP.
You are right. Though the second time loading attempt will not find a valid/newer ucode patch to update. So this won't cause a real/serious problem. But we had better to fix this anyway.
Quoted 2 lines
IMO, we should do this in cpu_init:
Quoted 8 lines
Quoted 4 lines
	if (cpu)
		load_ucode_ap();
no?
This check won't work when CPU0 is hot added. So we need to find a better way to fix this.
Maybe need to change the check as follows to take care of CPU0 hot add case?
if ((cpu && system_state == SYSTEM_BOOTING) || (system_state == SYSTEM_RUNNING))
	load_ucode_ap();
Thanks.
-Fenghua

Signature · 1 line

Sent from my mobile phone. Please excuse brevity and lack of formatting.

Borislav Petkov

maintainer

2013-08-08 10:02 UTC #5 lore raw

Quoted 1 line · H. Peter Anvin

How much does this matter?

I know what you're asking :-)

And no, it doesn't really matter as we fail in the patch version check so maybe only a comment will suffice here so that people don't get confused.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

maintainer

2013-08-08 18:28 UTC #6 lore raw

Quoted 8 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
How much does this matter?
I know what you're asking :-)
And no, it doesn't really matter as we fail in the patch version check so maybe only a comment will suffice here so that people don't get confused.

IOW, something like that:

---
From: Borislav Petkov <[email protected]>
Date: Thu, 8 Aug 2013 20:26:24 +0200
Subject: [PATCH] x86, microcode: Clarify loading on the AP

Hold it down why we're calling the AP microcode loading routine on the BSP too, for future reference.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

arch/x86/kernel/cpu/common.c +6 -2

index 25eb2747b063..58d37a27d317 100644
@@ -1226,8 +1226,12 @@ void cpu_init(void)
 	int i;
 
 	/*
-	 * Load microcode on this cpu if a valid microcode is available.
-	 * This is early microcode loading procedure.
+	 * Load microcode on this cpu if a valid microcode is available. This
+	 * is early microcode loading procedure. We execute this on the BSP too
+	 * but since a microcode has been potentially already applied on the
+	 * BSP, we will return prematurely here. It is easier to simply call it
+	 * again than filtering all the possible cases when we're not running
+	 * on the BSP.
 	 */
 	load_ucode_ap();

Signature · 8 lines

1.8.3

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Yu, Fenghua

Reviewed-by

2013-08-09 18:44 UTC #7 lore raw

Quoted 50 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Thursday, August 08, 2013 11:29 AM
Quoted 6 lines · Borislav Petkov
Quoted 1 line · H. Peter Anvin
How much does this matter?
I know what you're asking :-)
And no, it doesn't really matter as we fail in the patch version
check
Quoted 2 lines
so maybe only a comment will suffice here so that people don't get confused.
IOW, something like that:
---
From: Borislav Petkov <[email protected]>
Date: Thu, 8 Aug 2013 20:26:24 +0200
Subject: [PATCH] x86, microcode: Clarify loading on the AP
Hold it down why we're calling the AP microcode loading routine on the BSP too, for future reference.
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/cpu/common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/common.c +1 -2
index 25eb2747b063..58d37a27d317 100644
@@ -1226,8 +1226,12 @@ void cpu_init(void)
 	int i;
 
 	/*
-	 * Load microcode on this cpu if a valid microcode is available.
-	 * This is early microcode loading procedure.
+	 * Load microcode on this cpu if a valid microcode is available.
This
arch/x86/kernel/cpu/common.c +1 -0
+	 * is early microcode loading procedure. We execute this on the
BSP too
arch/x86/kernel/cpu/common.c +1 -0
+	 * but since a microcode has been potentially already applied on
the
arch/x86/kernel/cpu/common.c +1 -0
+	 * BSP, we will return prematurely here. It is easier to simply
call it
arch/x86/kernel/cpu/common.c +1 -0
+	 * again than filtering all the possible cases when we're not
running

arch/x86/kernel/cpu/common.c void cpu_init(void) · line 1236

Quoted 3 lines

arch/x86/kernel/cpu/common.c +1 -0
+	 * on the BSP.
 	 */
 	load_ucode_ap();

Load_ucode_ap() running on BSP during boot time wastes short time on scanning ucode blob. But the time should be very short.

Reviewed-by Fenghua Yu <[email protected]>

��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Borislav Petkov

maintainer

2013-08-08 17:49 UTC #8 lore raw

In reply to Yu, Fenghua

Quoted 8 lines · Fenghua

Quoted 3 lines
This check won't work when CPU0 is hot added. So we need to find a better way to fix this.
Maybe need to change the check as follows to take care of CPU0 hot add case?
if ((cpu && system_state == SYSTEM_BOOTING) || (system_state == SYSTEM_RUNNING))
	load_ucode_ap();

Ok, just for my own understanding - I haven't played with cpu hotadd yet so when you do this, is the hot-added socket containing the BSP not cpu 0 anymore?

Or when you hot-remove the socket containing the BSP, another AP becomes the BSP and preserves its old number?

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Yu, Fenghua

2013-08-09 18:38 UTC #9 lore raw

Quoted 14 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Thursday, August 08, 2013 10:49 AM
Quoted 5 lines · Fenghua
Quoted 3 lines
This check won't work when CPU0 is hot added. So we need to find a better way to fix this.
Maybe need to change the check as follows to take care of CPU0 hot
add case?
Quoted 2 lines
if ((cpu && system_state == SYSTEM_BOOTING) || (system_state ==
SYSTEM_RUNNING))
Quoted 1 line
	load_ucode_ap();

Quoted 8 lines

Ok, just for my own understanding - I haven't played with cpu hotadd yet
so when you do this, is the hot-added socket containing the BSP not cpu 0 anymore?
Or when you hot-remove the socket containing the BSP, another AP becomes
the BSP and preserves its old number?

When BSP is logically hot removed/offlined, there is no BSP any more in Linux, i.e. cpu0 is not in online mask. CPU0 can be logically hot added back again.

Currently kernel doesn't support physically hot remove CPU0 yet.

Thanks.