Eric, Thanks for the comments. Attach the updated patch, and you may help push it if it is ok. Thanks, Star > -----Original Message----- > From: Dong, Eric > Sent: Tuesday, July 16, 2019 3:24 PM > To: Zeng, Star ; devel@edk2.groups.io > Cc: Laszlo Ersek ; Ni, Ray ; Kumar, > Chandana C ; Li, Kevin Y > > Subject: RE: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin > code > > Hi Star, > > Suggest to add some code comments for the behavior, detail see the inline > comments. > with these comments, Reviewed-by: Eric Dong > > Thanks, > Eric > > -----Original Message----- > > From: Zeng, Star > > Sent: Friday, July 12, 2019 6:13 PM > > To: devel@edk2.groups.io > > Cc: Zeng, Star ; Laszlo Ersek > > ; Dong, Eric ; Ni, Ray > > ; Kumar, Chandana C ; > > Li, Kevin Y > > Subject: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1961 > > Enhance Ppin code to enable and unlock for TRUE State, and disable and > > lock for FALSE State. > > Note: enable and lock could not be set both. > > > > Cc: Laszlo Ersek > > Cc: Eric Dong > > Cc: Ray Ni > > Cc: Chandana Kumar > > Cc: Kevin Li > > Signed-off-by: Star Zeng > > --- > > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 +++++ > > .../CpuCommonFeaturesLib.c | 2 +- > > .../Library/CpuCommonFeaturesLib/Ppin.c | 65 +++++++++++++++---- > > 3 files changed, 70 insertions(+), 12 deletions(-) > > > > diff --git > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > index 9e784e916a85..8406c6c1619f 100644 > > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > +++ > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > @@ -863,6 +863,21 @@ FeatureControlGetConfigData ( > > IN UINTN NumberOfProcessors > > ); > > > > +/** > > + Prepares for the data used by CPU feature detection and initialization. > > + > > + @param[in] NumberOfProcessors The number of CPUs in the platform. > > + > > + @return Pointer to a buffer of CPU related configuration data. > > + > > + @note This service could be called by BSP only. > > +**/ > > +VOID * > > +EFIAPI > > +PpinGetConfigData ( > > + IN UINTN NumberOfProcessors > > + ); > > + > > /** > > Detects if Protected Processor Inventory Number feature supported > > on current > > processor. > > diff --git > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > index 7cc692efb649..fd43b8d66290 100644 > > --- > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > +++ > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > @@ -203,7 +203,7 @@ CpuCommonFeaturesLibConstructor ( > > if (IsCpuFeatureSupported (CPU_FEATURE_PPIN)) { > > Status = RegisterCpuFeature ( > > "PPIN", > > - NULL, > > + PpinGetConfigData, > > PpinSupport, > > PpinInitialize, > > CPU_FEATURE_PPIN, > > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > index e8a4de8dcf60..8067cf44d015 100644 > > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > @@ -8,6 +8,28 @@ > > > > #include "CpuCommonFeatures.h" > > > > +/** > > + Prepares for the data used by CPU feature detection and initialization. > > + > > + @param[in] NumberOfProcessors The number of CPUs in the platform. > > + > > + @return Pointer to a buffer of CPU related configuration data. > > + > > + @note This service could be called by BSP only. > > +**/ > > +VOID * > > +EFIAPI > > +PpinGetConfigData ( > > + IN UINTN NumberOfProcessors > > + ) > > +{ > > + VOID *ConfigData; > > + > > + ConfigData = AllocateZeroPool (sizeof > > +(MSR_IVY_BRIDGE_PPIN_CTL_REGISTER) * NumberOfProcessors); > > + ASSERT (ConfigData != NULL); > > + return ConfigData; > > +} > > + > > /** > > Detects if Protected Processor Inventory Number feature supported > > on current > > processor. > > @@ -34,6 +56,7 @@ PpinSupport ( > > ) > > { > > MSR_IVY_BRIDGE_PLATFORM_INFO_1_REGISTER PlatformInfo; > > + MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *MsrPpinCtrl; > > > > if ((CpuInfo->DisplayFamily == 0x06) && > > ((CpuInfo->DisplayModel == 0x3E) || // Xeon E5 V2 > > @@ -47,7 +70,12 @@ PpinSupport ( > > // Check whether platform support this feature. > > // > > PlatformInfo.Uint64 = AsmReadMsr64 > > (MSR_IVY_BRIDGE_PLATFORM_INFO_1); > > - return (PlatformInfo.Bits.PPIN_CAP != 0); > > + if (PlatformInfo.Bits.PPIN_CAP != 0) { > > + MsrPpinCtrl = (MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *) ConfigData; > > + ASSERT (MsrPpinCtrl != NULL); > > + MsrPpinCtrl[ProcessorNumber].Uint64 = AsmReadMsr64 > > (MSR_IVY_BRIDGE_PPIN_CTL); > > + return TRUE; > > + } > > } > > > > return FALSE; > > @@ -73,6 +101,7 @@ PpinSupport ( > > @retval RETURN_DEVICE_ERROR Device can't change state because it > > has been > > locked. > > > > + @note This service could be called by BSP only. > > **/ > > RETURN_STATUS > > EFIAPI > > @@ -83,16 +112,18 @@ PpinInitialize ( > > IN BOOLEAN State > > ) > > { > > - MSR_IVY_BRIDGE_PPIN_CTL_REGISTER MsrPpinCtrl; > > + MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *MsrPpinCtrl; > > + > > + MsrPpinCtrl = (MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *) ConfigData; > > + ASSERT (MsrPpinCtrl != NULL); > > > > // > > - // Check whether device already lock this register. > > - // If already locked, just base on the request state and > > + // Check whether processor already lock this register. > > + // If already locked, just based on the request state and > > // the current state to return the status. > > // > > - MsrPpinCtrl.Uint64 = AsmReadMsr64 (MSR_IVY_BRIDGE_PPIN_CTL); > > - if (MsrPpinCtrl.Bits.LockOut != 0) { > > - return MsrPpinCtrl.Bits.Enable_PPIN == State ? RETURN_SUCCESS : > > RETURN_DEVICE_ERROR; > > + if (MsrPpinCtrl[ProcessorNumber].Bits.LockOut != 0) { > > + return MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN == State ? > > + RETURN_SUCCESS : RETURN_DEVICE_ERROR; > > } > > > > // > > @@ -106,13 +137,25 @@ PpinInitialize ( > > return RETURN_SUCCESS; > > } > > > > - CPU_REGISTER_TABLE_WRITE_FIELD ( > > + if (State) { > > + // > > + // Enable and Unlock. > > + // > > + MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN = 1; > > + MsrPpinCtrl[ProcessorNumber].Bits.LockOut = 0; } else { > > 1. I suggest to add some comments about why unlock & enable need to set > at the same time. > > > > + // > > + // Disable and Lock. > > + // > > + MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN = 0; > > + MsrPpinCtrl[ProcessorNumber].Bits.LockOut = 1; } > > > 2. Same as comments 1. > > Thanks, > Eric > > + > > + CPU_REGISTER_TABLE_WRITE64 ( > > ProcessorNumber, > > Msr, > > MSR_IVY_BRIDGE_PPIN_CTL, > > - MSR_IVY_BRIDGE_PPIN_CTL_REGISTER, > > - Bits.Enable_PPIN, > > - (State) ? 1 : 0 > > + MsrPpinCtrl[ProcessorNumber].Uint64 > > ); > > > > return RETURN_SUCCESS; > > -- > > 2.21.0.windows.1