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

[Laszlo Ersek <lersek@redhat.com>]

On 10/15/18 04:49, 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(-)

(1) If it's possible, I suggest moving the (very nice) description from
the 0/4 cover letter to this patch. The cover letter is not captured in
the git commit history.

I don't insist, but it would be a nice touch, IMO.

> 
> 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
>  } REGISTER_TYPE;
>  
> +//
> +// CPU information.
> +//
> +typedef struct {
> +  UINT32        PackageCount;             // Packages in this CPU.

(2) Is it possible to have multiple packages in a single CPU? If not,
then please clean up the comment.

Did you perhaps mean "number of sockets in the system"?

> +  UINT32        CoreCount;                // Max Core count in the packages.
> +  UINT32        ThreadCount;              // MAx thread count in the cores.

(3) The word "MAx" should be "Max", I think.

> +  UINT32        *ValidCoresInPackages;    // Valid cores in each package.

(4) Is it possible to document the structure of this array (?) in some
detail? Other parts of "UefiCpuPkg/Include/AcpiCpuData.h" are very well
documented.

> +} 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.

(5) This header file spells "AP" in upper case elsewhere.

> +  //
> +  EFI_CPU_PHYSICAL_LOCATION  *ApLocation;

(6) Is this supposed to be an array? If so, what is the structure of the
array? What is the size?

(7) This is the first field in ACPI_CPU_DATA that has pointer type.
Other pointers are represented as EFI_PHYSICAL_ADDRESS.

What justifies this difference?

>  } ACPI_CPU_DATA;
>  
>  #endif
> 

(8) "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" will zero-fill the new fields.
Is that safe?

Thanks
Laszlo