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
