Eric, Can you post your changes to github yours mirror repo? I found #3/6 cannot be applied to my code properly.
Thanks/Ray > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, October 17, 2018 10:16 AM > To: [email protected] > Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek <[email protected]> > Subject: [Patch v2 0/6] Fix performance issue caused by Set MSR task. > > V2 changes include: > 1. Include the change for CpuCommonFeaturesLib which used to set MSR > base on its scope info. > 2. Include the change for CpuS3DataDxe driver which also handle the > AcpiCpuData data. > 3. Update code base on feedback for V1 changes. > > V1 changes include: > In a system which has multiple cores, current set register value task costs > huge times. > After investigation, current set MSR task costs most of the times. Current > logic uses SpinLock to let set MSR task as an single thread task for all > cores. > Because MSR has scope attribute which may cause GP fault if multiple APs > set MSR at the same time, current logic use an easiest solution (use SpinLock) > to avoid this issue, but it will cost huge times. > > In order to fix this performance issue, new solution will set MSRs base on > their scope attribute. After this, the SpinLock will not needed. Without > SpinLock, new issue raised which is caused by MSR dependence. For > example, MSR A depends on MSR B which means MSR A must been set after > MSR B has been set. Also MSR B is package scope level and MSR A is thread > scope level. If system has multiple threads, Thread 1 needs to set the thread > level MSRs and thread 2 needs to set thread and package level MSRs. Set > MSRs task for thread 1 and thread 2 like below: > > Thread 1 Thread 2 > MSR B N Y > MSR A Y Y > > If driver don't control execute MSR order, for thread 1, it will execute MSR A > first, but at this time, MSR B not been executed yet by thread 2. system may > trig exception at this time. > > In order to fix the above issue, driver introduces semaphore logic to control > the MSR execute sequence. For the above case, a semaphore will be add > between MSR A and B for all threads. Semaphore has scope info for it. The > possible scope value is core or package. > For each thread, when it meets a semaphore during it set registers, it will 1) > release semaphore (+1) for each threads in this core or package(based on > the scope info for this > semaphore) 2) acquire semaphore (-1) for all the threads in this core or > package(based on the scope info for this semaphore). With these two steps, > driver can control MSR sequence. Sample code logic like below: > > // > // First increase semaphore count by 1 for processors in this package. > // > for (ProcessorIndex = 0; ProcessorIndex < PackageThreadsCount ; > ProcessorIndex ++) { > LibReleaseSemaphore ((UINT32 *) &SemaphorePtr[PackageOffset + > ProcessorIndex]); > } > // > // Second, check whether the count has reach the check number. > // > for (ProcessorIndex = 0; ProcessorIndex < ValidApCount; ProcessorIndex ++) > { > LibWaitForSemaphore (&SemaphorePtr[ApOffset]); > } > > Platform Requirement: > 1. This change requires register MSR setting base on MSR scope info. If still > register MSR > for all threads, exception may raised. > > Known limitation: > 1. Current CpuFeatures driver supports DXE instance and PEI instance. But > semaphore logic > requires Aps execute in async mode which is not supported by PEI driver. > So CpuFeature > PEI instance not works after this change. We plan to support async mode > for PEI in phase > 2 for this task. > 2. Current execute MSR task code in duplicated in PiSmmCpuDxeSmm driver > and > RegisterCpuFeaturesLib library because the schedule limitation. Will merge > the code to > RegisterCpuFeaturesLib and export as an API in phase 2 for this task. > > Cc: Ruiyu Ni <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <[email protected]> > > > Eric Dong (6): > UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information. > UefiCpuPkg/RegisterCpuFeaturesLib.h: Add new dependence types. > UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore > type. > UefiCpuPkg/PiSmmCpuDxeSmm: Add logic to support semaphore type. > UefiCpuPkg/CpuS3DataDxe: Keep old data if value already existed. > UefiCpuPkg/CpuCommonFeaturesLib: Register MSR base on scope Info. > > UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 2 + > UefiCpuPkg/Include/AcpiCpuData.h | 45 +- > .../Include/Library/RegisterCpuFeaturesLib.h | 25 +- > UefiCpuPkg/Library/CpuCommonFeaturesLib/C1e.c | 8 + > UefiCpuPkg/Library/CpuCommonFeaturesLib/Eist.c | 12 + > .../Library/CpuCommonFeaturesLib/ExecuteDisable.c | 10 + > .../Library/CpuCommonFeaturesLib/FastStrings.c | 12 + > .../Library/CpuCommonFeaturesLib/FeatureControl.c | 38 ++ > .../CpuCommonFeaturesLib/LimitCpuIdMaxval.c | 14 + > .../Library/CpuCommonFeaturesLib/MachineCheck.c | 38 ++ > .../Library/CpuCommonFeaturesLib/MonitorMwait.c | 15 + > .../Library/CpuCommonFeaturesLib/PendingBreak.c | 11 + > UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c | 11 + > .../Library/CpuCommonFeaturesLib/ProcTrace.c | 11 + > UefiCpuPkg/Library/CpuCommonFeaturesLib/X2Apic.c | 10 + > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 438 > ++++++++++++++++--- > .../DxeRegisterCpuFeaturesLib.c | 71 ++- > .../DxeRegisterCpuFeaturesLib.inf | 3 + > .../PeiRegisterCpuFeaturesLib.c | 55 ++- > .../PeiRegisterCpuFeaturesLib.inf | 1 + > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 61 ++- > .../RegisterCpuFeaturesLib.c | 484 > ++++++++++++++++++--- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 406 +++++++++++------ > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 3 - > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 3 +- > 25 files changed, 1505 insertions(+), 282 deletions(-) > > -- > 2.15.0.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
