Re: [Patch 0/4] Fix performance issue caused by Set MSR task.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 15, 2018 11:52 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [Patch 0/4] Fix performance issue caused by Set MSR task.
> 
> Hi Eric,
> 
> On 10/15/18 04:49, Eric Dong wrote:
> > 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.
> 
> Do you mean that platforms are responsible for updating their register tables
> in:
> - ACPI_CPU_DATA.PreSmmInitRegisterTable,
> - ACPI_CPU_DATA.RegisterTable
> 
> so that the tables utilize the new Semaphore REGISTER_TYPE as appropriate?

Yes, platform should set MSR in these two tables base on MSR's scope info. Just like if the MSR is core level, this MSR should on been add to the AP which control the related core. 
Also if two MSRs have dependence and they have different scope info, a semaphore should been added between these two MSRs. 

> 
> >
> > 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.
> 
> I don't understand what you mean by "schedule limitation". Are you alluding
> to the upcoming edk2 stable tag (in November), or some other schedule?

Yes, I want to include this change in the upcoming edk2 stable tag. But I can't finish all these changes before it, so for this version, I just duplicate the code.

> 
> >    Will merge the code to
> >    RegisterCpuFeaturesLib and export as an API in phase 2 for this task.
> 
> While I agree that common code (especially complex code like this) should
> belong to libraries, there are platforms that consume PiSmmCpuDxeSmm,
> but don't consume RegisterCpuFeaturesLib in any way.
> 
> Do you plan to add the new function(s) to a RegisterCpuFeaturesLib instance,
> and make PiSmmCpuDxeSmm dependent on RegisterCpuFeaturesLib?

Yes, plan to export one new API in RegisterCpuFeaturesLib to let PiSmmCpuDxeSmm driver to consume it.\
This API used to program the register.

> 
> If so, I think it can work, but then the RegisterCpuFeaturesLib instance in
> question should do nothing at all in the constructor. On platforms that don't
> use this feature at all -- i.e., the Semaphore REGISTER_TYPE --, there should
> be no impact.
> 
> (BTW, DxeRegisterCpuFeaturesLib is currently restricted to DXE_DRIVER
> modules.)
> 

Thanks for your advice. Yes, I have did some POC code to export this API and already met such issue. I also met a dependence issue.
Let's discuss these issue when I do that changes.

> > Extra Notes:
> >   I will send the other patch to set MSR base on scope info and check in it
> before check in
> >   this serial.
> 
> I don't understand. I assume that you are referring to some concrete
> platform (?) where the Semaphore REGISTER_TYPE *must* be used, in order
> to successfully boot (and/or perform S3), if this series is applied.
> 
> What platform is that?

Yes, I used an internal reference platform to  verify the changes.  I have did some change(Set MSR base on scope info) to verify the solution but not finalize the change yet. I checked the boot result and console log to confirmed the code works as expectation. 
When I send this serial changes, I'm not finalize Set MSR base on scope info change, but I want to collect your feedback for this serial as soon as possible (I think you will be the first one who will reply this serial), so I add this note and send out this serial. Now I have finished code change to set MSRs base on its scope info. I will include this patch when I send the  v2 patch for this serial.

> 
> And, if that other patch is indeed a pre-requisite for *this* set (on some
> specific platform anyway), then people on that platform will not be able to
> test this series until you post those patches.
> 
> My point here is that, on that platform, the testing cannot be performed in
> separation, so it's not enough to establish the right dependency order *just*
> before check-in. It should be offered on the list as well.
> 
> Thanks,
> Laszlo
> 
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> >
> > Eric Dong (4):
> >   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/Include/AcpiCpuData.h                   |  23 +-
> >  .../Include/Library/RegisterCpuFeaturesLib.h       |  25 +-
> >  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 324 ++++++++++++---
> >  .../DxeRegisterCpuFeaturesLib.c                    |  71 +++-
> >  .../DxeRegisterCpuFeaturesLib.inf                  |   3 +
> >  .../PeiRegisterCpuFeaturesLib.c                    |  55 ++-
> >  .../PeiRegisterCpuFeaturesLib.inf                  |   1 +
> >  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  51 ++-
> >  .../RegisterCpuFeaturesLib.c                       | 452 ++++++++++++++++++---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  | 316 +++++++-------
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              |   3 -
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |   3 +-
> >  12 files changed, 1063 insertions(+), 264 deletions(-)
> >