On 12 November 2014 20:43, Olivier Martin <olivier.martin@xxxxxxx> 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. -- Ard. >> -----Original Message----- >> From: Olivier Martin [mailto:olivier.martin@xxxxxxx] >> Sent: 12 November 2014 19:43 >> To: 'Ard Biesheuvel'; edk2-devel@xxxxxxxxxxxxxxxxxxxxx >> Cc: Marc Zyngier; leif.lindholm@xxxxxxxxxx; >> christoffer.dall@xxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; >> lersek@xxxxxxxxxx >> 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@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
