On 07/08/16 10:59, Fan, Jeff wrote: > 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.
That's a good idea! Maybe we can sneak in some context so that MtrrDebugPrintAllMtrrsWorker() knows it is running on an AP, and omits debug logging. > 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. Sounds great! Thanks! Laszlo > > Thanks! > Jeff > > > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Friday, July 08, 2016 4:38 PM > To: Fan, Jeff; [email protected] > 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 <[email protected]> >> Cc: Feng Tian <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <[email protected]> >> Reviewed-by: Feng Tian <[email protected]> >> --- >> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
