On 9 June 2015 at 18:48, Laszlo Ersek <ler...@redhat.com> wrote:
> On 05/29/15 14:33, Ard Biesheuvel wrote:
>> Instead of inferring the GIC revision from the CPU id registers
>> and the presence/availability of the system register interface
>> upon each invocation, move the logic to a constructor and cache
>> the result.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c | 23 ++++++++++--
>> ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c | 23 ++++++++++--
>> ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf | 3 +-
>> 3 files changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> index 0e0fa3b9f33e..9853c7ba8566 100644
>> --- a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> +++ b/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>> @@ -15,9 +15,11 @@
>> #include <Library/ArmLib.h>
>> #include <Library/ArmGicLib.h>
>>
>> -ARM_GIC_ARCH_REVISION
>> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
>> +
>> +RETURN_STATUS
>> EFIAPI
>> -ArmGicGetSupportedArchRevision (
>> +ArmGicArchLibInitialize (
>> VOID
>> )
>> {
>> @@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
>> IccSre = ArmGicV3GetControlSystemRegisterEnable ();
>> }
>> if (IccSre & ICC_SRE_EL2_SRE) {
>> - return ARM_GIC_ARCH_REVISION_3;
>> + mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
>> + goto Done;
>> }
>> }
>>
>> - return ARM_GIC_ARCH_REVISION_2;
>> + mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
>> +
>> +Done:
>> + return RETURN_SUCCESS;
>> +}
>> +
>> +ARM_GIC_ARCH_REVISION
>> +EFIAPI
>> +ArmGicGetSupportedArchRevision (
>> + VOID
>> + )
>> +{
>> + return mGicArchRevision;
>> }
>> diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> index f256de704631..f8822a224580 100644
>> --- a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> +++ b/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>> @@ -15,9 +15,11 @@
>> #include <Library/ArmLib.h>
>> #include <Library/ArmGicLib.h>
>>
>> -ARM_GIC_ARCH_REVISION
>> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
>> +
>> +RETURN_STATUS
>> EFIAPI
>> -ArmGicGetSupportedArchRevision (
>> +ArmGicArchLibInitialize (
>> VOID
>> )
>> {
>> @@ -43,9 +45,22 @@ ArmGicGetSupportedArchRevision (
>> IccSre = ArmGicV3GetControlSystemRegisterEnable ();
>> }
>> if (IccSre & ICC_SRE_EL2_SRE) {
>> - return ARM_GIC_ARCH_REVISION_3;
>> + mGicArchRevision = ARM_GIC_ARCH_REVISION_3;
>> + goto Done;
>> }
>> }
>>
>> - return ARM_GIC_ARCH_REVISION_2;
>> + mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
>> +
>> +Done:
>> + return RETURN_SUCCESS;
>> +}
>> +
>> +ARM_GIC_ARCH_REVISION
>> +EFIAPI
>> +ArmGicGetSupportedArchRevision (
>> + VOID
>> + )
>> +{
>> + return mGicArchRevision;
>> }
>> diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>> b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>> index d71b2adc3027..7dbcb08f50d6 100644
>> --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>> +++ b/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>> @@ -17,7 +17,8 @@
>> FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
>> MODULE_TYPE = BASE
>> VERSION_STRING = 1.0
>> - LIBRARY_CLASS = ArmGicArchLib
>> + LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER
>> UEFI_APPLICATION
>> + CONSTRUCTOR = ArmGicArchLibInitialize
>>
>> [Sources.ARM]
>> Arm/ArmGicArchLib.c
>>
>
> I trust that you rebuilt all of the DSCs that use this (now restricted)
> library instance via their default library class resolutions, and there
> were no errors (ie. no other referring module types than spelled out
> here) :)
>
> Code-wise, it looks fine.
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>
> I think I'm finished reviewing the still pending patches. At the end of
> this series, we have three instances for the library class:
>
> ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf:
> LIBRARY_CLASS = ArmGicArchLib|SEC
>
> ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf:
> LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
>
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf:
> LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION
>
> The first one (without caching) is used for SEC modules in:
> - ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
> - ArmPlatformPkg/ArmPlatformPkg.dsc
> - ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
> - ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>
> The second one (with caching in a static variable) is used as the
> default resolution in:
> - ArmPkg/ArmPkg.dsc
> - ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
> - ArmPlatformPkg/ArmPlatformPkg.dsc
> - ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
> - ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>
> The third one (also with caching in a static variable, but taken from a
> PCD, not probed from hardware) is used as the default resolution in:
> - ArmVirtPkg/ArmVirt.dsc.inc
>
> Looks good!
>
Thanks for another elaborate review.
It turns out that Olivier is on vacation until next Monday, so
hopefully he will get around to reviewing these patches soon after.
--
Ard.
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
