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

[Laszlo Ersek <lersek@redhat.com>]

On 10/19/18 04:06, 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>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Include/AcpiCpuData.h | 67 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
> index 9e51145c08..005d48d7ca 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -22,9 +22,60 @@ 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.
> +  //
> +  // If the platform does not support MSR setting at S3 resume, and
> +  // therefore it doesn't need the dependency semaphores, it should set
> +  // this field to 0.
> +  //
> +  EFI_PHYSICAL_ADDRESS        ValidCoreCountPerPackage;
> +} CPU_STATUS_INFORMATION;
> +
>  //
>  // Element of register table entry
>  //
> @@ -147,6 +198,20 @@ 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.
> +  //
> +  // If the platform does not support MSR setting at S3 resume, and
> +  // therefore it doesn't need the dependency semaphores, it should set
> +  // this field to 0.
> +  //
> +  EFI_PHYSICAL_ADDRESS  ApLocation;
>  } ACPI_CPU_DATA;
>  
>  #endif
> 

Looks good, thank you! My R-b stands.
Laszlo