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

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

On 10/15/2018 10:49 AM, Eric Dong wrote:
> In order to support semaphore related logic, add new definition for it.
> 
> 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 | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
> index 9e51145c08..b3cf2f664a 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>   #ifndef _ACPI_CPU_DATA_H_
>   #define _ACPI_CPU_DATA_H_
>   
> +#include <Protocol/MpService.h>
> +
>   //
>   // Register types in register table
>   //
> @@ -22,9 +24,20 @@ typedef enum {
>     Msr,
>     ControlRegister,
>     MemoryMapped,
> -  CacheControl
> +  CacheControl, > +  Semaphore
I assume the REGISTER_TYPE definition will be move to internal 
(non-public) in phase 2.

>   } REGISTER_TYPE;
>   
> +//
> +// CPU information.
> +//
> +typedef struct {
> +  UINT32        PackageCount;             // Packages in this CPU.
> +  UINT32        CoreCount;                // Max Core count in the packages.
> +  UINT32        ThreadCount;              // MAx thread count in the cores.
> +  UINT32        *ValidCoresInPackages;    // Valid cores in each package.

Can you please add more comments to describe each field above?
PackageCount is easy to understand.
But CoreCount is not. Maybe different packages have different number of 
cores. In this case, what value will CoreCount be?
Similar question to ThreadCount.

What does ValidCoresInPackages mean? Does it hold the valid (non-dead) 
core numbers for each package? So it's a UINT32 array with PackageCount 
elements?
How about using name ValidCoreCountPerPackage?
How about using MaxCoreCount/MaxThreadCount for CoreCount and ThreadCount?

> +} CPU_STATUS_INFORMATION;
> +
>   //
>   // Element of register table entry
>   //
> @@ -147,6 +160,14 @@ typedef struct {
>     // provided.
>     //
>     UINT32                ApMachineCheckHandlerSize;
> +  //
> +  // CPU information which is required when set the register table.
> +  //
> +  CPU_STATUS_INFORMATION     CpuStatus;
> +  //
> +  // Location info for each ap.
> +  //
> +  EFI_CPU_PHYSICAL_LOCATION  *ApLocation;

Please use EFI_PHYSICAL_ADDRESS for ApLocation. It's ok now. But if 
there are more fields below ApLocation, the offset of those fields 
differs between PEI and DXE. That will cause bugs.

>   } ACPI_CPU_DATA;
>   
>   #endif
> 


-- 
Thanks,
Ray