comments inline On Fri, 26 Aug 2022 at 05:28, Huang, Long1 <long1.hu...@intel.com> wrote: > > Hi All, > > I'm the submitter. >
Hello Huang, Long, Thanks for providing this background - it is very helpful. Some of this should have been recorded in the bugzilla entry or the patch commit log, though, as now, nobody understood the problem or why the fix was necessary to begin with. > ==================Here's the DSC precedence rule from DSC > Spec=================== > The Library Instances will be selected using the following rules to satisfy a > library class for > each module listed in the [Components] section (in order of highest > precedence): > 1. <LibraryClasses> 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] > =========================================================================== > This is incomplete. Before that, the DSC spec says """ The first globally defined library instance, defined in a DSC file, that satisfies a module's requirement for a Library Class, unless specifically overridden by the module in the [Components] section, will be used. """ Should we simply drop that part from the DSC spec? Also, do you happen to have any idea for the rationale of having 2. supersede 3. explicltly? > Background: > Our system takes DXE and later stages as 64-bit by default, using > [LibraryClasses.X64] or appending a more detailed section "MODULE_TYPE"(e.g. > [LibraryClasses.X64.DXE_CORE], [LibraryClasses.X64.DXE_SMM_DRIVER]). For SEC > or PEI, use [LibraryClasses.IA32] or [LibraryClasses.IA32.SEC], > [LibraryClasses.IA32.PEIM], etc. It works fine for traditional. > > With the arrival of pure X64, there will be some linking issues in this way. > Because which library to link against cannot be determined simply using ARCH. > During the development of X64 PEI, we found that the actual behavior of > BaseTools for DSC precedence: > [LibraryClasses.common.PEIM] < [LibraryClasses.X64] > [LibraryClasses.IA32.PEIM] > [LibraryClasses.X64] > [LibraryClasses.X64.PEIM] > [LibraryClasses.X64] > > We can see that this does not match the order of 4 and 5 in the spec. > > So when we tried to use "[LibraryClasses.common.MODULE_TYPE]" such the common > ways to support both X64 and IA32 SEC & PEI and avoid duplication, before the > fixed patch, modules in the PEI stage will linking DXE and later stage's > libraries first(under [LibraryClasses.X64]), it will cause many puzzling > linking issues. After the fixed patch downstream, we setup the X64 PEI build > target, and works well for several days(both for IA32 and X64 SEC & PEI > build). > OK. The problem, however, is that many platform definitions may exist that actually rely on this behavior. And the symptom could be something that only manifests itself at runtime. > For Ard Biesheuvel's question: > I think it may be the LockBoxLib under [LibraryClasses.ARM, > LibraryClasses.AARCH64] doesn't provide the MODULE_TYPE section, it can work > before this patch as I said above. But now it should be replaced with > [LibraryClasses.ARM.MODULE_TYPE, LibraryClasses.AARCH64.MODULE_TYPE] to get > higher priority than LockBoxLib under [LibraryClasses.comon.MODULE_TYPE]. > Thanks for clarifying that. But fixing the MdeModulePkg build is not the priority here. The MdeModulePkg breakage is the canary in the coalmine here, and tells us that making changes to the build tools like this is dangerous, and may affect many users of this code base. > Conclusion: > From my point of view, I think the DSC precedence rule defined by spec is > reasonable and make sense. MinPlatform gives us a good example, using the > method of [LibraryClasses.common.MODULE_TYPE]. Do not assume the current > arch, use "common" instead, then always append "MODULE_TYPE" section. It's > clear, compatible and unambiguous. So we strongly support the idea of the DSC > spec: Unless specified under [Components], then more detailed sections, > higher priority. I think it will benefit future development. > Doesn't this mean that you should not be using [LibraryClasses.X64] to begin with? Or am I misunderstanding the point you are making? Because in that case, the problem would not exist either. > Short term solution: > we can agree to temporarily revert this patch for stable tag if we can't fix > the MdeModulePkg build issues or other potential problems immediately. For > us, we will downstream overwrite it to keep this change to support the X64 > build target. > Fixing MdeModulePkg is easy. Fixing all the DSCs out in the world that may silently break because of this change is much harder. > Long term solution: > We suggest that we can discuss the rationale of this rule and evaluate if > this patch should be reintroduced after the stable tag. > I agree. But we should clarify the DSC spec first, and at least consider aligning the spec with the existing code (which has been in wide use for many years) instead of the other way around. Thanks, Ard. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#92860): https://edk2.groups.io/g/devel/message/92860 Mute This Topic: https://groups.io/mt/93231667/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
