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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114321): https://edk2.groups.io/g/devel/message/114321
Mute This Topic: https://groups.io/mt/103935344/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

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

Which is why I did it that way for TPM.  A single include file is enough
instead of having multiple include files for multiple module types.  The
OvmfPkg/Include/Dsc/OvmfTpmLibs.dsc.inc is placed in a location where
this works without problems, and I've placed the new shell include at
the same location.

I also have a WIP patch series (need to undust and rebase it ...) doing
the same for the crypto stuff, and that has the same problem the TPM
include has:  We have different configurations for PEI (stripped down
config, with only the hash functions for measurements) and DXE (full
TLS support).  And it is a single file with multiple sections too ...

tl;dr: I prefer the with-sections style.

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

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

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

Thanks for all this background info, much appreciated.

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

Given that EnrollDefaultKeys depends on the shell to launch
I'm wondering whenever we should just change that and make the
EnrollDefaultKeys build depend on BUILD_SHELL (and also move it
into Shell*.inc) ?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114613): https://edk2.groups.io/g/devel/message/114613
Mute This Topic: https://groups.io/mt/103935344/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-