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

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

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, October 16, 2018 12:03 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> 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 <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.

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