I also thought using a global variable in my initial implementation. But it does not work when the library is called from XIP phases. In this case the global variables live in Read-Only memory and it would crash as soon as you try to write to it. This change is only safe when this library is called from these following module types: DXE_DRIVER, DXE_RUNTIME_DRIVER, UEFI_DRIVER, or UEFI_APPLICATION. The way to fix it is to introduce a new ArmPkg/Drivers/ArmGic/ArmGicDxeLib.inf (limited to the above MODULE_TYPEs) and to enable your change in this file. You would also need to move out the functions from ArmPkg/Drivers/ArmGic/ArmGicLib.c that use this change (ie: using either the global variable mGicArchRevision or calling ArmGicGetSupportedArchRevision()). > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@xxxxxxxxxx] > Sent: 12 November 2014 11:47 > To: Olivier Martin; edk2-devel@xxxxxxxxxxxxxxxxxxxxx > Cc: Marc Zyngier; leif.lindholm@xxxxxxxxxx; > christoffer.dall@xxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; > lersek@xxxxxxxxxx; Ard Biesheuvel > Subject: [PATCH 1/2] ArmGicLib: cache GIC arch revision in global > variable > > Before extending the GIC arch revision check to support an emulated > GICv2 on GICv3 capable hardware in the next patch, add a constructor > to ArmGicLib that caches the result of the check so we don't have to > go through it on every call into ArmGicLib. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > --- > ArmPkg/Drivers/ArmGic/ArmGicLib.c | 42 +++++++++++++++++++---------- > -------- > ArmPkg/Drivers/ArmGic/ArmGicLib.inf | 1 + > 2 files changed, 23 insertions(+), 20 deletions(-) > > diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c > b/ArmPkg/Drivers/ArmGic/ArmGicLib.c > index 1e5924f5a49f..c48b97e3eb4d 100644 > --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c > +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c > @@ -20,6 +20,8 @@ > #include "GicV2/ArmGicV2Lib.h" > #include "GicV3/ArmGicV3Lib.h" > > +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; > + > UINTN > EFIAPI > ArmGicGetInterfaceIdentification ( > @@ -72,10 +74,7 @@ ArmGicAcknowledgeInterrupt ( > ) > { > UINTN Value; > - ARM_GIC_ARCH_REVISION Revision; > - > - Revision = ArmGicGetSupportedArchRevision (); > - if (Revision == ARM_GIC_ARCH_REVISION_2) { > + if (mGicArchRevision == ARM_GIC_ARCH_REVISION_2) { > Value = ArmGicV2AcknowledgeInterrupt (GicInterruptInterfaceBase); > // InterruptId is required for the caller to know if a valid or > spurious > // interrupt has been read > @@ -83,7 +82,7 @@ ArmGicAcknowledgeInterrupt ( > if (InterruptId != NULL) { > *InterruptId = Value & ARM_GIC_ICCIAR_ACKINTID; > } > - } else if (Revision == ARM_GIC_ARCH_REVISION_3) { > + } else if (mGicArchRevision == ARM_GIC_ARCH_REVISION_3) { > Value = ArmGicV3AcknowledgeInterrupt (); > } else { > ASSERT_EFI_ERROR (EFI_UNSUPPORTED); > @@ -102,12 +101,9 @@ ArmGicEndOfInterrupt ( > IN UINTN Source > ) > { > - ARM_GIC_ARCH_REVISION Revision; > - > - Revision = ArmGicGetSupportedArchRevision (); > - if (Revision == ARM_GIC_ARCH_REVISION_2) { > + if (mGicArchRevision == ARM_GIC_ARCH_REVISION_2) { > ArmGicV2EndOfInterrupt (GicInterruptInterfaceBase, Source); > - } else if (Revision == ARM_GIC_ARCH_REVISION_3) { > + } else if (mGicArchRevision == ARM_GIC_ARCH_REVISION_3) { > ArmGicV3EndOfInterrupt (Source); > } else { > ASSERT_EFI_ERROR (EFI_UNSUPPORTED); > @@ -183,12 +179,9 @@ ArmGicEnableInterruptInterface ( > IN INTN GicInterruptInterfaceBase > ) > { > - ARM_GIC_ARCH_REVISION Revision; > - > - Revision = ArmGicGetSupportedArchRevision (); > - if (Revision == ARM_GIC_ARCH_REVISION_2) { > + if (mGicArchRevision == ARM_GIC_ARCH_REVISION_2) { > ArmGicV2EnableInterruptInterface (GicInterruptInterfaceBase); > - } else if (Revision == ARM_GIC_ARCH_REVISION_3) { > + } else if (mGicArchRevision == ARM_GIC_ARCH_REVISION_3) { > ArmGicV3EnableInterruptInterface (); > } else { > ASSERT_EFI_ERROR (EFI_UNSUPPORTED); > @@ -201,14 +194,23 @@ ArmGicDisableInterruptInterface ( > IN INTN GicInterruptInterfaceBase > ) > { > - ARM_GIC_ARCH_REVISION Revision; > - > - Revision = ArmGicGetSupportedArchRevision (); > - if (Revision == ARM_GIC_ARCH_REVISION_2) { > + if (mGicArchRevision == ARM_GIC_ARCH_REVISION_2) { > ArmGicV2DisableInterruptInterface (GicInterruptInterfaceBase); > - } else if (Revision == ARM_GIC_ARCH_REVISION_3) { > + } else if (mGicArchRevision == ARM_GIC_ARCH_REVISION_3) { > ArmGicV3DisableInterruptInterface (); > } else { > ASSERT_EFI_ERROR (EFI_UNSUPPORTED); > } > } > + > + > +RETURN_STATUS > +EFIAPI > +ArmGicLibInitialize ( > + VOID > + ) > +{ > + mGicArchRevision = ArmGicGetSupportedArchRevision (); > + > + return RETURN_SUCCESS; > +} > diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf > b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf > index 81282b9b8299..ceeb95c53e98 100644 > --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.inf > +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.inf > @@ -18,6 +18,7 @@ > MODULE_TYPE = SEC > VERSION_STRING = 1.0 > LIBRARY_CLASS = ArmGicLib > + CONSTRUCTOR = ArmGicLibInitialize > > [Sources] > ArmGicLib.c > -- > 1.8.3.2 > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
