Messages in this thread | | | Subject | RE: [PATCH 3/5] x86, AMD: cleanup: merge common code in early microcode loading | From | "H. Peter Anvin" <> | Date | Wed, 07 Aug 2013 19:22:39 -0700 |
| |
How much does this matter?
"Yu, Fenghua" <fenghua.yu@intel.com> wrote: >> 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
-- Sent from my mobile phone. Please excuse brevity and lack of formatting.
| |