Re: portability of ShellPkg

[Laszlo Ersek <lersek@redhat.com>]

On 09/05/18 19:25, Leif Lindholm 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?

Such errors can be narrowed down by removing the resolution altogether
from the DSC file, for the library class in question. Then the build
will fail and the error message will report the dependency chain.

With the following patch:

> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
> index 59dd07e0ae62..ebd7f23a8bee 100644
> --- a/ShellPkg/ShellPkg.dsc
> +++ b/ShellPkg/ShellPkg.dsc
> @@ -58,7 +58,6 @@ [LibraryClasses.common]
>    IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
>
>    UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf
> -  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
>    DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
>    DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf

and the following command:

 build -a X64 -p ShellPkg/ShellPkg.dsc \
   -m ShellPkg/Application/Shell/Shell.inf

(obviously one can use any other suitable architecture with "-a"), the
error is:

> ShellPkg/ShellPkg.dsc(...): error 4000: Instance of library class [HobLib] is not found
>         in [MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf] [X64]
>         consumed by module [ShellPkg/Application/Shell/Shell.inf]

Some parts still have to be filled in manually (to "meet in the
middle"), but ultimately, the dependencies go like this:

(1) UefiShellDebug1CommandsLib.inf depends on the BcfgCommandLib class,
    for implementing the BCFG shell command (as required by the shell
    spec, for the Debug1 (and Install1) profiles.

(2) ShellPkg.dsc resolves the BcfgCommandLib class to the sole instance
    in edk2, namely
    "ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.inf".

(3) This library instance depends on the UefiBootManagerLib class (for
    the EfiBootManagerVariableToLoadOption(),
    EfiBootManagerLoadOptionToVariable(), and
    EfiBootManagerFreeLoadOption() APIs).

(4) The edk2 tree provides one instance of the UefiBootManagerLib class,
    and ShellPkg.dsc indeed contains that resolution: namely to
    "MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf".

(5) This library instance depends on the HobLib class. (More on this
    later.)

(6) ShellPkg.dsc resolves the HobLib class to
    "MdePkg/Library/DxeHobLib/DxeHobLib.inf", whose constructor, as
    explained by Andrew, blows up in the absence of the HOB list.

Now the question is where the PI (i.e., non-UEFI) requirement (for the
HOB list) is *incorrectly* introduced, in the abobve dependency chain.
For that, let's investigate (5) more closely:

The only HobLib-dependent function in
"MdeModulePkg/Library/UefiBootManagerLib" seems to be
BmSetMemoryTypeInformationVariable() [BmMisc.c], which calls:

- GetBootModeHob()
- GetFirstGuidHob() -- with gEfiMemoryTypeInformationGuid,
- GET_GUID_HOB_DATA_SIZE()
- GET_GUID_HOB_DATA()

This function goes back to the inception of the library instance (commit
1d1122292572, "MdeModulePkg: Add UefiBootManagerLib", 2015-05-06), so we
cannot narrow down its purpose from that angle.

However, it is clear that just because UefiShellBcfgCommandLib needs
three functions for Boot and Driver option manipulation (which *are*
UEFI concepts), it should not inherit a PI dependency on HOBs, for
tracking the maximum usages of various memory types -- which is entirely
irrelevant for the BCFG command.

There are three options:

One is to implement a HobLib instance that calls ASSERT(FALSE) *plus*
CpuDeadLoop() in all of its functions, and then use that in
ShellPkg.dsc. The shell should never ever touch HOBs (in practice, this
means that BmSetMemoryTypeInformationVariable() should never be reached
in UefiBootManagerLib, when it is used from the shell), so blowing up in
all those functions -- but *not* the constructor -- would be valid. And,
part of this option's appeal is that, with LTO enabled, the compiler
might even eliminate all the HobLib functions (if it can statically
prove that the functions are never reached).

Another (possibly more elegant, but surely more difficult) option is to
split up UefiBootManagerLib somehow, so that a subset of its functions
can be used without PI dependencies.

Yet another option would be to reimplement UefiShellBcfgCommandLib
without a dependency on UefiBootManagerLib. If you think about it,
UefiShellBcfgCommandLib is not (a part of) a UEFI boot manager, so
arguably it shouldn't depend, by design, on the UefiBootManagerLib
class. (This is not the case for e.g. PlatformBootManagerLib
implementations, which *are* parts of the UEFI boot manager in edk2, so
they are allowed to call utility functions from UefiBootManagerLib .) Of
course this just leads back to the previous argument, i.e. Boot#### and
Driver#### manipulation utilities should be available to client code
without having to depend on the whole UefiBootManagerLib class.

Following the path of least resistance, I'd first try the
"BaseHobLibNull" instance (option 1).

Thanks
Laszlo