From: Andrew Fish <afish@apple.com>
To: "Carsey, Jaben" <jaben.carsey@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Ni, Ruiyu" <ruiyu.ni@intel.com>, Alexander Graf <agraf@suse.de>,
Heinrich Schuchardt <xypron.glpk@gmx.de>,
AKASHI Takahiro <takahiro.akashi@linaro.org>,
Mike Kinney <michael.d.kinney@intel.com>,
Laszlo Ersek <lersek@redhat.com>
Subject: Re: portability of ShellPkg
Date: Wed, 05 Sep 2018 11:33:18 -0700 [thread overview]
Message-ID: <E9100386-06EA-49BE-99B2-1CE799B90B7A@apple.com> (raw)
In-Reply-To: <8BDDD669-C118-4C7E-9A99-896B844EFA56@intel.com>
Jaben,
Sorry to test compile a library just list the library in the [Components] section of the DSC file. MdePkg/MdePkg.dsc is an example of this.
Thanks,
Andrew Fish
> On Sep 5, 2018, at 11:23 AM, Carsey, Jaben <jaben.carsey@intel.com> wrote:
>
> Aha. So that is very different from a non NULL library when listed in the components section. The goal is to test compile the library but not use I think. Is there any way to do that?
>
> Jaben
>
> On Sep 5, 2018, at 11:21 AM, Andrew Fish <afish@apple.com <mailto:afish@apple.com>> wrote:
>
>>
>>
>>> On Sep 5, 2018, at 11:05 AM, Carsey, Jaben <jaben.carsey@intel.com <mailto:jaben.carsey@intel.com>> wrote:
>>>
>>> But a NULL lib listed in components section shouldn’t be linked in to anything...
>>>
>>
>> Jaben,
>>
>> A NULL library class means force it to be linked in.
>>
>> ShellPkg/ShellPkg.dsc:70: # [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
>> ShellPkg/ShellPkg.dsc:72: NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> ShellPkg/ShellPkg.dsc:75: NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
>> ShellPkg/ShellPkg.dsc:78: NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> ShellPkg/ShellPkg.dsc:110: NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:111: NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:112: NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:114: NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:115: NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:116: NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:117: NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
>> ShellPkg/ShellPkg.dsc:118: NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
>>
>> Libraries can get pulled in via other libraries. The only way to tell for sure is to look at the build report.
>>
>> $ build -p ShellPkg/ShellPkg.dec -a X64 -t XCODE5 --report-file=report.txt
>> $ cat report.txt | grep HobLib
>> /Volumes/Case/UDK2018/MdePkg/Library/DxeHobLib/DxeHobLib.inf
>> {HobLib: C = HobLibConstructor Time = 19ms}
>>
>> You can comment out the HobLib reference in the ShellPkg.dsc file and figure out who is using it "#### ffHobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf"
>> $ build -p ShellPkg/ShellPkg.dec -a X64 -t XCODE5 --report-file=report.txt
>> ...
>> build.py...
>> /Volumes/Case/UDK2018/ShellPkg/ShellPkg.dsc(...): error 4000: Instance of library class [HobLib] is not found
>> in [/Volumes/Case/UDK2018/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf] [X64]
>> consumed by module [/Volumes/Case/UDK2018/ShellPkg/Application/Shell/Shell.inf]
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>> Unless is listed below with the shell INF also, it just test compiles it...
>>>
>>> Or so I thought.
>>>
>>> On Sep 5, 2018, at 11:04 AM, Andrew Fish <afish@apple.com <mailto:afish@apple.com>> wrote:
>>>
>>>>
>>>>
>>>>> On Sep 5, 2018, at 10:30 AM, Carsey, Jaben <jaben.carsey@intel.com <mailto:jaben.carsey@intel.com>> wrote:
>>>>>
>>>>> How does removing a lib from the components section affect the shell binary output?
>>>>>
>>>>> Is the asset at compile time?
>>>>
>>>> Jaben,
>>>>
>>>> The issue is likely with the HOB lib constructor in the Shell iASSERTing. Leif's example platform supports UEFI, but not PI so it is not expected that HOBs exist.
>>>>
>>>> The library has an explicit assumption that HOBs exist, and that is not the case in Leif's platform.
>>>>
>>>> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/DxeHobLib/HobLib.c#L54 <https://github.com/tianocore/edk2/blob/master/MdePkg/Library/DxeHobLib/HobLib.c#L54>
>>>>
>>>>
>>>> VOID *mHobList =
>>>> NULL;
>>>>
>>>>
>>>> /**
>>>>
>>>> Returns the pointer to the HOB list.
>>>>
>>>>
>>>> This function returns the pointer to first HOB in the list.
>>>>
>>>> For PEI phase, the PEI service GetHobList() can be used to retrieve the pointer
>>>>
>>>> to the HOB list. For the DXE phase, the HOB list pointer can be retrieved through
>>>>
>>>> the EFI System Table by looking up theHOB list GUID in the System Configuration Table.
>>>>
>>>> Since the System Configuration Table does not exist that the time the DXE Core is
>>>>
>>>> launched, the DXE Core uses a global variable from the DXE Core Entry Point Library
>>>>
>>>> to manage the pointer to the HOB list.
>>>>
>>>>
>>>> If the pointer to the HOB list is NULL, then ASSERT().
>>>>
>>>>
>>>> This function also caches the pointer to the HOB list retrieved.
>>>>
>>>>
>>>> @return The pointer to the HOB list.
>>>>
>>>>
>>>> **/
>>>>
>>>> VOID *
>>>>
>>>> EFIAPI
>>>>
>>>> GetHobList (
>>>>
>>>> VOID
>>>>
>>>> )
>>>>
>>>> {
>>>>
>>>> EFI_STATUS Status;
>>>>
>>>>
>>>> if (mHobList ==
>>>> NULL) {
>>>>
>>>> Status =
>>>> EfiGetSystemConfigurationTable (&gEfiHobListGuid, &mHobList);
>>>>
>>>> ASSERT_EFI_ERROR (Status);
>>>>
>>>> ASSERT (mHobList !=
>>>> NULL);
>>>>
>>>> }
>>>>
>>>> return mHobList;
>>>>
>>>> }
>>>>
>>>>
>>>> /**
>>>>
>>>> The constructor function caches the pointer to HOB list by calling GetHobList()
>>>>
>>>> and will always return EFI_SUCCESS.
>>>>
>>>>
>>>> @param ImageHandle The firmware allocated handle for the EFI image.
>>>>
>>>> @param SystemTable A pointer to the EFI System Table.
>>>>
>>>>
>>>> @retval EFI_SUCCESS The constructor successfully gets HobList.
>>>>
>>>>
>>>> **/
>>>>
>>>> EFI_STATUS
>>>>
>>>> EFIAPI
>>>>
>>>> HobLibConstructor (
>>>>
>>>> IN EFI_HANDLE ImageHandle,
>>>>
>>>> IN EFI_SYSTEM_TABLE *SystemTable
>>>>
>>>> )
>>>>
>>>> {
>>>>
>>>> GetHobList ();
>>>>
>>>>
>>>> return EFI_SUCCESS;
>>>>
>>>> }
>>>>
>>>>
>>>>
>>>> /**
>>>>
>>>> Returns the next instance of a HOB type from the starting HOB.
>>>>
>>>>
>>>> This function searches the first instance of a HOB type from the starting HOB pointer.
>>>>
>>>> If there does not exist such HOB type from the starting HOB pointer, it will return NULL.
>>>>
>>>> In contrast with macro GET_NEXT_HOB(), this function does not skip the starting HOB pointer
>>>>
>>>> unconditionally: it returns HobStart back if HobStart itself meets the requirement;
>>>>
>>>> caller is required to use GET_NEXT_HOB() if it wishes to skip current HobStart.
>>>>
>>>>
>>>> If HobStart is NULL, then ASSERT().
>>>>
>>>>
>>>> @param Type The HOB type to return.
>>>>
>>>> @param HobStart The starting HOB pointer to search from.
>>>>
>>>>
>>>> @return The next instance of a HOB type from the starting HOB.
>>>>
>>>>
>>>> **/
>>>>
>>>> VOID *
>>>>
>>>> EFIAPI
>>>>
>>>> GetNextHob (
>>>>
>>>> IN UINT16 Type,
>>>>
>>>> IN CONST VOID *HobStart
>>>>
>>>> )
>>>>
>>>> {
>>>>
>>>> EFI_PEI_HOB_POINTERS Hob;
>>>>
>>>>
>>>> ASSERT (HobStart !=
>>>> NULL);
>>>>
>>>>
>>>> Hob.Raw = (UINT8 *) HobStart;
>>>>
>>>> //
>>>>
>>>> // Parse the HOB list until end of list or matching type is found.
>>>>
>>>> //
>>>>
>>>> while (!END_OF_HOB_LIST (Hob)) {
>>>>
>>>> if (Hob.Header->HobType == Type) {
>>>>
>>>> return Hob.Raw;
>>>>
>>>> }
>>>>
>>>> Hob.Raw =
>>>> GET_NEXT_HOB (Hob);
>>>>
>>>> }
>>>>
>>>> return
>>>> NULL;
>>>>
>>>> }
>>>>
>>>> Thanks,
>>>>
>>>> Andrew Fish
>>>>
>>>>>
>>>>> Jaben
>>>>>
>>>>>> On Sep 5, 2018, at 10:26 AM, Leif Lindholm <leif.lindholm@linaro.org <mailto:leif.lindholm@linaro.org>> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> (This is partly a summary of discussions that have been held on IRC
>>>>>> and offline, with Alex Graf and Mike Kinney.)
>>>>>>
>>>>>> The UEFI Shell, as produced by the contents of ShellPkg, is needed for
>>>>>> running the UEFI SCT. This has never been problematic before - but now
>>>>>> we are starting to run SCT on the U-Boot implementation of the UEFI
>>>>>> interfaces, certain implicit assumptions may need to be made explicit,
>>>>>> and perhaps reevaluated.
>>>>>>
>>>>>> My feeling is the following:
>>>>>> - The MinUefiShell variant should be sufficient to run SCT.
>>>>>> - The UEFI Shell as provided by ShellPkg (any flavour) should run on
>>>>>> any valid UEFI implementation. Where underlying functionality is
>>>>>> missing for certain commands, those commands should be
>>>>>> degraded/disabled to let remaining commands function.
>>>>>>
>>>>>> Ideally, I would like to see a Readme.md in ShellPkg, basically
>>>>>> providing a mission statement. I could write one, but I expect the
>>>>>> people who actually maintain it would be better suited :)
>>>>>>
>>>>>> We currently have an issue with running the shell on U-Boot because
>>>>>> even MinUefiShell pulls in UefiShellDebug1CommandsLib.inf. This
>>>>>> appears to be inadvertent, since it is also included a few lines
>>>>>> further down inside an !ifndef $(NO_SHELL_PROFILES) guard.
>>>>>> So I would propose the following patch (and can send it out properly
>>>>>> if the maintainers agree):
>>>>>>
>>>>>> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
>>>>>> index 59dd07e0ae..c852abd3f7 100644
>>>>>> --- a/ShellPkg/ShellPkg.dsc
>>>>>> +++ b/ShellPkg/ShellPkg.dsc
>>>>>> @@ -101,7 +101,6 @@ [Components]
>>>>>> ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
>>>>>> ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
>>>>>> ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
>>>>>> - ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
>>>>>> ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
>>>>>> ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
>>>>>>
>>>>>> The reason this causes a problem is because this module has a
>>>>>> dependency on HobLib, which ASSERTS if it does not find any HOBs lying
>>>>>> around. Since HOBs are a PI concept rather than a UEFI concept,
>>>>>> ideally we would not terminate the shell if they are missing. However,
>>>>>> since the HobLib is generic to EDK2, we also shouldn't just go
>>>>>> stripping ASSERTs out of it. The above patch gives us a way of
>>>>>> unblocking the SCT on U-Boot UEFI while we consider what to do about
>>>>>> the bigger question.
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> /
>>>>>> Leif
>>>>
>>
next prev parent reply other threads:[~2018-09-05 18:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-05 17:25 portability of ShellPkg Leif Lindholm
2018-09-05 17:30 ` Carsey, Jaben
2018-09-05 17:41 ` Leif Lindholm
2018-09-05 18:03 ` Andrew Fish
2018-09-05 18:05 ` Carsey, Jaben
2018-09-05 18:20 ` Andrew Fish
2018-09-05 18:23 ` Carsey, Jaben
2018-09-05 18:33 ` Andrew Fish [this message]
2018-09-05 18:53 ` Carsey, Jaben
2018-09-05 18:43 ` Laszlo Ersek
2018-09-05 19:47 ` Andrew Fish
2018-09-06 2:34 ` Ni, Ruiyu
2018-09-06 9:56 ` Laszlo Ersek
2018-09-06 17:17 ` Kinney, Michael D
2018-09-06 22:31 ` Andrew Fish
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E9100386-06EA-49BE-99B2-1CE799B90B7A@apple.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox