Messages in this thread | | | From | "Yu, Fenghua" <> | Subject | RE: [PATCH 3/5] x86, AMD: cleanup: merge common code in early microcode loading | Date | Wed, 7 Aug 2013 23:34:01 +0000 |
| |
> From: Yu, Fenghua > Sent: Wednesday, August 07, 2013 3:46 PM > > > From: Borislav Petkov [mailto:bp@alien8.de] > > 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 <just.for.lkml@googlemail.com> > > > > > > --- 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
| |