On 7 April 2016 at 13:18, Laszlo Ersek <ler...@redhat.com> wrote:
> 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:
>
Please don't ask :-) These predate my involvement with Tianocore, and
my suspicion is that many of these choices are simply based on
whatever happened to produce a working image.
> - 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...
>
Indeed. We would *love* the kill the built in linux loader (bill), and
we don't include it in any AArch64 builds by default. However, like
the ARM BDS, it is used in the wild, and we can't simply remove it
just yet. In fact, now that my 32-bit ARM stub support is upstream, we
should probably drop it from ArmVirtQemu altogether (since QEMU
already has a built in linux loader if you run it without UEFI)
> - ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
>
> I guess this is our main target. This one is therefore certainly
> correct to set the PCDs.
>
Ack
> - 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.
>
Well, at least they don't all have constructors, right?
> 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.
>
Ack
> - 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.
>
Indeed, I will propose a separate patch for that.
> - 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.
>
Ack, Thanks a lot for tracking that down
>
> 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.
>
OK
> ... 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. :)
>
Yes, I think that makes sense.
> 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.
>
I wondered about that. If the only difference is the prototype of the
constructor, then I should probable just drop this hunk (and the one
that changes the constructor signature)
>> 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.
>
Well, I think this is a good opportunity to get rid of these things,
so please don't hesitate pointing them out.
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>
> (25) Given the DepEx, you might want to replace this with an
> ASSERT_EFI_ERROR(). Up to you.
>
Yes. It think I may have done that in another patch in the series.
>> +
>> + 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?
>
Yep
>> + 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?
>
I think SwapBytes () should be preferred, and I think that this is the
only reason the dependency remains. The reason is that the new drivers
don't have access to the DeviceTreeBaseAddress PCD, so many of the
non-trivial libfdt functions are not even callable by them
>> + 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,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
