Re: [Patch v3 1/6] UefiCpuPkg/Include/AcpiCpuData.h: Add Semaphore related Information.

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 10/18/2018 3:34 PM, Eric Dong wrote:
> 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
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray