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.biesheu...@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 <ler...@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.
Tell me when this part of the series is ready for being applied. (Hm,
actually, you could push them yourself!)
Thanks
Laszlo
> + 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
>
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
