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

["Dong, Eric" <eric.dong@intel.com>]

Hi Ruiyu,

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, October 16, 2018 10:27 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [Patch 1/4] UefiCpuPkg/Include/AcpiCpuData.h: Add
> Semaphore related Information.
> 
> 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.
> 

Yes.

> >   } 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?

Will add more comments in the next version.

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

CoreCount means the max core count in the CPU.  ThreadCount means max thread count in the CPU. Will add comments in next version change.

> 
> 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?

Yes.

> How about using name ValidCoreCountPerPackage?
> How about using MaxCoreCount/MaxThreadCount for CoreCount and
> ThreadCount?
> 

Ok, will use these names 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.
> > +  //
> > +  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.
> 

Yes,  update code in next version.

> >   } ACPI_CPU_DATA;
> >
> >   #endif
> >
> 
> 
> --
> Thanks,
> Ray