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]>
--- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22 06:22:32.000000000 +0200
+++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23 20:00:04.889508712 +0200
@@ -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();
}
On Tue, Jul 23, 2013 at 11:00:26PM +0200, Torsten Kaiser wrote:
> 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]>
>
> --- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22 06:22:32.000000000 +0200
> +++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23 20:00:04.889508712 +0200
> @@ -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;
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?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
> From: Borislav Petkov [mailto:[email protected]]
> Sent: Wednesday, July 24, 2013 6:33 AM
> On Tue, Jul 23, 2013 at 11:00:26PM +0200, Torsten Kaiser wrote:
> > 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]>
> >
> > --- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
> 06:22:32.000000000 +0200
> > +++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
> 20:00:04.889508712 +0200
> > @@ -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;
>
> 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.
>
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.
> 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?
> From: Yu, Fenghua
> Sent: Wednesday, August 07, 2013 3:46 PM
>
> > From: Borislav Petkov [mailto:[email protected]]
> > Sent: Wednesday, July 24, 2013 6:33 AM
> > On Tue, Jul 23, 2013 at 11:00:26PM +0200, Torsten Kaiser wrote:
> > > 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]>
> > >
> > > --- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
> > 06:22:32.000000000 +0200
> > > +++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
> > 20:00:04.889508712 +0200
> > > @@ -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;
> >
> > 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.
> >
>
> 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.
>
> > 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.
>
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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
How much does this matter?
"Yu, Fenghua" <[email protected]> wrote:
>> From: Yu, Fenghua
>> Sent: Wednesday, August 07, 2013 3:46 PM
>>
>> > From: Borislav Petkov [mailto:[email protected]]
>> > Sent: Wednesday, July 24, 2013 6:33 AM
>> > On Tue, Jul 23, 2013 at 11:00:26PM +0200, Torsten Kaiser wrote:
>> > > 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]>
>> > >
>> > > --- a/arch/x86/kernel/microcode_amd_early.c 2013-07-22
>> > 06:22:32.000000000 +0200
>> > > +++ b/arch/x86/kernel/microcode_amd_early.c 2013-07-23
>> > 20:00:04.889508712 +0200
>> > > @@ -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;
>> >
>> > 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.
>> >
>>
>> 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.
>>
>> > 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.
>>
>
>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
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
On Wed, Aug 07, 2013 at 07:22:39PM -0700, H. Peter Anvin wrote:
> 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.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Aug 07, 2013 at 11:34:01PM +0000, Yu, Fenghua wrote:
> > 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.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, Aug 08, 2013 at 12:02:38PM +0200, Borislav Petkov wrote:
> On Wed, Aug 07, 2013 at 07:22:39PM -0700, H. Peter Anvin wrote:
> > 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(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 25eb2747b063..58d37a27d317 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -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();
--
1.8.3
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
> From: Borislav Petkov [mailto:[email protected]]
> Sent: Thursday, August 08, 2013 10:49 AM
> On Wed, Aug 07, 2013 at 11:34:01PM +0000, Yu, Fenghua wrote:
> > > 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?
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.
-Fenghua
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> From: Borislav Petkov [mailto:[email protected]]
> Sent: Thursday, August 08, 2013 11:29 AM
> On Thu, Aug 08, 2013 at 12:02:38PM +0200, Borislav Petkov wrote:
> > On Wed, Aug 07, 2013 at 07:22:39PM -0700, H. Peter Anvin wrote:
> > > 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(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c
> b/arch/x86/kernel/cpu/common.c
> index 25eb2747b063..58d37a27d317 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -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();
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?