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
