portability of ShellPkg

portability of ShellPkg

[Leif Lindholm]

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

Re: portability of ShellPkg

[Carsey, Jaben]

How does removing a lib from the components section affect the shell binary output?

Is the asset at compile time?

Jaben

> On Sep 5, 2018, at 10:26 AM, Leif Lindholm <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

Re: portability of ShellPkg

[Leif Lindholm]

On Wed, Sep 05, 2018 at 05:30:23PM +0000, Carsey, Jaben wrote:
> How does removing a lib from the components section affect the shell binary output?

Maybe it doesn't and I'm barking up the wrong tree? Unfortunately,
the only thing that means is we don't have a trivial workaround.

/
    Leif

> Is the asset at compile time?
> 
> Jaben
> 
> > On Sep 5, 2018, at 10:26 AM, Leif Lindholm <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

Re: portability of ShellPkg

[Andrew Fish]

> On Sep 5, 2018, at 10:30 AM, Carsey, Jaben <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> 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

Re: portability of ShellPkg

[Carsey, Jaben]

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

Re: portability of ShellPkg

[Andrew Fish]

> On Sep 5, 2018, at 11:05 AM, Carsey, Jaben <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
>> 

Re: portability of ShellPkg

[Carsey, Jaben]

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

        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

Re: portability of ShellPkg

[Andrew Fish]

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
>>>> 
>> 

Re: portability of ShellPkg

[Carsey, Jaben]

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

Re: portability of ShellPkg

[Laszlo Ersek]

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

Re: portability of ShellPkg

[Andrew Fish]

> 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

Re: portability of ShellPkg

[Ni, Ruiyu]

On 9/6/2018 3:47 AM, Andrew Fish wrote:
> 
> 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
> 
> 

Andrew,
I agree refactoring UefiBootManagerLib to separate the pure UEFI, pure
PI and EDKII specific looks much more cleaner.
So far the PI related bits in UefiBootManagerLib may include:
1. Hob access for S4 support (memory type information HOB).
2. Boot from Firmware Volume support.

But that requires introducing two or more library classes so affecting
all existing platforms. EfiCreateEventLegacyBootEx() in UefiLib also 
touches PI event gEfiEventLegacyBootGuid.

And I think the value of refactor might be small.

I think root cause of this problem is not UefiBootManagerLib includes
PI, it's the assertion in DxeHobLib.
So I am thinking maybe a very light fix is to remove the constructor of
DxeHobLib.

I talked with Liming about this and he suggested that instead of 
removing the constructor, it's safer to just remove the assertion in the 
constructor. Because removing the constructor of HobLib may cause 
AutoGen process generates a different order of library constructors 
calling sequence, which may break the platform.

So I propose to just remove the assertion in DxeHobLib constructor.
Thoughts?



-- 
Thanks,
Ray

Re: portability of ShellPkg

[Laszlo Ersek]

On 09/06/18 04:34, Ni, Ruiyu wrote:
> On 9/6/2018 3:47 AM, Andrew Fish wrote:
>>
>> 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
>>
>>
> 
> Andrew,
> I agree refactoring UefiBootManagerLib to separate the pure UEFI, pure
> PI and EDKII specific looks much more cleaner.
> So far the PI related bits in UefiBootManagerLib may include:
> 1. Hob access for S4 support (memory type information HOB).
> 2. Boot from Firmware Volume support.
> 
> But that requires introducing two or more library classes so affecting
> all existing platforms. EfiCreateEventLegacyBootEx() in UefiLib also
> touches PI event gEfiEventLegacyBootGuid.
> 
> And I think the value of refactor might be small.
> 
> I think root cause of this problem is not UefiBootManagerLib includes
> PI, it's the assertion in DxeHobLib.
> So I am thinking maybe a very light fix is to remove the constructor of
> DxeHobLib.
> 
> I talked with Liming about this and he suggested that instead of
> removing the constructor, it's safer to just remove the assertion in the
> constructor. Because removing the constructor of HobLib may cause
> AutoGen process generates a different order of library constructors
> calling sequence, which may break the platform.
> 
> So I propose to just remove the assertion in DxeHobLib constructor.
> Thoughts?

I think keeping the constructor itself is important and a good idea.

I also think that we should *perhaps* keep the assertion *somewhere*,
just not in the constructor. Because, at least some of the HobLib APIs
cannot return errors (and their callers expect them to succeed at all
times). This suggests we should still trip assertions when a HobLib API
is called *in practice* on a non-PI UEFI platform.

Thanks
Laszlo

Re: portability of ShellPkg

[Kinney, Michael D]

Ray,

I have a few ideas here.

1) Add a new UefiHobLib instance that is only for 
   use by UEFI Drivers and UEFI Applications.  It fails
   gracefully if the HOB List is not in the UEFI System
   Configuration Table.  BootMode if HobList is not present
   can be BOOT_WITH_FULL_CONFIGURATION.

2) Remove ASSERT() from the CONSTRUCTOR and a return
   NULL from the Get*() functions if mHobList is NULL.
   BootMode can be BOOT_WITH_FULL_CONFIGURATION if
   mHobList is NULL.

3) Update UEFI Shell to not use the UefiBootManagerLib.
   This would add duplicate code to the UEFI Shell.

4) Update UefiBootManagerLib to not use the HobLib.
   Instead look for HobList in UEFI System Configuration
   Table and fail gracefully if it is not present.
   This would add some duplicate code to the 
   UefiBootManagerLib to parse the Hob List.

Best regards,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 6, 2018 2:57 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Andrew Fish
> <afish@apple.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-
> devel@lists.01.org; Carsey, Jaben
> <jaben.carsey@intel.com>; Alexander Graf
> <agraf@suse.de>; Heinrich Schuchardt
> <xypron.glpk@gmx.de>; AKASHI Takahiro
> <takahiro.akashi@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: portability of ShellPkg
> 
> On 09/06/18 04:34, Ni, Ruiyu wrote:
> > On 9/6/2018 3:47 AM, Andrew Fish wrote:
> >>
> >> 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
> >>
> >>
> >
> > Andrew,
> > I agree refactoring UefiBootManagerLib to separate the
> pure UEFI, pure
> > PI and EDKII specific looks much more cleaner.
> > So far the PI related bits in UefiBootManagerLib may
> include:
> > 1. Hob access for S4 support (memory type information
> HOB).
> > 2. Boot from Firmware Volume support.
> >
> > But that requires introducing two or more library
> classes so affecting
> > all existing platforms. EfiCreateEventLegacyBootEx()
> in UefiLib also
> > touches PI event gEfiEventLegacyBootGuid.
> >
> > And I think the value of refactor might be small.
> >
> > I think root cause of this problem is not
> UefiBootManagerLib includes
> > PI, it's the assertion in DxeHobLib.
> > So I am thinking maybe a very light fix is to remove
> the constructor of
> > DxeHobLib.
> >
> > I talked with Liming about this and he suggested that
> instead of
> > removing the constructor, it's safer to just remove
> the assertion in the
> > constructor. Because removing the constructor of
> HobLib may cause
> > AutoGen process generates a different order of library
> constructors
> > calling sequence, which may break the platform.
> >
> > So I propose to just remove the assertion in DxeHobLib
> constructor.
> > Thoughts?
> 
> I think keeping the constructor itself is important and
> a good idea.
> 
> I also think that we should *perhaps* keep the assertion
> *somewhere*,
> just not in the constructor. Because, at least some of
> the HobLib APIs
> cannot return errors (and their callers expect them to
> succeed at all
> times). This suggests we should still trip assertions
> when a HobLib API
> is called *in practice* on a non-PI UEFI platform.
> 
> Thanks
> Laszlo

Re: portability of ShellPkg

[Andrew Fish]

Mike,

More possible ideas

I think from a 10,000 foot level the issue is the Public definition of the HOB lib calls out the ASSERT behavior 

5) Make the ASSERT() conditional via a PCD and update the HOB lib definition. For platforms that make HOBs options GetHobList() must be called 1st, and non of the other APIs can be called on platforms that don't support HOBs. That has the upside of only needing to make the Lib constructor and GetHobList() ASSERTs conditional onPCDs. 

6) Split the existing UefiBootManagerLib in a backwards compatible way by making a new UefiBootLib. UefiBootManagerLib.h can include UefiBootLib.h and UefiBootManagerLib.inf could depend on UefiBootLib. This would only require adding UefiBootLib to the library class of existing DSC files. 

Thanks,

Andrew Fish

> On Sep 6, 2018, at 10:17 AM, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> 
> Ray,
> 
> I have a few ideas here.
> 
> 1) Add a new UefiHobLib instance that is only for 
>   use by UEFI Drivers and UEFI Applications.  It fails
>   gracefully if the HOB List is not in the UEFI System
>   Configuration Table.  BootMode if HobList is not present
>   can be BOOT_WITH_FULL_CONFIGURATION.
> 
> 2) Remove ASSERT() from the CONSTRUCTOR and a return
>   NULL from the Get*() functions if mHobList is NULL.
>   BootMode can be BOOT_WITH_FULL_CONFIGURATION if
>   mHobList is NULL.
> 
> 3) Update UEFI Shell to not use the UefiBootManagerLib.
>   This would add duplicate code to the UEFI Shell.
> 
> 4) Update UefiBootManagerLib to not use the HobLib.
>   Instead look for HobList in UEFI System Configuration
>   Table and fail gracefully if it is not present.
>   This would add some duplicate code to the 
>   UefiBootManagerLib to parse the Hob List.
> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, September 6, 2018 2:57 AM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Andrew Fish
>> <afish@apple.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-
>> devel@lists.01.org; Carsey, Jaben
>> <jaben.carsey@intel.com>; Alexander Graf
>> <agraf@suse.de>; Heinrich Schuchardt
>> <xypron.glpk@gmx.de>; AKASHI Takahiro
>> <takahiro.akashi@linaro.org>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Subject: Re: portability of ShellPkg
>> 
>> On 09/06/18 04:34, Ni, Ruiyu wrote:
>>> On 9/6/2018 3:47 AM, Andrew Fish wrote:
>>>> 
>>>> 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
>>>> 
>>>> 
>>> 
>>> Andrew,
>>> I agree refactoring UefiBootManagerLib to separate the
>> pure UEFI, pure
>>> PI and EDKII specific looks much more cleaner.
>>> So far the PI related bits in UefiBootManagerLib may
>> include:
>>> 1. Hob access for S4 support (memory type information
>> HOB).
>>> 2. Boot from Firmware Volume support.
>>> 
>>> But that requires introducing two or more library
>> classes so affecting
>>> all existing platforms. EfiCreateEventLegacyBootEx()
>> in UefiLib also
>>> touches PI event gEfiEventLegacyBootGuid.
>>> 
>>> And I think the value of refactor might be small.
>>> 
>>> I think root cause of this problem is not
>> UefiBootManagerLib includes
>>> PI, it's the assertion in DxeHobLib.
>>> So I am thinking maybe a very light fix is to remove
>> the constructor of
>>> DxeHobLib.
>>> 
>>> I talked with Liming about this and he suggested that
>> instead of
>>> removing the constructor, it's safer to just remove
>> the assertion in the
>>> constructor. Because removing the constructor of
>> HobLib may cause
>>> AutoGen process generates a different order of library
>> constructors
>>> calling sequence, which may break the platform.
>>> 
>>> So I propose to just remove the assertion in DxeHobLib
>> constructor.
>>> Thoughts?
>> 
>> I think keeping the constructor itself is important and
>> a good idea.
>> 
>> I also think that we should *perhaps* keep the assertion
>> *somewhere*,
>> just not in the constructor. Because, at least some of
>> the HobLib APIs
>> cannot return errors (and their callers expect them to
>> succeed at all
>> times). This suggests we should still trip assertions
>> when a HobLib API
>> is called *in practice* on a non-PI UEFI platform.
>> 
>> Thanks
>> Laszlo