| Message ID | 1415792793-8488-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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@linaro.org] > Sent: 12 November 2014 19:49 > To: Ard Biesheuvel > Cc: Olivier Martin; edk2-devel@lists.sourceforge.net; Marc Zyngier; > leif.lindholm@linaro.org; kvmarm@lists.cs.columbia.edu; > lersek@redhat.com > 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@linaro.org> > > --- > > 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. ------------------------------------------------------------------------------ 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 13 November 2014 17:47, Olivier Martin <olivier.martin@arm.com> wrote: > 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'. > OK, thanks! For the record, the original symptom I experienced was an unresponsive BDS UI, i.e., countdown timer not changing and no response to key presses. However, it turned out that I was using the GICv3 capable Foundation model (using --gicv3 cmdline option) in combination with a GICv2 device tree, and hence there was no awareness in KVM of the SRE capability and the need to disable/trap it. So when I started using the correct device tree, I started getting exceptions due to the fact that KVM trapped the ICC_SRE_EL1 access but did not actually handle it. Christoffer's fix for that issue is here: http://marc.info/?l=linux-arm-kernel&m=141582259511445&w=2 In summary, if you want to run the latest version of QEMU/KVM mach-virt port of Tianocore on a GICv3 capable host, you will need to apply this patch and the Linux patch above *and* ensure that the host device tree accurately reports the GICv3 capability and not just the GICv2 compatible MMIO interface. Regards, Ard. > Christoffer, yes ArmGicV3SetControlSystemRegisterEnable() does an isb() > after changing the system register. > >> -----Original Message----- >> From: Christoffer Dall [mailto:christoffer.dall@linaro.org] >> Sent: 12 November 2014 19:49 >> To: Ard Biesheuvel >> Cc: Olivier Martin; edk2-devel@lists.sourceforge.net; Marc Zyngier; >> leif.lindholm@linaro.org; kvmarm@lists.cs.columbia.edu; >> lersek@redhat.com >> 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@linaro.org> >> > --- >> > 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. > > > > ------------------------------------------------------------------------------ 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
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 + // 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); + 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); + 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);
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.) Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- 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(-)