Re: portability of ShellPkg

[Andrew Fish <afish@apple.com>]

> On Sep 5, 2018, at 11:43 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> 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()
> 

Laszlo,

gEfiMemoryTypeInformationGuid is an edk2/MdeModulePkg concept used to give the DXE Core hints on how to reduce fragmentation in the memory map. Typically there is code in PEI that creates a HOB and may consume a variable written by the BDS. This library seems to be the generic way to do it on an edk2 platform. 

Thus this library is not just PI but edk2 MdeModulePkg specific in some of its assumptions. In general this UefiBootManagerLib seems focused on construction and edk2 BDS. It would probably make more sense to break out the UEFI Spec related bits so they could be used in generic UEFI Applications. 

Thanks,

Andrew Fish


> 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