On 04/07/16 13:44, Ard Biesheuvel wrote: > On 7 April 2016 at 13:18, Laszlo Ersek <[email protected]> wrote:
>> 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. Right. Anyway, as we've repeatedly determined from both practice and the INF file spec, the MODULE_TYPE for a library instance doesn't seem to affect anything beyond the constructor's signature (if any). So, let's leave these the way 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... >> > > 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) I certainly support the full removal of the linux loader app from ArmVirtQemu; if it eases patch review for me, then it's already worth it (for me). Feel free to post a separate patch for it, I guess. >> 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 I mentioned the build report file, but I guess it bears some stressing -- it is super helpful. I don't like to rely on the build report file exclusively, but it is very good for verifying a hypothesis. Even the PCD usage (see (22)(a)) can be confirmed with it! [snip] >>> 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, It is, to my knowledge. > then I should probable just drop this hunk (and the one > that changes the constructor signature) I agree. [snip] >> (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 Heh, very good point! So, I guess I'll await your v3, re-check patches 01-07 (wherever I had comments), and resume with v3 08/21. Is that alright? Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
