Ard, I have just reviewed and committed your patch (SVN rev16344). I removed the reference to Linux kernel to answer Christoffer comment. I also replaced 'lower levels' by 'higher privilege levels'. Christoffer, yes ArmGicV3SetControlSystemRegisterEnable() does an isb() after changing the system register. > -----Original Message----- > From: Christoffer Dall [mailto:christoffer.dall@xxxxxxxxxx] > Sent: 12 November 2014 19:49 > To: Ard Biesheuvel > Cc: Olivier Martin; edk2-devel@xxxxxxxxxxxxxxxxxxxxx; Marc Zyngier; > leif.lindholm@xxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; > lersek@xxxxxxxxxx > Subject: Re: [PATCH 2/2] ArmGicLib: select GICv2 mode if SRE is present > but unavailable > > On Wed, Nov 12, 2014 at 12:46:33PM +0100, Ard Biesheuvel wrote: > > Even if the CPU id registers indicate hardware support for the > > System Register interface to the GIC, higher exception levels > > may disable that interface and only allow access through MMIO. > > > > So move the enabling of the SRE bit to the GIC version detection > > routine: if we trigger an exception, we would have anyway at a > > later stage, so the net effect is the same. However, if setting > > the bit doesn't stick, it means we can switch to MMIO and proceed > > normally otherwise. (This is analogous to how the Linux kernel > > behaves when executed as a guest under KVM.) > > not quite, Linux tries to use the device it's being told about in the > DT. But it still does the set/check of the bit, but if it doesn't > stick, it prints an error and dies... > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > --- > > ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c | 14 +++++++++++++- > > ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c | 14 +++++++++++++- > > ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 8 -------- > > 3 files changed, 26 insertions(+), 10 deletions(-) > > > > diff --git a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > > index 5a7837f43d94..e2955ae861d5 100644 > > --- a/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > > +++ b/ArmPkg/Drivers/ArmGic/AArch64/ArmGicArchLib.c > > @@ -15,6 +15,8 @@ > > #include <Library/ArmLib.h> > > #include <Library/ArmGicLib.h> > > > > +#include "GicV3/ArmGicV3Lib.h" > > + > > ARM_GIC_ARCH_REVISION > > EFIAPI > > ArmGicGetSupportedArchRevision ( > > @@ -28,7 +30,17 @@ ArmGicGetSupportedArchRevision ( > > // driver requires SRE. If only Memory mapped access is available > we try to > > // drive the GIC as a v2. > > if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) { > > - return ARM_GIC_ARCH_REVISION_3; > > + // Make sure System Register access is enabled (SRE). This > depends on the > > + // lower levels giving us permission, otherwise we will either > cause an > > lower levels... not sure what the convention here is, but higher > privilege level would be more accurate, no? > > > + // exception here, or the write doesn't stick in which case we > need to > > + // fall back to the GICv2 MMIO interface. > > + // Note: We do not need to set ICC_SRE_EL2.Enable because the OS > is started > > + // at the same exception level. > > + // It is the OS responsibility to set this bit. > > + ArmGicV3SetControlSystemRegisterEnable > (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE); > > don't you need an isb() here like the linux code has or is that implied > in the ArmGicV3SetControlSystemRegisterEnable function? > > > + if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) > { > > + return ARM_GIC_ARCH_REVISION_3; > > + } > > } > > > > return ARM_GIC_ARCH_REVISION_2; > > diff --git a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > > index 668858f79a3d..02e5638f2fcf 100644 > > --- a/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > > +++ b/ArmPkg/Drivers/ArmGic/Arm/ArmGicArchLib.c > > @@ -15,6 +15,8 @@ > > #include <Library/ArmLib.h> > > #include <Library/ArmGicLib.h> > > > > +#include "GicV3/ArmGicV3Lib.h" > > + > > ARM_GIC_ARCH_REVISION > > EFIAPI > > ArmGicGetSupportedArchRevision ( > > @@ -28,7 +30,17 @@ ArmGicGetSupportedArchRevision ( > > // driver requires SRE. If only Memory mapped access is available > we try to > > // drive the GIC as a v2. > > if (ArmReadIdPfr1 () & ARM_PFR1_GIC) { > > - return ARM_GIC_ARCH_REVISION_3; > > + // Make sure System Register access is enabled (SRE). This > depends on the > > + // lower levels giving us permission, otherwise we will either > cause an > > + // exception here, or the write doesn't stick in which case we > need to > > + // fall back to the GICv2 MMIO interface. > > + // Note: We do not need to set ICC_SRE_EL2.Enable because the OS > is started > > + // at the same exception level. > > + // It is the OS responsibility to set this bit. > > + ArmGicV3SetControlSystemRegisterEnable > (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE); > > same > > > + if (ArmGicV3GetControlSystemRegisterEnable () & ICC_SRE_EL2_SRE) > { > > + return ARM_GIC_ARCH_REVISION_3; > > + } > > } > > > > return ARM_GIC_ARCH_REVISION_2; > > diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > > index 8042f718f5b0..f756d3080386 100644 > > --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > > +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c > > @@ -281,14 +281,6 @@ GicV3DxeInitialize ( > > } > > } > > > > - // Make sure System Register access is enabled (SRE). This depends > on the > > - // lower levels giving us permission, otherwise we will cause an > exception > > - // here. > > - // Note: We do not need to set ICC_SRE_EL2.Enable because the OS > is started at the > > - // same exception level. > > - // It is the OS responsibility to set this bit. > > - ArmGicV3SetControlSystemRegisterEnable > (ArmGicV3GetControlSystemRegisterEnable () | ICC_SRE_EL2_SRE); > > - > > // Set binary point reg to 0x7 (no preemption) > > ArmGicV3SetBinaryPointer (0x7); > > > > -- > > 1.8.3.2 > > > apart from the above, the logic looks good to me. _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
