Re: portability of ShellPkg

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

But a NULL lib listed in components section shouldn’t be linked in to anything...

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

        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