On 13 November 2014 17:47, Olivier Martin <olivier.martin@xxxxxxx> 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@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
