| Message ID | 1432902820-18721-6-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 9 June 2015 at 18:48, Laszlo Ersek <lersek@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.biesheuvel@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 <lersek@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.
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
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.biesheuvel@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(-)