public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Dong, Eric" <eric.dong@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
	"devel@edk2.groups.io" <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
Date: Tue, 16 Jul 2019 07:24:00 +0000	[thread overview]
Message-ID: <ED077930C258884BBCB450DB737E662259E96E9C@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20190712101258.17512-1-star.zeng@intel.com>

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


  parent reply	other threads:[~2019-07-16  7:24 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 10:12 [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code Zeng, Star
2019-07-12 22:12 ` Laszlo Ersek
2019-07-16  7:24 ` Dong, Eric [this message]
2019-07-16  8:00   ` Zeng, Star
2019-07-16  8:05     ` Dong, Eric

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ED077930C258884BBCB450DB737E662259E96E9C@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox