[edk2-devel] [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
Ni, Ray
ray.ni at intel.com
Mon Jul 15 04:59:23 UTC 2019
Reviewed-by: Ray Ni <ray.ni at intel.com>
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, July 12, 2019 6:53 PM
> To: Dong, Eric <eric.dong at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>; Kumar,
> Chandana C <chandana.c.kumar at intel.com>; Zeng, Star
> <star.zeng at intel.com>
> Subject: RE: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> gBS service.
>
> Some minor comments inline.
>
> > -----Original Message-----
> > From: Dong, Eric
> > Sent: Friday, July 12, 2019 9:53 AM
> > To: devel at edk2.groups.io
> > Cc: Ni, Ray <ray.ni at intel.com>; Laszlo Ersek <lersek at redhat.com>;
> > Kumar, Chandana C <chandana.c.kumar at intel.com>; Zeng, Star
> > <star.zeng at intel.com>
> > Subject: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> > gBS service.
>
> "gBS service" is not match with the assertion information, gBS is the concept
> in DXE phase.
> So here, please "PEI service" to be accurate.
>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
> >
> > AP calls CollectProcessorData() to collect processor info.
> > CollectProcessorData function finally calls PcdGetSize function to get
> > DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
>
> Same comments with above.
>
> > which caused below assert info:
> > Processor Info: Package: 1, MaxCore : 4, MaxThread: 1
> > Package: 0, Valid Core : 4
> > ASSERT [CpuFeaturesPei] c:\projects\jsl\jsl_v1193\Edk2\MdePkg\Library
> > \PeiServicesTablePointerLibIdt\PeiServicesTablePointer.c(48):
> > PeiServices != ((void *) 0)
> >
> > This change uses saved global pcd size instead of calls PcdGetSize to
> > fix this issue.
> >
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Chandana Kumar <chandana.c.kumar at intel.com>
> > Cc: Star Zeng <star.zeng at intel.com>
> > Signed-off-by: Eric Dong <eric.dong at intel.com>
> > ---
> > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 13
> > ++++++++----- .../RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c |
> > 5 +++++
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index aff7ad600c..87bfc64250 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -246,19 +246,20 @@ CpuInitDataInitialize (
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> > @param[in] OrFeatureBitMask The feature bit mask to do OR
> operation
> > + @param[in] BitMaskSize The CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > SupportedMaskOr (
> > IN UINT8 *SupportedFeatureMask,
> > - IN UINT8 *OrFeatureBitMask
> > + IN UINT8 *OrFeatureBitMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > - UINTN BitMaskSize;
> > UINT8 *Data1;
> > UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data1 = SupportedFeatureMask;
> > Data2 = OrFeatureBitMask;
> > for (Index = 0; Index < BitMaskSize; Index++) { @@ -384,12 +385,14
> > @@ CollectProcessorData (
> > //
> > SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > - CpuFeature->FeatureMask
> > + CpuFeature->FeatureMask,
> > + CpuFeaturesData->BitMaskSize
> > );
> > } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo,
> > CpuFeature->ConfigData)) {
> > SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > - CpuFeature->FeatureMask
> > + CpuFeature->FeatureMask,
> > + CpuFeaturesData->BitMaskSize
> > );
> > }
> > Entry = Entry->ForwardLink;
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index fa0f0b41e2..36aabd7267 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > @@ -658,6 +658,11 @@ RegisterCpuFeatureWorker (
> > InitializeListHead (&CpuFeaturesData->FeatureList);
> > InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> > InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> > + //
> > + // Driver has assumption that these three PCD should has same
> > + buffer
> > size.
>
> It is library, not driver. So how about "The code has assumption that these
> three PCDs should have same buffer size."?
> The proposed sentence also uses 'PCDs' and 'have'.
>
>
> With the comments handled, Reviewed-by: Star Zeng <star.zeng at intel.com>
>
> Thanks,
> Star
>
> > + //
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesCapability));
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesSupport));
> > CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> > }
> > ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> > --
> > 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43703): https://edk2.groups.io/g/devel/message/43703
Mute This Topic: https://groups.io/mt/32437607/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list