On 04/07/16 14:04, Ard Biesheuvel wrote:
> On 7 April 2016 at 14:02, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 04/07/16 13:44, Ard Biesheuvel wrote:
>>> On 7 April 2016 at 13:18, Laszlo Ersek <ler...@redhat.com> 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?
I certainly don't *want* to ignore it, but I think we have identified
several aspects that may affect many of the remaining patches:
(a) UINT32 vs. UINTN output parameters in the protocol interface
(b) LibFdt usage / SwapBytes()
(c) (important:) for the commit messages, tracking down the clients of
the new plugin library instances that set PCDs (to avoid
multi-setting), recursively; plus tracking down all the consumers
of those PCDs (to ensure they will all see the updated PCD values).
(d) line lengths
(e) argument indentation in function calls
Issues (a), (b), (d) and (e) will continue tripping me up in the review,
and you can fix them up without me actually identifying each individual
location.
Issue (c) requires me to spend multiple tens of minutes per affected
patch. I would prefer if you spent those minutes, wrote up your findings
in the commit messages, so I'd only need verify your findings, not find
them myself from scratch.
In other words, the above issues should be fixed not just for the sake
of the final code and messages that get into the repository, but also in
order to help me review the rest of the series.
Normally I do prefer to review all patches in a series, before asking
for the next version, but I believe the above problems might affect the
rest of the patches. My main worry is that if I keep getting tripped up
by them, I will get de-sensitivised for version 3, and maybe miss
something else that would be important.
Consider, if I review the rest of v2, and point out twenty more
instances of (d) and (e), then when I review v3, I will have to verify
whether you fixed up every single one of those instances. This wastes a
lot of gray matter. :) This is the kind of issue that you can locate
yourself in the rest of the series (now that I've pointed out a few
instances), so they wouldn't even exist to begin with, when I looked at
the rest of the patches, in v3, for the first time.
Of course, I could be wrong too. If you assure me that issues (a)
through (e) will probably not impede my review in the remaining v2
patches :), then I'm more than happy to continue reviewing v2!
To repeat: saving time for me (at the cost of your time) is just the
small goal. The big goal is to preserve my sensitivity to your patches.
Thanks.
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
