Re: [edk2-devel] [PATCH 02/11] OvmfPkg: add ShellLibs.dsc.inc

["Laszlo Ersek" <lersek@redhat.com>]

I have several comments on this one:

On 1/24/24 17:37, Gerd Hoffmann wrote:
> Move EFI Shell libraries from OvmfPkgX64.dsc to
> the new ShellComponents.dsc.inc include file.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Include/Dsc/ShellLibs.dsc.inc | 11 +++++++++++
>  OvmfPkg/OvmfPkgX64.dsc                | 11 +++++------
>  2 files changed, 16 insertions(+), 6 deletions(-)
>  create mode 100644 OvmfPkg/Include/Dsc/ShellLibs.dsc.inc
>
> diff --git a/OvmfPkg/Include/Dsc/ShellLibs.dsc.inc b/OvmfPkg/Include/Dsc/ShellLibs.dsc.inc
> new file mode 100644
> index 000000000000..2e0bac74a261
> --- /dev/null
> +++ b/OvmfPkg/Include/Dsc/ShellLibs.dsc.inc
> @@ -0,0 +1,11 @@
> +##
> +#    SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +!if $(BUILD_SHELL) == TRUE
> +
> +[LibraryClasses]
> +  ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> +  ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> +
> +!endif

(1) First, in general, I don't agree with including section headers such
as [LibraryClasses] in include files. As far as I can remember, my
opinion has always been that the include files should not attempt to
specify context; the context spec (i.e., the section header, and the
placement of the !include directive) is up to the top-level DSC file
itself.

(

The idea being that, if an include file provides its own section header,
then the !include directive *changes* the context for the subsequent
parts of the including DSC file. That can be very annoying / surprising,
and I'd only really want to accept it *if* we established a hard rule
that *any* !include directive in any DSC or FDF etc file should
unconditionally be followed by an explicit section header -- even if
that section header were the same as the one just before the !include
directive. Like:

[LibraryClasses]
  FooLib|FooPkg/Library/BaseFooLib/FooLib.inf

!include OtherLibs.inc

[LibraryClasses]
  BarLib|BarPkg/Library/BaseBarLib/BarLib.inf

)

My preferred style is observed for example in:

- NetworkPkg/NetworkLibs.dsc.inc
- RedfishPkg/RedfishLibs.dsc.inc

Alas, it is not observed in:

- MdePkg/MdeLibs.dsc.inc
- OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc

At least, from the last two, OvmfTpmLibs.dsc.inc has an excuse: it
provides library class resolutions for different module types.

So here I'd kind of prefer if we *didn't* include the [LibraryClasses]
header. There's no reason to.


(2) This change makes the ShellCEntryLib resolution dependent on
BUILD_SHELL, which is a functional change, and it is not justified,
AFAICT.

(That's why you have to compensate for it in a module scope lib class
resolution, under "EnrollDefaultKeys.inf".)

There are three "levels" of applications in edk2:
- bare bones
- shell
- libc


(2.1) "Bare bones" is for example
"MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.inf".

Any module qualifies that has (a) MODULE_TYPE=UEFI_APPLICATION and (b)
ENTRY_POINT=FunctionName and (c) UefiApplicationEntryPoint listed under
[LibraryClasses].

These applications have entry point functions like

  EFI_STATUS
  EFIAPI
  FunctionName (
    IN EFI_HANDLE        ImageHandle,
    IN EFI_SYSTEM_TABLE  *SystemTable
    )

If they want to parse their "command line parameters", they have to open
EFI_LOADED_IMAGE_PROTOCOL on their ImageHandle, and parse
LoadOptionsSize and LoadOptions manually.


(2.2) shell app is for example
"OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf".

Any module qualifies that has (a) MODULE_TYPE=UEFI_APPLICATION, (b)
ENTRY_POINT=ShellCEntryLib, and (c) ShellCEntryLib listed under
[LibraryClasses].

These apps are launched like this:

- the shell loads the image with gBS->LoadImage()

- the shell installs an EFI_SHELL_PARAMETERS_PROTOCOL instance on the
resultant image handle. This protocol interface contains broken-out Argv
and Argc members, prepared by the shell, from the command line that
invokes the app.

- the shell launches the image with gBS->StartImage()

- the image is entered in the ShellCEntryLib() function (from the sole
"ShellCEntryLib" library instance), per ENTRY_POINT define:

  EFI_STATUS
  EFIAPI
  ShellCEntryLib (
    IN EFI_HANDLE        ImageHandle,
    IN EFI_SYSTEM_TABLE  *SystemTable
    )

- ShellCEntryLib() opens EFI_SHELL_PARAMETERS_PROTOCOL on the image
handle, and calls ShellAppMain():

  INTN
  EFIAPI
  ShellAppMain (
    IN UINTN   Argc,
    IN CHAR16  **Argv
    );

with

    ReturnFromMain = ShellAppMain (
                       EfiShellParametersProtocol->Argc,
                       EfiShellParametersProtocol->Argv
                       );

- That is where the actual application code starts running. The actual
application source includes "ShellPkg/Include/Library/ShellCEntryLib.h"
for declaring ShellAppMain().

If such an application is launched outside of the shell, then the
EFI_SHELL_PARAMETERS_PROTOCOL lookup on the image handle fails in
ShellCEntryLib(), and ShellAppMain() can not be called.

The point is that the "ShellPkg/Application/Shell/Shell.inf" binary
itself is of type (2.1), not type (2.2). The shell itself cannot depend
on EFI_SHELL_PARAMETERS_PROTOCOL.

Our BUILD_SHELL macro controls whether we build the shell itself, but
that is independent of whether we build applications that require the
shell to launch them. Making the ShellCEntryLib class resolution
dependent on BUILD_SHELL would only be valid if we also built all the
shell applications dependent on the BUILD_SHELL macro -- which is not
the case.


(2.3) "libc apps", for completeness: these live at
<https://github.com/tianocore/edk2-libc.git>; for example
"AppPkg/Applications/Main/Main.inf".

Any module qualifies that has MODULE_TYPE=UEFI_APPLICATION,
ENTRY_POINT=ShellCEntryLib, and lists LibC under [LibraryClasses].

The sole LibC instance (StdLib/LibC/Main/Main.c) provides a
ShellAppMain() function, for ShellCEntryLib() to call, that mangles Argc
and Argv so they can be passed to the actual application entry point
function main().


> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 043b0a7a67e0..eae025bd0163 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -258,16 +258,12 @@ [LibraryClasses]
>    TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
>  !endif
>
> -!if $(BUILD_SHELL) == TRUE
> -  ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> -!endif
> -  ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> -
>    S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>
>  !include OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc
> +!include OvmfPkg/Include/Dsc/ShellLibs.dsc.inc
>
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> @@ -960,7 +956,10 @@ [Components]
>
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> -  OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
> +  OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf {
> +    <LibraryClasses>
> +    ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.inf
> +  }
>  !endif
>
>    OvmfPkg/PlatformDxe/Platform.inf

(3) Side comment (because this part should go away anyway): the
"ShellCEntryLib|..." line should be indented by two more space
characters.

Summary:

- please consider dropping the [LibraryClasses] header from the include
file

- the ShellCEntryLib class should be resolved regardless of BUILD_SHELL
(and so that resolution may not even belong in the ShellLibs.dsc.inc
file?)

- EnrollDefaultKeys.inf needs no module-scope lib class resolution

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114402): https://edk2.groups.io/g/devel/message/114402
Mute This Topic: https://groups.io/mt/103935344/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-