On 21 November 2017 at 20:17, Laszlo Ersek <ler...@redhat.com> wrote: > On 11/21/17 18:57, Ard Biesheuvel wrote: >> >> >>> On 21 Nov 2017, at 17:49, Laszlo Ersek <ler...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
