public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Eric Dong <eric.dong@intel.com>, edk2-devel@lists.01.org
Subject: Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
Date: Wed, 13 Feb 2019 04:01:25 +0100	[thread overview]
Message-ID: <7831b95d-de56-2bb1-9d3f-99cbf596dcb8@redhat.com> (raw)
In-Reply-To: <20190213020405.18800-4-eric.dong@intel.com>

On 02/13/19 03:04, Eric Dong wrote:
> PcdCpuFeaturesSupport used to specify the platform policy about
> what CPU features this platform supports. This value is decide by
> platform owner, not by hardware. After this change, this PCD will
> be used in IsCpuFeatureSupported function only.
> 
> Now RegisterCpuFeaturesLib use this PCD as an template to Get the
> pcd size. Update the code logic to replace it with
> PcdCpuFeaturesSetting.
> 
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375
> 
> Cc: Ray Ni <Ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 66 +++++++---------------
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
>  .../RegisterCpuFeaturesLib.c                       | 10 ++--
>  3 files changed, 24 insertions(+), 53 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 4ebd0025b4..762eaec277 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -74,27 +74,6 @@ GetSettingPcd (
>    return SettingBitMask;
>  }
>  
> -/**
> -  Worker function to get PcdCpuFeaturesSupport.
> -
> -  @return  The pointer to CPU feature bits mask buffer.
> -**/
> -UINT8 *
> -GetSupportPcd (
> -  VOID
> -  )
> -{
> -  UINT8                  *SupportBitMask;
> -
> -  SupportBitMask = AllocateCopyPool (
> -          PcdGetSize (PcdCpuFeaturesSupport),
> -          PcdGetPtr (PcdCpuFeaturesSupport)
> -          );
> -  ASSERT (SupportBitMask != NULL);
> -
> -  return SupportBitMask;
> -}
> -
>  /**
>    Collects CPU type and feature information.
>  
> @@ -282,11 +261,6 @@ CpuInitDataInitialize (
>    ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
>    CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
>    ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
> -
> -  //
> -  // Get support and configuration PCDs
> -  //
> -  CpuFeaturesData->SupportPcd       = GetSupportPcd ();
>  }
>  
>  /**
> @@ -306,7 +280,7 @@ SupportedMaskOr (
>    UINT8                  *Data1;
>    UINT8                  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>    Data1 = SupportedFeatureMask;
>    Data2 = OrFeatureBitMask;
>    for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -331,7 +305,7 @@ SupportedMaskAnd (
>    UINT8                  *Data1;
>    UINT8                  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>    Data1 = SupportedFeatureMask;
>    Data2 = AndFeatureBitMask;
>    for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -356,7 +330,7 @@ SupportedMaskCleanBit (
>    UINT8                  *Data1;
>    UINT8                  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>    Data1 = SupportedFeatureMask;
>    Data2 = AndFeatureBitMask;
>    for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -387,7 +361,7 @@ IsBitMaskMatch (
>    UINT8                  *Data1;
>    UINT8                  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>  
>    Data1 = SupportedFeatureMask;
>    Data2 = ComparedFeatureBitMask;
> @@ -426,22 +400,22 @@ CollectProcessorData (
>    Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
>    while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
>      CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> -    if (IsBitMaskMatch (CpuFeaturesData->SupportPcd, CpuFeature->FeatureMask)) {
> -      if (CpuFeature->SupportFunc == NULL) {
> -        //
> -        // If SupportFunc is NULL, then the feature is supported.
> -        //
> -        SupportedMaskOr (
> -          CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> -          CpuFeature->FeatureMask
> -          );
> -      } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, CpuFeature->ConfigData)) {
> -        SupportedMaskOr (
> -          CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> -          CpuFeature->FeatureMask
> -          );
> -      }
> +
> +    if (CpuFeature->SupportFunc == NULL) {
> +      //
> +      // If SupportFunc is NULL, then the feature is supported.
> +      //
> +      SupportedMaskOr (
> +        CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> +        CpuFeature->FeatureMask
> +        );
> +    } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, CpuFeature->ConfigData)) {
> +      SupportedMaskOr (
> +        CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> +        CpuFeature->FeatureMask
> +        );
>      }
> +
>      Entry = Entry->ForwardLink;
>    }
>  }

The functional effect of the change in CollectProcessorData() is too
complex for me to determine, so I'd like to leave the review of this
patch to Ray only.

(1) FWIW, I've attempted to read the BZ carefully, and it seems the
patch doesn't do all things that the BZ suggests:

- The IsCpuFeatureSupported() function is not removed. That could be
correct, but then I think it should be explained in the BZ (i.e. update
the scope).

- Is it asserted somewhere that PcdCpuFeaturesSetting and
PcdCpuFeaturesCapability have equal size?

(2) Independently, please append (or prepend) a cleanup patch to the
series. Namely, the comment block on IsCpuFeatureSetInCpuPcd()
references "PcdCpuFeaturesSupport". It should reference "CpuBitMask"
instead, IMO.

Thanks
Laszlo

> @@ -646,8 +620,6 @@ AnalysisProcessorFeatures (
>        DumpCpuFeature (CpuFeature);
>        Entry = Entry->ForwardLink;
>      }
> -    DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
> -    DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
>      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
>      DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
>      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n"));
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 3e0a342fd1..836ed3549c 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -81,7 +81,6 @@ typedef struct {
>    LIST_ENTRY               FeatureList;
>  
>    CPU_FEATURES_INIT_ORDER  *InitOrder;
> -  UINT8                    *SupportPcd;
>    UINT8                    *CapabilityPcd;
>    UINT8                    *SettingPcd;
>  
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 3540029079..3e8e899766 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -31,7 +31,7 @@ IsCpuFeatureMatch (
>  {
>    UINTN                 BitMaskSize;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>    if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) {
>      return TRUE;
>    } else {
> @@ -53,7 +53,7 @@ DumpCpuFeatureMask (
>    UINT8                  *Data8;
>    UINTN                  BitMaskSize;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>    Data8       = (UINT8 *) FeatureMask;
>    for (Index = 0; Index < BitMaskSize; Index++) {
>      DEBUG ((DEBUG_INFO, " %02x ", *Data8++));
> @@ -100,7 +100,7 @@ IsBitMaskMatchCheck (
>    UINT8      *Data1;
>    UINT8      *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>  
>    Data1 = FeatureMask;
>    Data2 = DependentBitMask;
> @@ -656,7 +656,7 @@ RegisterCpuFeatureWorker (
>    UINTN                      BitMaskSize;
>    BOOLEAN                    FeatureExist;
>  
> -  BitMaskSize     = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize     = PcdGetSize (PcdCpuFeaturesSetting);
>    CpuFeaturesData = GetCpuFeaturesData ();
>    if (CpuFeaturesData->FeaturesCount == 0) {
>      InitializeListHead (&CpuFeaturesData->FeatureList);
> @@ -870,7 +870,7 @@ RegisterCpuFeature (
>    BeforeAll            = FALSE;
>    AfterAll             = FALSE;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>  
>    VA_START (Marker, InitializeFunc);
>    Feature = VA_ARG (Marker, UINT32);
> 



  reply	other threads:[~2019-02-13  3:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-13  2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
2019-02-13  2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
2019-02-13  2:23   ` Laszlo Ersek
2019-02-13  2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
2019-02-13  2:43   ` Laszlo Ersek
2019-02-14  2:00     ` Dong, Eric
2019-02-14  8:47   ` Ni, Ray
2019-02-13  2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
2019-02-13  3:01   ` Laszlo Ersek [this message]
2019-02-14  1:01     ` Dong, Eric
2019-02-14  8:59   ` Ni, Ray
2019-02-14  8:57 ` [Patch 0/3] Simplify CPU Features solution Ni, Ray

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=7831b95d-de56-2bb1-9d3f-99cbf596dcb8@redhat.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