Laszlo, Thanks your feedback and provided the history on MTRRs sync code.
DEBUG () running on Aps is a common issue to be avoided. For MtrrLib, DEBUG() is using DEBUG_CACHE for debug purpose only. Usually, MTRRs setting should be same between BSP/Aps. Dump MTRRs are enough for BSP. Maybe, we could enhance MtrrLib to avoid DEBUG () running on Aps. For the case 2) in StartupAllAPs() call in InitializeMpSupport(), it will be handled in our refactor on MP support between CpuMpPei and CpuDxe driver as Mike mentioned. I wish we could sent out MP Initial Lib support in a couple of weeks. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, July 08, 2016 4:38 PM To: Fan, Jeff; edk2-de...@ml01.01.org Cc: Kinney, Michael D; Tian, Feng Subject: Re: [Patch] UefiCpuPkg/CpuDxe: StartupAllAPs in parallel mode Jeff, On 07/08/16 09:45, Jeff Fan wrote: > SetMemoryAttributes() will sync BSP's MTRRs settings to all APs by > StartupAllAPs service in serial mode. It may caused much performance > impact if there are too much processors in system. This update is to > invoke StartupAllAps in parallel mode. IA32 SDM does suggest to program MTRRs > in parallel mode. > > Cc: Michael Kinney <michael.d.kin...@intel.com> > Cc: Feng Tian <feng.t...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff....@intel.com> > Reviewed-by: Feng Tian <feng.t...@intel.com> > --- > UefiCpuPkg/CpuDxe/CpuDxe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index daf97bd..78b2c88 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -1,7 +1,7 @@ > /** @file > CPU DXE Module. > > - Copyright (c) 2008 - 2013, Intel Corporation. All rights > reserved.<BR> > + Copyright (c) 2008 - 2016, Intel Corporation. All rights > + reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license > may be found at @@ -422,7 +422,7 @@ CpuSetMemoryAttributes ( > MpStatus = MpService->StartupAllAPs ( > MpService, // This > SetMtrrsFromBuffer, // Procedure > - TRUE, // SingleThread > + FALSE, // SingleThread > NULL, // WaitEvent > 0, // TimeoutInMicrosecsond > &MtrrSettings, // ProcedureArgument > (1) This change looks "obvious" on the surface, and to be honest, when looking at your patch now, I couldn't tell for a minute what I was thinking when I set SingleThread=TRUE, in commit 94941c8853448. Digging down a bit, I believe I remember now. The APs execute the following call chain: SetMtrrsFromBuffer() [UefiCpuPkg/CpuDxe/CpuMp.c] MtrrSetAllMtrrs() [UefiCpuPkg/Library/MtrrLib/MtrrLib.c] MtrrDebugPrintAllMtrrs() [UefiCpuPkg/Library/MtrrLib/MtrrLib.c] MtrrDebugPrintAllMtrrsWorker() [UefiCpuPkg/Library/MtrrLib/MtrrLib.c] I believe my reason for SingleThread=TRUE may have been that MtrrDebugPrintAllMtrrsWorker() is not safe for parallel execution (with the many DEBUG()s in it). I agree this would be a nice improvement, both for performance and for compliance with the SDM -- if only we could make SetMtrrsFromBuffer() thread-safe, top to bottom. (In my other patchset that you just reviewed (thanks for that BTW!), I also verified that the WriteFeatureControl() AP procedure would be safe for parallel execution. It is: it calls only AsmWriteMsr64(), which compiles to a single WRMSR instruction. But here, SetMtrrsFromBuffer() is much more complex, top to bottom.) (2) If we end up modifying SingleThread to FALSE, then this is not the only location that should be updated -- there's another StartupAllAPs() call in InitializeMpSupport(). (See again commit 94941c8853448.) Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
