From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=SJfAnmYj; spf=pass (domain: linaro.org, ip: 209.85.221.67, mailfrom: leif.lindholm@linaro.org) Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by groups.io with SMTP; Tue, 27 Aug 2019 13:59:30 -0700 Received: by mail-wr1-f67.google.com with SMTP id u16so237868wrr.0 for ; Tue, 27 Aug 2019 13:59:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=oBWwwY8xJ1JwWil/MOBtYDYOIQBG/D4RMaVVL/wq5IE=; b=SJfAnmYjGe7VuEfpmMgYTN8QdZht/rWtMqDONmUqGoI7P+CMKmRXvUsMVvd7TMUoka EBP/soW8BLJpKRcRNzG0bxLdlGIPatNUhI4A5O7NzwGkZ+bcI2Nco4X2X+VaUtLHWB70 AYqHf6BivCulGYwrl3ajW5yuWX2c3M5QugsLhs4nbQdHNx9mXZm1UliTV+z79wKh49O1 0DlvKfSmQYhBE7aBIzTU5WZ3O8VezpCQj5WsjAP9W0gAiYP7MGgYiD1sgn68IsEHZXNU RYVOMvapw2HjatttlxejGMY2TaJfzJyyIqb9MhvICSZfddRO54zG5JIwxXSNtRwxU8z9 pEng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=oBWwwY8xJ1JwWil/MOBtYDYOIQBG/D4RMaVVL/wq5IE=; b=WEo3EC5YRJoyLnBi5wdDVUTZB3n9KeL2fntEWbHJ61SRySoQe0eePzH7hVTmxS0f6M ChrstonMu6vQqU5vrwbdkM5ycurS1cPDfthTgKvQpwhfOOc9CVBCseWJLLDih9hF5CkO FuLuL8eRVmfbYjYkNxx+46ryqzHOw84AkrS+Ga9n3W/0hyZa9RFfvuhuaWSn+I+0Wh8L EPsnn5+4SbmZ7ZrrRr9DSxooJiHrEJOtIv2gOuLvlT4fF7wwUZjAr0kazpD3a9FR+aEP 7YXiZpk/ejo+g5yVF3rzPduE3UE1UcYZHUVtG8xo0hjOplr7V25wlr+AE72tv1Ckz3a2 lHrg== X-Gm-Message-State: APjAAAUGM4C+6LfOyCBXmmh+13oXYahtlj2YnBJd+eUW/zLfGMtNVw3O t7nLS97qE8WhCrl/3NZTAs/xZA== X-Google-Smtp-Source: APXvYqzEcFfTrymxGka9tjXP1M0hz2aQnwg9hjZLqkk1OBZDbggIy2IsVMWGan4tglX+uNCi+rBbYA== X-Received: by 2002:adf:8004:: with SMTP id 4mr123879wrk.341.1566939568685; Tue, 27 Aug 2019 13:59:28 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id m7sm606006wmi.18.2019.08.27.13.59.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Aug 2019 13:59:28 -0700 (PDT) Date: Tue, 27 Aug 2019 21:59:26 +0100 From: "Leif Lindholm" To: Laszlo Ersek Cc: devel@edk2.groups.io, Andrew Fish , Michael D Kinney , Baptiste Gerondeau , Jian J Wang , Hao A Wu , "Feng, Bob C" , Liming Gao Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more) Message-ID: <20190827205926.GG29255@bivouac.eciton.net> References: <20190827124328.9034-1-leif.lindholm@linaro.org> <6ce1988a-bd79-893e-5d8d-724b98329ab9@redhat.com> MIME-Version: 1.0 In-Reply-To: <6ce1988a-bd79-893e-5d8d-724b98329ab9@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline +Bob, Liming, On Tue, Aug 27, 2019 at 09:26:05PM +0200, Laszlo Ersek wrote: > Hi Leif, > > On 08/27/19 14:43, Leif Lindholm wrote: > > Commit 4a1f6b85c184 > > ("MdeModulePkg: add LockBoxNullLib for !IA32/X64 in .dsc") > > added an ARM/AARCH64 resolution for LockBoxLib. However, this failed > > to address the overrides provided for PEIM, DXE_DRIVER, > > DXE_RUNTIME_DRIVER, DXE_SMM_DRIVER and UEFI_DRIVER, so any modules > > of those classes still failed to build. > > > > Break these out properly into their own LibraryClasses sections. > > > > Resolves BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2134 > > > > Signed-off-by: Leif Lindholm > > Reported-by: Baptiste Gerondeau > > Cc: Jian J Wang > > Cc: Hao A Wu > > --- > > > > I don't understand how the above would appear to work back when I > > submitted the previous patch but not work now, but I haven't dug > > into it deeper. Including the x86-specific LockBoxLib in the > > .common section is however clearly not correct. > > I agree with you that the present situation is not correct. > > According to: > > https://edk2-docs.gitbooks.io/edk-ii-dsc-specification/2_dsc_overview/26_[libraryclasses]_section_processing.html > > the library class resolutions take effect in the following order > (entries near the top have higher priority): > > > 1. associated with the INF file in the [Components] section > > 2. [LibraryClasses.$(Arch).$(MODULE_TYPE), LibraryClasses.$(Arch).$(MODULE_TYPE)] > > 3. [LibraryClasses.$(Arch).$(MODULE_TYPE)] > > 4. [LibraryClasses.common.$(MODULE_TYPE)] > > 5. [LibraryClasses.$(Arch)] > > 6. [LibraryClasses.common] or [LibraryClasses] > > (Side comment 1: levels #2 and #3 look very similar; I think the > difference is that #2 is supposed to be a multi-arch and/or > multi-module-type section, while #3 is a single-arch and > single-module-type section.) > > Commit 4a1f6b85c184 ("MdeModulePkg: add LockBoxNullLib for !IA32/X64 in > .dsc", 2019-03-27) provided a LockBoxLib resolution at level #5: Yes. > > [LibraryClasses.ARM, LibraryClasses.AARCH64] > > However, the other LockBoxLib resolutions are at level #4: > > > [LibraryClasses.common.PEIM] > > [LibraryClasses.common.DXE_DRIVER] > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > [LibraryClasses.common.DXE_SMM_DRIVER] > > [LibraryClasses.common.UEFI_DRIVER] > > So the latter taking priority is actually specified behavior. Hmm. That's not great. Anyway, I stopped being lazy and did a bisect. The culprit is e8449e1d8e3b ("BaseTools: Decouple AutoGen Objects"), marked as resolving https://bugzilla.tianocore.org/show_bug.cgi?id=1875. This also affects SignedCapsulePkg/SignedCapsulePkg.dsc (although once addressed, AARCH64 also needs a NULL entry added for CompilerIntrinsicsLib. > (Side comment 2: EBC is in the same boat, from commit cbcccd2c9d93 > ("Update Code to pass EBC compiler", 2013-05-13): > > > [LibraryClasses.EBC] > > LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf > ) > > As to why this breakage was not exposed right at commit 4a1f6b85c184 -- > I have no idea. Perhaps it was hidden by a BaseTools issue that has been > fixed meanwhile. Yes. But it is also a fundamental change in tool behaviour introduced on 9 August. I am really uncomfortable about this making it into the release this week - but I also believe this is the foundation for the multiprocess autogen. Since you have very helpfully analyzed *what* changed ... would the better "fix" for 2019.08 be to intentionally break the new code to conform to the old behaviour - and then revert that patch after the tag? If we do that, this patch could then wait and indeed be merged as part of the same set. > On 08/27/19 14:43, Leif Lindholm wrote: > > I think a fix for this issue needs to go into 2019.08, > > I agree the problem should be fixed in 2019.08 -- taking your word that > commit 4a1f6b85c184 *appeared* to fix the MdeModulePkg.dsc build for > ARM/AARCH64, we now have a regression since that commit (dated > 2019-03-27). > > > but if someone has a prettier suggestion, I am not wedded to this one. > > I think this is good enough. The lib class resolutions are raised to > level #2, but they will no longer match ARM / AARCH64, so your level#5 > addition from commit 4a1f6b85c184 will take effect. > > > > > MdeModulePkg/MdeModulePkg.dsc | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc > > index 4320839abfb5..15ba96cecbed 100644 > > --- a/MdeModulePkg/MdeModulePkg.dsc > > +++ b/MdeModulePkg/MdeModulePkg.dsc > > @@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM] > > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf > > ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSectionLib/PeiExtractGuidedSectionLib.inf > > + > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM] > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf > > (1) I suggest replacing "," with ", ". (That's more consistent with > preexistent section headers in the DSC file.) Applies to the other new > section headers too. Yes, fair point. > > > > [LibraryClasses.common.DXE_CORE] > > @@ -118,18 +120,22 @@ [LibraryClasses.common.DXE_CORE] > > > > [LibraryClasses.common.DXE_DRIVER] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > - LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > > ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.inf > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > > > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_DRIVER] > > + LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > + > > [LibraryClasses.common.DXE_RUNTIME_DRIVER] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > > - LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf > > > > +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X64.DXE_RUNTIME_DRIVER] > > + LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > + > > [LibraryClasses.common.SMM_CORE] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib/PiSmmCoreMemoryAllocationLib.inf > > @@ -143,13 +149,17 @@ [LibraryClasses.common.DXE_SMM_DRIVER] > > MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf > > MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf > > + SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > + > > +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.DXE_SMM_DRIVER] > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.inf > > - SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf > > > > I wonder if this is really necessary. I would assume all the > DXE_SMM_DRIVER modules are listed under > > [Components.IA32, Components.X64] > > already. But, this hunk certainly doesn't hurt. Well, this fixes the current issue. I completely agree the file could benefit from some overall restructuring. > > [LibraryClasses.common.UEFI_DRIVER] > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf > > DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf > > + > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI_DRIVER] > > LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > With (1) fixed (feel free to correct that just before pushing): > > Reviewed-by: Laszlo Ersek Thanks! > Do wait for maintainer review, of course. Of course. Best Regards, Leif