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

[Laszlo Ersek <lersek@redhat.com>]

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* :)

> 
>>> 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.

Thanks!
Laszlo