On 07/12/16 13:58, Fan, Jeff wrote: > 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.
Sounds good to me, thanks! Laszlo > > -----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
