public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Eric Dong <eric.dong@intel.com>
To: edk2-devel@lists.01.org
Cc: Ruiyu Ni <ruiyu.ni@intel.com>, Laszlo Ersek <lersek@redhat.com>
Subject: [Patch v3 1/6] UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information.
Date: Thu, 18 Oct 2018 15:34:43 +0800	[thread overview]
Message-ID: <20181018073448.11496-2-eric.dong@intel.com> (raw)
In-Reply-To: <20181018073448.11496-1-eric.dong@intel.com>

v3 changes:
1. Move CPU_FEATURE_DEPENDENCE_TYPE definition here from RegisterCpuFeaturesLib.h file.
2. Add Invalid type for REGISTER_TYPE which will be used in code.

v2 changes:
1. Add more description about why we do this change.
2. Change structure field type from pointer to EFI_PHYSICAL_ADDRESS because it will
   be share between PEI and DXE.

v1 Changes:
In order to support semaphore related logic, add new definition for it.

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.

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>
---
 UefiCpuPkg/Include/AcpiCpuData.h | 59 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index 9e51145c08..9fd87c1a8d 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -22,9 +22,56 @@ typedef enum {
   Msr,
   ControlRegister,
   MemoryMapped,
-  CacheControl
+  CacheControl,
+
+  //
+  // Semaphore type used to control the execute sequence of the Msr.
+  // It will be insert between two Msr which has execute dependence.
+  //
+  Semaphore,
+  InvalidReg
 } REGISTER_TYPE;
 
+//
+// Describe the dependency type for different features.
+// The value set to CPU_REGISTER_TABLE_ENTRY.Value when the REGISTER_TYPE is Semaphore.
+//
+typedef enum {
+  NoneDepType,
+  ThreadDepType,
+  CoreDepType,
+  PackageDepType,
+  InvalidDepType
+} CPU_FEATURE_DEPENDENCE_TYPE;
+
+//
+// CPU information.
+//
+typedef struct {
+  //
+  // Record the package count in this CPU.
+  //
+  UINT32                      PackageCount;
+  //
+  // Record the max core count in this CPU.
+  // Different packages may have different core count, this value
+  // save the max core count in all the packages.
+  //
+  UINT32                      MaxCoreCount;
+  //
+  // Record the max thread count in this CPU.
+  // Different cores may have different thread count, this value
+  // save the max thread count in all the cores.
+  //
+  UINT32                      MaxThreadCount;
+  //
+  // This field points to an array.
+  // This array saves valid core count (type UINT32) of each package.
+  // The array has PackageCount elements.
+  //
+  EFI_PHYSICAL_ADDRESS        ValidCoreCountPerPackage;
+} CPU_STATUS_INFORMATION;
+
 //
 // Element of register table entry
 //
@@ -147,6 +194,16 @@ typedef struct {
   // provided.
   //
   UINT32                ApMachineCheckHandlerSize;
+  //
+  // CPU information which is required when set the register table.
+  //
+  CPU_STATUS_INFORMATION     CpuStatus;
+  //
+  // Location info for each AP.
+  // It points to an array which saves all APs location info.
+  // The array count is the AP count in this CPU.
+  //
+  EFI_PHYSICAL_ADDRESS  ApLocation;
 } ACPI_CPU_DATA;
 
 #endif
-- 
2.15.0.windows.1



  reply	other threads:[~2018-10-18  7:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-18  7:34 [Patch 0/6] Fix performance issue caused by Set MSR task Eric Dong
2018-10-18  7:34 ` Eric Dong [this message]
2018-10-18  8:07   ` [Patch v3 1/6] UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information Ni, Ruiyu
2018-10-18 16:20   ` Laszlo Ersek
2018-10-18  7:34 ` [Patch v3 2/6] UefiCpuPkg/RegisterCpuFeaturesLib.h: Add new dependence types Eric Dong
2018-10-18  8:08   ` Ni, Ruiyu
2018-10-18  7:34 ` [Patch v3 3/6] UefiCpuPkg/RegisterCpuFeaturesLib: Add logic to support semaphore type Eric Dong
2018-10-18  8:19   ` Ni, Ruiyu
2018-10-18  7:34 ` [Patch v3 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: " Eric Dong
2018-10-18  8:20   ` Ni, Ruiyu
2018-10-18 17:58   ` Laszlo Ersek
2018-10-18  7:34 ` [Patch v3 5/6] UefiCpuPkg/CpuS3DataDxe: Keep old data if value already existed Eric Dong
2018-10-18 16:46   ` Laszlo Ersek
2018-10-18  7:34 ` [Patch v3 6/6] UefiCpuPkg/CpuCommonFeaturesLib: Register MSR base on scope Info Eric Dong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181018073448.11496-2-eric.dong@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox