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

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

Thanks for the detailed comments.
Very minor comments in below.

On 10/17/2018 10:16 AM, Eric Dong wrote:
> 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.
> 
> 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 | 45 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
> index 9e51145c08..f1439dcf9a 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -22,9 +22,42 @@ typedef enum {
>     Msr,
>     ControlRegister,
>     MemoryMapped,
> -  CacheControl
> +  CacheControl,

Suggest to leave a blank line here. This can make the comment block in 
below looks closer to the field (Semaphore).
Similar to all the comment blocks below.
> +  //
> +  // Semaphore type used to control the execute sequence of the Msr.
> +  // It will be insert between two Msr which has execute dependence.
> +  //
> +  Semaphore
>   } REGISTER_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 fild is an pointer type which point to an array.

"This field points to an array."?

> +  // This array used to save the valid cores in different packages in this CPU.
> +  // The array count is the package number in this CPU.
"This array saves valid core count (type UINT32) of each package."
"The array has PackageCount elements."
> +  //
> +  EFI_PHYSICAL_ADDRESS        ValidCoresPerPackages;
> +} CPU_STATUS_INFORMATION;
> +
>   //
>   // Element of register table entry
>   //
> @@ -147,6 +180,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 in this CPU.
Suggest to remove "in this CPU". Because for a multiple sockets system, 
there are multiple CPUs.
> +  // The array count is the AP count in this CPU.
> +  //
> +  EFI_PHYSICAL_ADDRESS  ApLocation;
>   } ACPI_CPU_DATA;
>   
>   #endif
> 


-- 
Thanks,
Ray