| Message ID | 1432902820-18721-8-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 1 June 2015 at 20:36, Laszlo Ersek <lersek@redhat.com> wrote: > one question below > > On 05/29/15 14:33, Ard Biesheuvel wrote: >> Since it is arguably incorrect to infer the GIC revision from CPU >> ID and GIC feature registers on platforms that describe the GIC in >> the device tree, this implements the library class ArmGicArchLib >> tailored for such platforms. >> >> The supported GIC revision is retrieved from the dynamic PCD that >> is set based on the GIC DT node. >> >> This means this library can only execute post DXE core, but this is >> not a problem for any of the virt platforms. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmVirtPkg/ArmVirt.dsc.inc | 2 +- >> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++++++ >> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 ++++++ >> 3 files changed, 116 insertions(+), 1 deletion(-) >> >> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc >> index 900d5c3d9dd0..fd20ff39a068 100644 >> --- a/ArmVirtPkg/ArmVirt.dsc.inc >> +++ b/ArmVirtPkg/ArmVirt.dsc.inc >> @@ -71,7 +71,7 @@ >> ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf >> DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf >> ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf >> - ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf >> + ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf >> ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf >> ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf >> ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf >> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> new file mode 100644 >> index 000000000000..732860cadfe6 >> --- /dev/null >> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c >> @@ -0,0 +1,75 @@ >> +/** @file >> + ArmGicArchLib library class implementation for DT based virt platforms >> + >> + Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> >> + >> + This program and the accompanying materials >> + are licensed and made available under the terms and conditions of the BSD License >> + which accompanies this distribution. The full text of the license may be found at >> + http://opensource.org/licenses/bsd-license.php >> + >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> + >> +**/ >> + >> +#include <Base.h> >> + >> +#include <Library/ArmGicLib.h> >> +#include <Library/ArmGicArchLib.h> >> +#include <Library/PcdLib.h> >> +#include <Library/DebugLib.h> >> + >> +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; >> + >> +RETURN_STATUS >> +EFIAPI >> +ArmVirtGicArchLibConstructor ( >> + VOID >> + ) >> +{ >> + UINT32 IccSre; >> + >> + switch (PcdGet32 (PcdArmGicRevision)) { >> + >> + case 3: >> + // >> + // The default implementation of ArmGicArchLib is responsible for enabling >> + // the system register interface on the GICv3 if one is found. So let's do >> + // the same here. >> + // >> + IccSre = ArmGicV3GetControlSystemRegisterEnable (); > > Are you satisfied with the performance improvements of this version? The > ArmReadIdPfr0() call has been eliminated indeed, but > ArmGicV3GetControlSystemRegisterEnable() is still called by each binary > that is linked against this library. > > Since in the blurb you said > >> the detection code runs very often, and is quite >> heavy-weight when running under virtualization (especially KVM) > > I'm uncertain about the perf improvements here. The frequency of the > code is exactly the same (*), so any significant savings would have to > come from eliminating ArmReadIdPfr0(). > > If that's a big enough win, then: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Otherwise, you could move ArmGicV3GetControlSystemRegisterEnable() to > VirtFdtDxe (patch #6), and call it when you see PropertyTypeGicV3. It > wouldn't be very nice, but certainly not uglier than the > VirtioMmioInstallDevice() calls! :) That's just what VirtFdtDxe does. > > (*) Hm, wait, I missed something. I skipped patches 1-5, but now I can > see that in "ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c", > it's actually ArmGicGetSupportedArchRevision() that contains all of the > detection code. That may indeed be a big change in frequency, so I can > imagine the big wins. Thus, my R-b is unconditional after all. > Indeed. This series is slightly redundant since it - reduces the sysreg access frequency for all versions - adds the caching for the new non-SEC version - uses the DT for the virt targets and any two out of those three would largely eliminate the issue. However, since the DT based one is actually more correct as well, I decided to propose all three, and decide how to proceed based on the discussion that follows. (Re correctness: the current code would break when emulating an out-of-kernel GICv3 irqchip under KVM on hardware that has GICv2 hardware only. This is purely theoretical though) > Tell me when this part of the series is ready for being applied. (Hm, > actually, you could push them yourself!) > I will wait for Olivier and Leif to comment on the remaining patches. We could drop the ArmGicArchSecLib patches, but I need the split off in #3 for this patch to do anything meaningful.
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc index 900d5c3d9dd0..fd20ff39a068 100644 --- a/ArmVirtPkg/ArmVirt.dsc.inc +++ b/ArmVirtPkg/ArmVirt.dsc.inc @@ -71,7 +71,7 @@ ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf - ArmGicArchLib|ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf + ArmGicArchLib|ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c new file mode 100644 index 000000000000..732860cadfe6 --- /dev/null +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c @@ -0,0 +1,75 @@ +/** @file + ArmGicArchLib library class implementation for DT based virt platforms + + Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> + + This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#include <Base.h> + +#include <Library/ArmGicLib.h> +#include <Library/ArmGicArchLib.h> +#include <Library/PcdLib.h> +#include <Library/DebugLib.h> + +STATIC ARM_GIC_ARCH_REVISION mGicArchRevision; + +RETURN_STATUS +EFIAPI +ArmVirtGicArchLibConstructor ( + VOID + ) +{ + UINT32 IccSre; + + switch (PcdGet32 (PcdArmGicRevision)) { + + case 3: + // + // The default implementation of ArmGicArchLib is responsible for enabling + // the system register interface on the GICv3 if one is found. So let's do + // the same here. + // + IccSre = ArmGicV3GetControlSystemRegisterEnable (); + if (!(IccSre & ICC_SRE_EL2_SRE)) { + ArmGicV3SetControlSystemRegisterEnable (IccSre | ICC_SRE_EL2_SRE); + IccSre = ArmGicV3GetControlSystemRegisterEnable (); + } + + // + // Unlike the default implementation, there is no fall through to GICv2 + // mode if this GICv3 cannot be driven in native mode due to the fact + // that the System Register interface is unavailable. + // + ASSERT (IccSre & ICC_SRE_EL2_SRE); + + mGicArchRevision = ARM_GIC_ARCH_REVISION_3; + break; + + case 2: + mGicArchRevision = ARM_GIC_ARCH_REVISION_2; + break; + + default: + DEBUG ((EFI_D_ERROR, "%a: No GIC revision specified!\n", __FUNCTION__)); + return RETURN_NOT_FOUND; + } + return RETURN_SUCCESS; +} + +ARM_GIC_ARCH_REVISION +EFIAPI +ArmGicGetSupportedArchRevision ( + VOID + ) +{ + return mGicArchRevision; +} diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf new file mode 100644 index 000000000000..c85b2d44d856 --- /dev/null +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf @@ -0,0 +1,40 @@ +#/** @file +# +# Component description file for ArmVirtGicArchLib module +# +# Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR> +# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +#**/ + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = ArmVirtGicArchLib + FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed + MODULE_TYPE = BASE + VERSION_STRING = 1.0 + LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION + CONSTRUCTOR = ArmVirtGicArchLibConstructor + +[Sources] + ArmVirtGicArchLib.c + +[LibraryClasses] + PcdLib + DebugLib + ArmGicLib + +[Packages] + MdePkg/MdePkg.dec + ArmPkg/ArmPkg.dec + ArmVirtPkg/ArmVirtPkg.dec + +[Pcd] + gArmVirtTokenSpaceGuid.PcdArmGicRevision
Since it is arguably incorrect to infer the GIC revision from CPU ID and GIC feature registers on platforms that describe the GIC in the device tree, this implements the library class ArmGicArchLib tailored for such platforms. The supported GIC revision is retrieved from the dynamic PCD that is set based on the GIC DT node. This means this library can only execute post DXE core, but this is not a problem for any of the virt platforms. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/ArmVirt.dsc.inc | 2 +- ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 75 ++++++++++++ ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 40 ++++++ 3 files changed, 116 insertions(+), 1 deletion(-)