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

Zeng, Star posted 1 patch 18 weeks 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 18 weeks 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 17 weeks 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 17 weeks 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 17 weeks 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 18 weeks 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]
-=-=-=-=-=-=-=-=-=-=-=-