Laszlo, I think of it. We could just remove the MtrrDebugPrintAllMtrrs() call from MtrrSetAllMtrrs() implementation in MtrrLib.
The reason is that Aps' MTRR settings should be always same with the BPS's. BSP will set the MTRRs setting by MtrrSetMemoryAttribute() or other APIs. And Aps will always sync the whole MTRRs setting by MtrrSetAllMtrrs(). MtrrSetMemoryAttribute() or other APIs will display all MTRRs setting after MTRRs setting changed. That means we could get the latest MTRRs setting when BSP updated MTRR setting. We needn't MtrrSetAllMtrrs() to dump all MTRRs setting again. The MTRRs output message is duplicated. After removed MtrrDebugPrintAllMtrrs() call from MtrrSetAllMtrrs() and enabled EFI_D_CACHE flag. I can see the updated MTRR setting and no debug message output from Aps. I could create the patch on MtrrLib to address this issue. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Friday, July 08, 2016 5:45 PM To: Fan, Jeff; [email protected] Cc: Kinney, Michael D; Tian, Feng Subject: Re: [Patch] UefiCpuPkg/CpuDxe: StartupAllAPs in parallel mode 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
