On 04/06/16 18:15, Ard Biesheuvel wrote:
> Instead of relying on VirtFdtDxe to populate the GIC related PCDs, move
> this handling to our implementation of ArmGicArchLib, and retrieve the
> required DT info using the new FDT client protocol.
>
> This removes one of the reasons we need to load VirtFdtDxe first using
> an 'A PRIORI' declaration in the platform FDF.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 84
> ++++++++++++++++++--
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf | 15 +++-
> 2 files changed, 92 insertions(+), 7 deletions(-)
Urgh.
I guess the conversion (together with the next patch) should be
"straightforward". However, before I try to validate the code movement,
I have a small and a big question.
(21) small one: what do you need <libfdt.h>, FdtLib, and (consequently)
EmbeddedPkg/EmbeddedPkg.dec for?
... Nevermind, coming back from the source code below, I understand.
I'll make a suggestion down there. Consider (21) a no-op.
(22) The big question is this: how can I determine where this library
instance is used?
Namely, before the conversion, PcdGic*Base would be set only once (in
VirtFdtDxe). That means two things: (a) it is set *for sure*, for any
DXE driver that might want to look at the PCDs, plus (b) the PCDs are
not set more than once.
I can't see how this conversion guarantees either.
Let's start with question (b). (Breaking (b) is not a catastrophe, but
it's ugly.)
I tried to round up users of the lib class ArmGicArchLib. I found two
client INF files:
- ArmPkg/Drivers/ArmGic/ArmGicLib.inf
- ArmPkg/Drivers/ArmGic/ArmGicSecLib.inf
These are themselves library instances (with MODULE_TYPE=SEC, WTF?) So,
I guess I have to find the clients of the lib class ArmGicLib. They are:
- ArmPkg/Application/LinuxLoader/LinuxLoader.inf
We use this in our ARM (32-bit) builds. It means whenever this app is
run, it will re-set the PCDs. :/ ... Although, it looks like the
application only depends on ArmGicLib in the AARCH64 case, precisely
which architecture we don't build the application for. Okay,
ultimately, but confusing...
- ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
I guess this is our main target. This one is therefore certainly
correct to set the PCDs.
- ArmPlatformPkg/ArmVExpressPkg/ArmVExpressDxe/ArmFvpDxe.inf
We never include this driver. Okay.
- ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
- ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
- ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
What the heck, all of these library instances are for class
ArmGicArchLib, which is what I started out with! We actually use the
last one (this patch modifies exactly it); it creates a circular
library dependency between ArmGicLib and ArmGicArchLib. Sweet $DEITY.
Anyway, since I started out tracking down ArmGicArchLib, and we don't
use the first two of the above, they don't add new things to follow.
- ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf
We have a library class resolution to this, in "ArmVirt.dsc.inc", for
ArmPlatformSecExtraActionLib. That's a dead-end, thankfully; grepping
for the lib class, only
"ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibCTA9x4/ArmVExpressSecLib.inf"
depends on it, which I can't imagine we use. The resolution should be
dropped from "ArmVirtPkg/ArmVirt.dsc.inc", probably.
- ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
- ArmPlatformPkg/PrePi/PeiMPCore.inf
These are SEC modules, and we don't use them. Okay.
So, ultimately, the only user of this library instance is
"ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". ... Indeed, checking the build
report file for ArmVirtQemu (AARCH64), I find ArmVirtGicArchLib (and
ArmGicLib too) only under "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf".
Question (22)(b) is therefore cleared; the PCDs in question will only be
set once, when "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" is dispatched.
Let's see (22)(a): can anything consume these PCDs before
"ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" sets them?
- ArmPkg/Application/LinuxLoader/LinuxLoader.inf
Same as above: uses the PCDs only on AARCH64, when we don't build it.
- ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
The library sets the PCDs for it in time.
-
ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf
We don't use this (it wants the PCDs as Fixed, anyway).
- ArmPlatformPkg/Library/DebugSecExtraActionLib/DebugSecExtraActionLib.inf
- ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
- ArmPlatformPkg/PrePi/PeiMPCore.inf
Ditto for these three.
Alright! It was exhausting to verify it (you could have written it up
too... :)), but the conversion seems safe. Ultimately, as (22), I'll
make this request:
(22) please document in the commit message, that in all our builds, only
"ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" links against this library, and
only "ArmPkg/Drivers/ArmGic/ArmGicDxe.inf" consumes the PCDs.
... This is actually a "standard requirement" for the rest of the
plugin-like patches, more or less. Do you think (knowing the rest of the
series) that it might make sense for you to post a v2 after my comments
thus far? Mainly, this "usage analysis" is exhausting, and for the rest
of the series, I'd prefer to validate yours, not construct mine. :)
Anyway, let's see the rest of this patch (INF file moved to the top --
can you perhaps adopt a diff-order file, for formatting the patches?):
> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
> b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
> index c85b2d44d856..57086242de1f 100644
> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.inf
> @@ -18,7 +18,7 @@ [Defines]
> INF_VERSION = 0x00010005
> BASE_NAME = ArmVirtGicArchLib
> FILE_GUID = 87b0dc84-4661-4deb-a789-97977ff636ed
> - MODULE_TYPE = BASE
> + MODULE_TYPE = DXE_DRIVER
(23) I don't think it's necessary. You use neither ImageHandle nor
SystemTable below. The client type restriction (on the LIBRARY_CLASS
line below) suffices.
> VERSION_STRING = 1.0
> LIBRARY_CLASS = ArmGicArchLib|DXE_DRIVER UEFI_DRIVER
> UEFI_APPLICATION
> CONSTRUCTOR = ArmVirtGicArchLibConstructor
> @@ -30,11 +30,22 @@ [LibraryClasses]
> PcdLib
> DebugLib
> ArmGicLib
> + UefiBootServicesTableLib
> + FdtLib
>
> [Packages]
> MdePkg/MdePkg.dec
> ArmPkg/ArmPkg.dec
> ArmVirtPkg/ArmVirtPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
> +
> +[Protocols]
> + gFdtClientProtocolGuid
>
> [Pcd]
> - gArmVirtTokenSpaceGuid.PcdArmGicRevision
> + gArmTokenSpaceGuid.PcdGicDistributorBase
> + gArmTokenSpaceGuid.PcdGicRedistributorsBase
> + gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
> +
> +[Depex]
> + gFdtClientProtocolGuid
>
Looks okay (modulo (21) above).
> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> index 732860cadfe6..686622228831 100644
> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> @@ -1,7 +1,7 @@
> /** @file
> ArmGicArchLib library class implementation for DT based virt platforms
>
> - Copyright (c) 2015, Linaro Ltd. All rights reserved.<BR>
> + Copyright (c) 2015 - 2016, 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
> @@ -19,21 +19,83 @@
> #include <Library/ArmGicArchLib.h>
> #include <Library/PcdLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <libfdt.h>
> +
> +#include <Protocol/FdtClient.h>
>
> STATIC ARM_GIC_ARCH_REVISION mGicArchRevision;
>
> -RETURN_STATUS
> +EFI_STATUS
> EFIAPI
> ArmVirtGicArchLibConstructor (
> - VOID
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> - UINT32 IccSre;
> + UINT32 IccSre;
> + FDT_CLIENT_PROTOCOL *FdtClient;
> + CONST VOID *Reg;
> + UINTN RegElemSize, RegSize;
> + UINTN GicRevision;
> + EFI_STATUS Status;
> + UINT64 DistBase, CpuBase, RedistBase;
>
> - switch (PcdGet32 (PcdArmGicRevision)) {
> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, (VOID
> **)&FdtClient);
(24) This line is too long.
Can you double-check the series that added lines don't exceed 79
characters? (I think "BaseTools/Scripts/PatchCheck.py" might help you
verify this -- see a7e173b07a1e.) I guess I shouldn't obsess about code
that is moved, but code you write genuinely for this work shouldn't be
too wide.
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
(25) Given the DepEx, you might want to replace this with an
ASSERT_EFI_ERROR(). Up to you.
> +
> + GicRevision = 2;
> + Status = FdtClient->FindCompatibleNodeReg (FdtClient,
> + "arm,cortex-a15-gic",
> + &Reg,
> + &RegElemSize,
> + &RegSize);
(26) The indentation of the arguments is not correct; they should be
aligned with "FindCompatibleNodeReg", plus one or two spaces.
Can you re-check the series for such indentation problems?
> + if (Status == EFI_NOT_FOUND) {
> + GicRevision = 3;
> + Status = FdtClient->FindCompatibleNodeReg (FdtClient,
> + "arm,gic-v3",
> + &Reg,
> + &RegElemSize,
> + &RegSize);
> + }
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + switch (GicRevision) {
>
> case 3:
> //
> + // The GIC v3 DT binding describes a series of at least 3 physical (base
> + // addresses, size) pairs: the distributor interface (GICD), at least one
> + // redistributor region (GICR) containing dedicated redistributor
> + // interfaces for all individual CPUs, and the CPU interface (GICC).
> + // Under virtualization, we assume that the first redistributor region
> + // listed covers the boot CPU. Also, our GICv3 driver only supports the
> + // system register CPU interface, so we can safely ignore the MMIO
> version
> + // which is listed after the sequence of redistributor interfaces.
> + // This means we are only interested in the first two memory regions
> + // supplied, and ignore everything else.
> + //
> + ASSERT (RegSize >= 32);
> +
> + // RegProp[0..1] == { GICD base, GICD size }
> + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]);
Sigh. So this is the answer to (21). Here's another proposal then:
(27) can we perhaps dispense with the byte order conversion helpers from
libfdt? I mean, it is annoying to include the FDT library *just* for
this, when we have our shiny new protocol ready, for the rest of the
device tree massaging. So the idea is:
- If (knowing the rest of the series) you deem that libfdt is frequently
needed in addition to the new protocol *because* we frequently
manipulate the DTB beyond the capabilities of the new protocol (and in
a way that is hard to extract into the protocol), then sure, libfdt
and the protocol should co-exist in client code, and I have nothing
against fdt64_to_cpu()
- OTOH, if we mostly (only?) include LibFdt on top of the new protocol,
in the rest of the series too, because we can't live without
fdt64_to_cpu() and friends, then I suggest to remove the LibFdt
dependency, and exploit that FDT internals are always big-endian,
while UEFI is always little-endian. Therefore, use the SwapBytes64()
function (and friends) directly, from BaseLib.
What do you think?
> + ASSERT (DistBase < MAX_UINT32);
> +
> + // RegProp[2..3] == { GICR base, GICR size }
> + RedistBase = fdt64_to_cpu (((UINT64 *)Reg)[2]);
> + ASSERT (RedistBase < MAX_UINT32);
> +
> + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
> + PcdSet32 (PcdGicRedistributorsBase, (UINT32)RedistBase);
> +
> + DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
> + DistBase, RedistBase));
> +
> + //
> // 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.
> @@ -55,6 +117,18 @@ ArmVirtGicArchLibConstructor (
> break;
>
> case 2:
> + ASSERT (RegSize == 32);
> +
> + DistBase = fdt64_to_cpu (((UINT64 *)Reg)[0]);
> + CpuBase = fdt64_to_cpu (((UINT64 *)Reg)[2]);
> + ASSERT (DistBase < MAX_UINT32);
> + ASSERT (CpuBase < MAX_UINT32);
> +
> + PcdSet32 (PcdGicDistributorBase, (UINT32)DistBase);
> + PcdSet32 (PcdGicInterruptInterfaceBase, (UINT32)CpuBase);
> +
> + DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
> +
> mGicArchRevision = ARM_GIC_ARCH_REVISION_2;
> break;
>
This looks good to me (with the above remarks in mind).
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
