[edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Zeng, Star posted 1 patch 1 week ago
Failed in applying to current master (apply log)
.../CpuCommonFeaturesLib/CpuCommonFeatures.h  | 15 +++++
.../CpuCommonFeaturesLib.c                    |  2 +-
.../Library/CpuCommonFeaturesLib/Ppin.c       | 65 +++++++++++++++----
3 files changed, 70 insertions(+), 12 deletions(-)

[edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Posted by Zeng, Star 1 week ago
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 <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Kevin Li <kevin.y.li@intel.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 .../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 {
+    //
+    // Disable and Lock.
+    //
+    MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN = 0;
+    MsrPpinCtrl[ProcessorNumber].Bits.LockOut = 1;
+  }
+
+  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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43653): https://edk2.groups.io/g/devel/message/43653
Mute This Topic: https://groups.io/mt/32440401/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Posted by Dong, Eric 4 days ago
Hi Star,

Suggest to add some code comments for the behavior, detail see the inline comments.
with these comments, Reviewed-by: Eric Dong <eric.dong@intel.com>

Thanks,
Eric
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, July 12, 2019 6:13 PM
> To: devel@edk2.groups.io
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> <kevin.y.li@intel.com>
> 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 <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Kevin Li <kevin.y.li@intel.com>
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  .../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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43768): https://edk2.groups.io/g/devel/message/43768
Mute This Topic: https://groups.io/mt/32440401/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Posted by Zeng, Star 4 days ago
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 <star.zeng@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> <kevin.y.li@intel.com>
> 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 <eric.dong@intel.com>
> 
> Thanks,
> Eric
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Friday, July 12, 2019 6:13 PM
> > To: devel@edk2.groups.io
> > Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Kumar, Chandana C <chandana.c.kumar@intel.com>;
> > Li, Kevin Y <kevin.y.li@intel.com>
> > 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 <lersek@redhat.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> > Cc: Kevin Li <kevin.y.li@intel.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >  .../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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43769): https://edk2.groups.io/g/devel/message/43769
Mute This Topic: https://groups.io/mt/32440401/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Posted by Dong, Eric 4 days ago
Done

SHA-1: 84a459472075d94963463bffaa5dc6eee47f14c3

* 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.
According to SDM, once Enable_PPIN is set, attempt to write
1 to LockOut will cause #GP, and writing 1 to LockOut is
permitted only if Enable_PPIN is clear.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Kevin Li <kevin.y.li@intel.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, July 16, 2019 4:00 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> <kevin.y.li@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin
> code
> 
> 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 <star.zeng@intel.com>; devel@edk2.groups.io
> > Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>;
> > Kumar, Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> > <kevin.y.li@intel.com>
> > 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 <eric.dong@intel.com>
> >
> > Thanks,
> > Eric
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Friday, July 12, 2019 6:13 PM
> > > To: devel@edk2.groups.io
> > > Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Kumar, Chandana C <chandana.c.kumar@intel.com>;
> > > Li, Kevin Y <kevin.y.li@intel.com>
> > > 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 <lersek@redhat.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> > > Cc: Kevin Li <kevin.y.li@intel.com>
> > > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > > ---
> > >  .../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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43770): https://edk2.groups.io/g/devel/message/43770
Mute This Topic: https://groups.io/mt/32440401/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code

Posted by Laszlo Ersek 1 week ago
On 07/12/19 12:12, Star Zeng wrote:
> 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 <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Kevin Li <kevin.y.li@intel.com>
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  .../CpuCommonFeaturesLib/CpuCommonFeatures.h  | 15 +++++
>  .../CpuCommonFeaturesLib.c                    |  2 +-
>  .../Library/CpuCommonFeaturesLib/Ppin.c       | 65 +++++++++++++++----
>  3 files changed, 70 insertions(+), 12 deletions(-)

I'll skip this patch.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43673): https://edk2.groups.io/g/devel/message/43673
Mute This Topic: https://groups.io/mt/32440401/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-