Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:[email protected]]
> Sent: Tuesday, October 16, 2018 12:03 AM
> To: Dong, Eric <[email protected]>; [email protected]
> Cc: Ni, Ruiyu <[email protected]>
> Subject: Re: [Patch 1/4] UefiCpuPkg/Include/AcpiCpuData.h: Add
> Semaphore related Information.
>
> 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.
Code change for this patch can't show all the information for the description.
I add this description in 3/4 change.
But this change is the first one for this serial, I think it's ok to add
description here. Will add it in V2 change.
>
> >
> > 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"?
Yes, I means sockets in the system, i think socket == package. Just like below
definition in MdePkg\Include\Protocol\MpService.h file, it use package instead
of socket.
///
/// Structure that describes the pyhiscal location of a logical CPU.
///
typedef struct {
///
/// Zero-based physical package number that identifies the cartridge
of the processor.
///
UINT32 Package;
///
/// Zero-based physical core number within package of the processor.
///
UINT32 Core;
///
/// Zero-based logical thread number within core of the processor.
///
UINT32 Thread;
} EFI_CPU_PHYSICAL_LOCATION;
>
> > + 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.
Yes, will update it in next version.
>
> > + 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.
Yes, will add description in next version.
>
> > +} 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.
Ok, will update it in next version
>
> > + //
> > + 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?
Yes, it's point to an array. Will add comments in this definition in next
version.
>
> (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?
Yes, here I should use EFI_PHYSICAL_ADDRESS instead of pointer type. Will
update it in my next change.
> > } ACPI_CPU_DATA;
> >
> > #endif
> >
>
> (8) "UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c" will zero-fill the new fields.
> Is that safe?
It's not safe, I missed code change in CpuS3DataDxe, it should keep these data
if OldAcpiCpuData already exist. Will update it in the next version.
>
> Thanks
> Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
