On 7 April 2016 at 14:02, Laszlo Ersek <[email protected]> wrote: > 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? >
Does that mean you are going to ignore my v2? _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
