Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 13, 2019 11:01 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify
> PcdCpuFeaturesSupport.
> 
> 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).

Agree, added notes in the BZ to explain what real changes will do.

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

Sorry, I missed this part. Will include it in V2 changes.

> 
> (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.

Agree, will include it in v2 changes.

Thanks,
Eric
> 
> 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);
> >