Re: [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 21 November 2017 at 20:17, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/21/17 18:57, Ard Biesheuvel wrote:
>>
>>
>>> On 21 Nov 2017, at 17:49, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 11/17/17 17:09, Ard Biesheuvel wrote:
>>>> Equivalent to the PrePi based platforms, this patch implements the
>>>> newly introduced ArmVirtMemInfo library class via a separate PEIM
>>>> and PPI.
>>>>
>>>> The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
>>>> API function ArmPlatformInitializeSystemMemory () to retrieve memory
>>>> information from the DT, ensuring that it will be present when
>>>> MemoryPeim() is executed. This is a bit of a hack, and someting we
>>>> will need to get rid of if we want to reduce our dependency on
>>>> ArmPlatformLib altogether.
>>>
>>> OK, so whenever I try to look at this code, my brain crashes. This
>>> occasion is no exception. All the more reason to clean it all up; thanks
>>> for doing that.
>>>
>>> So, I guess we are talking about the following call stack. If you agree,
>>> please add it to the commit message (IMO it's OK to exceed the preferred
>>> 74 chars width for such sections):
>>>
>>>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
>>>    InitializeMemory()                            [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>>      ArmPlatformInitializeSystemMemory()         [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>>>        //
>>>        // set PcdSystemMemorySize from the DT
>>>        //
>>>      MemoryPeim()                                [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>>        InitMmu()                                 [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>>          ArmPlatformGetVirtualMemoryMap()        [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>>>            //
>>>            // consume PcdSystemMemorySize
>>>            //
>>>
>>> Here we have two ArmVirtPlatformLib actions:
>>>
>>> - The "PCD consumption" half of that has been moved -- well, copied, for
>>>  now -- to QemuVirtMemInfoLib, in patch 12/15.
>>>
>>> - And we are now looking for a new home for the "PCD production" half.
>>>
>>
>> Yes
>>
>>>> Putting this code in a ArmVirtMemInfo library constructor is
>>>> problematic as well, given that the implementation uses other
>>>> libraries, among which PcdLib, and so we need to find another way to
>>>> run it before MemoryPeim()
>>>
>>> Hm, this claim could be true :) , but I'm not totally convinced by the
>>> way you put it.
>>>
>>> First off, I notice that
>>> "ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
>>> spell out PcdLib as a dependency. This is quite bad, because it uses
>>> PCDs.
>>>
>>> However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
>>> of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
>>> This must be due to some of the library instances already pulling in
>>> PeiPcdLib.
>>>
>>> Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
>>> the DT and to set the PCD, *plus* we spell out PcdLib in the
>>> QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
>>> MemoryInitPeim module should remain the same. And, the PeiPcdLib
>>> constructor should run before the QemuVirtMemInfoLib constructor
>>> (parsing the DT and setting the PCD).
>>>
>>> What's wrong with this argument?
>>>
>>
>> I guess you’re right. Direct dependencies between libraries with constructors are handled correctly by the tools, I simply managed to confuse myself due to the issues with transitive dependencies which you surely remember.
>
> Right, I suspected that this experience was in the background. If I
> remember correctly, the issue was when some libraries had constructors
> while some others had none (and required explicit init function calls
> instead). In some cases this mixing was not possible to avoid due to
> circular dependencies between constructors, but in turn the explicit
> calls didn't get ordered right, or some such. *shudder* :)
>

The core of the issue is that transitive library dependencies are not
honoured in the ordering of constructor invocations if they pass
through a library that has no constructor itself.

I.e., libA with a constructor

depending on libB with no constructor

depending on libC with a constructor

Currently, libA's constructor may be called before libC's constructor,
which is clearly a bug, and which is the reason why I /think/ we
generally shouldn't be relying on other libraries in constructor
implementations.

>>
>>>> So instead, create a separate PEIM that encapsulates the
>>>> ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
>>>> library class implementation is also provided that depexes on the PPI,
>>>> which ensures that the code is called in the correct order.
>>>
>>> I understand what this does, but I find it very complex.
>>>
>>> Sometimes, whenever we want to make sure that a PCD is set dynamically
>>> "no later than" it's consumed by a "common" module outside of
>>> ArmVirtPkg, we create a dedicated library instance (with all the right
>>> library dependencies spelled out in its INF file), set the PCD in its
>>> constructor, and hook it into the consumer module via NULL class
>>> resolution.
>>>
>>> In this case, we have a lib instance that is used through an actual lib
>>> class already, so I would suggest to add a constructor function, and to
>>> spell out the PcdLib dependency in the INF file.
>>>
>>> ... In fact, this is something that I missed in patch 12 --
>>> QemuVirtMemInfoLib already uses PCDs, but doesn't include "PcdLib" under
>>> [LibraryClasses]. That is a bug inherited from ArmVirtPlatformLib (see
>>> above), and should be fixed. And then we should only need the
>>> constructor.
>>>
>>> What do you think?
>>>
>>
>> I think you’re right.
>>
>> So how do you propose i go about creating two versions of QemuVirtMemInfoLib, only one of which has a constructor? Share the .c file between two infs in the same directory?
>
> Hm, I think I missed the impact on ArmVirtQemuKernel. In the current
> set, its DSC file is only modified in patch 12. (I missed that too.) Are
> any changes necessary for ArmVirtQemuKernel that are not contained in
> this set?
>
> Either way, what you propose above seems to be the standard approach to
> me: use two INF files, keep the bulk of the code in one (shared) C file,
> and add the constructor to another (non-shared) C file (i.e., referenced
> by only one of the INF files). Should the constructor need shared
> utility functions from the shared C file, add an internal header for those.
>

Would you object to having a single .c file, but only declare the
constructor in one of the two .INFs? The code will be pruned anyway,
due to our use of -ffunction-sections