Re: portability of ShellPkg

["Carsey, Jaben" <jaben.carsey@intel.com>]

But that is the line Leif wants to remove... the library from the components section.  Looks just like MdePkg to me... what is different?

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Andrew Fish
> Sent: Wednesday, September 05, 2018 11:33 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Heinrich Schuchardt
> <xypron.glpk@gmx.de>; edk2-devel@lists.01.org; Alexander Graf
> <agraf@suse.de>; AKASHI Takahiro <takahiro.akashi@linaro.org>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: Re: [edk2] portability of ShellPkg
> Importance: High
> 
> 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/UefiShellLevel2Comma
> ndsLib.inf
> >> ShellPkg/ShellPkg.dsc:111:
> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1Comma
> ndsLib.inf
> >> ShellPkg/ShellPkg.dsc:112:
> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3Comma
> ndsLib.inf
> >> ShellPkg/ShellPkg.dsc:114:
> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Com
> mandsLib.inf
> >> ShellPkg/ShellPkg.dsc:115:
> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1Com
> mandsLib.inf
> >> ShellPkg/ShellPkg.dsc:116:
> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Com
> mandsLib.inf
> >> ShellPkg/ShellPkg.dsc:117:
> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1
> CommandsLib.inf
> >> ShellPkg/ShellPkg.dsc:118:
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2
> CommandsLib.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/Ue
> fiBootManagerLib.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/DxeHobLi
> b/HobLib.c#L54
> <https://github.com/tianocore/edk2/blob/master/MdePkg/Library/DxeHob
> Lib/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/UefiShellDriver1CommandsL
> ib.inf
> >>>>>>
> ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLi
> b.inf
> >>>>>> -
> ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Commands
> Lib.inf
> >>>>>>
> ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Comm
> andsLib.inf
> >>>>>>
> ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Comm
> andsLib.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
> >>>>
> >>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel