From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::229; helo=mail-it0-x229.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2088721B00DC1 for ; Tue, 21 Nov 2017 12:28:00 -0800 (PST) Received: by mail-it0-x229.google.com with SMTP id o130so8035096itg.0 for ; Tue, 21 Nov 2017 12:32:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=fOsO3sS6PEIYJOvb4slFSeWShMLdpV5MBs6nY1km8Qw=; b=e9cNeQvTeoAANRmEevuH/BJldAaxRRmWonTRi3SDnSnAxMsTBY5Ai7SpvzrUrZ1eqK cctpegsNFsP6vLI2Kb4XcQy+TGbEwioob7LtBGu7EcR9kFjY4fUoX9hyr9O+sbFbwV7L /iBTC94Q0Yg1bNJdChLHoskavZNswkEwPrnYY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=fOsO3sS6PEIYJOvb4slFSeWShMLdpV5MBs6nY1km8Qw=; b=nN/bj7kDC6GZf+zob3hXFYR/nfIdnGfawmNKtjzqi3dI3B/41sWnufHNvoMsOHU7sb WcZS6A+uZa96OvhV49SpsoSSZpUEI0w1omadL4v0y24CCgVvWpDEB5oiH9hTA7H0qNFd 8QiODfXMeu5G16K+Le/1eAn70sDQjrWQsHEV243mIJbswImNeJEpSAXHxbJcKIq/WodN bhytb7Sm1KSvbiThKzHbmIPtyVJ3tmJxBELjfXEfADvObR0fzzQwENyUlk49l1pxCXYm UUQbyO8J9KPLfZsita2tzD9/lWE+XcsJ8Rzdnp9KRZ6dQ3FZ00wkuK6QOOoC+WzEfPoo vsMQ== X-Gm-Message-State: AJaThX59expzhDLJSkGntbfzG3Jq2RMeQX3kAX5e9/G7D+tymEQMLmVq XJjS3NKEgY8MISwH0THDc5VEJ4e+xg6j3Cmuz/gZPQ== X-Google-Smtp-Source: AGs4zMZp3ts28bV0cfv75x13zIo1VqN3CMvoB409TXg7JdacEPxEzrxtEoRLXgPnbZi4cdid4n7n1DvosmO3F5jhGbI= X-Received: by 10.36.141.70 with SMTP id w67mr3616782itd.58.1511296335683; Tue, 21 Nov 2017 12:32:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Tue, 21 Nov 2017 12:32:14 -0800 (PST) In-Reply-To: <4e079219-e2e1-20b1-6772-97249925a4bf@redhat.com> References: <20171117160913.17292-1-ard.biesheuvel@linaro.org> <20171117160913.17292-14-ard.biesheuvel@linaro.org> <0795247f-2296-e486-50f5-0ba4caaa150b@redhat.com> <4D4989D0-DC49-40D2-A20A-9B14A0973BA4@linaro.org> <4e079219-e2e1-20b1-6772-97249925a4bf@redhat.com> From: Ard Biesheuvel Date: Tue, 21 Nov 2017 20:32:14 +0000 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Nov 2017 20:28:01 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 21 November 2017 at 20:17, Laszlo Ersek wrote: > On 11/21/17 18:57, Ard Biesheuvel wrote: >> >> >>> On 21 Nov 2017, at 17:49, Laszlo Ersek 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; thank= s >>> 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 preferre= d >>> 74 chars width for such sections): >>> >>> ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQem= u.dsc] >>> InitializeMemory() [ArmPlatformPkg/Memory= InitPei/MemoryInitPeim.c] >>> ArmPlatformInitializeSystemMemory() [ArmVirtPkg/Library/Ar= mVirtPlatformLib/Virt.c] >>> // >>> // set PcdSystemMemorySize from the DT >>> // >>> MemoryPeim() [ArmVirtPkg/Library/Ar= mVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c] >>> InitMmu() [ArmVirtPkg/Library/Ar= mVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c] >>> ArmPlatformGetVirtualMemoryMap() [ArmVirtPkg/Library/Ar= mVirtPlatformLib/VirtMem.c] >>> // >>> // consume PcdSystemMemorySize >>> // >>> >>> Here we have two ArmVirtPlatformLib actions: >>> >>> - The "PCD consumption" half of that has been moved -- well, copied, fo= r >>> 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=E2=80=99re right. Direct dependencies between libraries with= constructors are handled correctly by the tools, I simply managed to confu= se myself due to the issues with transitive dependencies which you surely r= emember. > > 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" unde= r >>> [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=E2=80=99re right. >> >> So how do you propose i go about creating two versions of QemuVirtMemInf= oLib, only one of which has a constructor? Share the .c file between two in= fs 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 thos= e. > 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