Hi Ruiyu,
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, October 16, 2018 10:27 AM
> To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <ler...@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...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.d...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
