| Message ID | 1415792793-8488-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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@linaro.org] > Sent: 12 November 2014 11:47 > To: Olivier Martin; edk2-devel@lists.sourceforge.net > Cc: Marc Zyngier; leif.lindholm@linaro.org; > christoffer.dall@linaro.org; kvmarm@lists.cs.columbia.edu; > lersek@redhat.com; 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@linaro.org> > --- > 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 > ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
I forgot to say your line endings are not correct. You have to use CRLF - sorry that's the coding convention. > -----Original Message----- > From: Olivier Martin [mailto:olivier.martin@arm.com] > Sent: 12 November 2014 19:43 > To: 'Ard Biesheuvel'; edk2-devel@lists.sourceforge.net > Cc: Marc Zyngier; leif.lindholm@linaro.org; > christoffer.dall@linaro.org; kvmarm@lists.cs.columbia.edu; > lersek@redhat.com > Subject: RE: [PATCH 1/2] ArmGicLib: cache GIC arch revision in global > variable > > 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@linaro.org] > > Sent: 12 November 2014 11:47 > > To: Olivier Martin; edk2-devel@lists.sourceforge.net > > Cc: Marc Zyngier; leif.lindholm@linaro.org; > > christoffer.dall@linaro.org; kvmarm@lists.cs.columbia.edu; > > lersek@redhat.com; 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@linaro.org> > > --- > > 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 > > ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
On 12 November 2014 20:43, Olivier Martin <olivier.martin@arm.com> wrote: > I forgot to say your line endings are not correct. You have to use CRLF - > sorry that's the coding convention. > I know, I know ... I just failed to pay attention to it. Anyway, we can drop this patch entirely, I did not realize that ArmGicLib was executed straight from flash, so this approach clearly cannot work. I would like your comments on 2/2 though. Your current code breaks the KVM case as ID_AA64_PFR0 having a non-zero GIC field does not necessarily imply that the SRE is available.
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
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@linaro.org> --- ArmPkg/Drivers/ArmGic/ArmGicLib.c | 42 +++++++++++++++++++------------------ ArmPkg/Drivers/ArmGic/ArmGicLib.inf | 1 + 2 files changed, 23 insertions(+), 20 deletions(-)