On 10/18/18 09:34, 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...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.d...@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: Laszlo Ersek <ler...@redhat.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
