On 10/15/18 04:49, Eric Dong wrote:
> In order to support semaphore related logic, add new definition for it.
>
> Cc: Ruiyu Ni <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <[email protected]>
> ---
> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
