From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 222BEAC14D7 for ; Thu, 25 Jan 2024 13:14:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+eC9Qw26orS3TC+tM392IPZAgWWVjcn3TTHp3k0NBlU=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706188469; v=1; b=um9pNm0pTZ51WewW7xTwP0e2pV+R+W5jKjWvKt6DZ6pufa8GNFAJV+bvqiaUoz7qStqAWDfm p13P6fd2pWVmyASLp7lvWmFJ+FpNA07UmrYGOkMovM6ipIcqwoaSK6rvPQyrlQFIkBT+Hsei3og hLArZqaeEoax2gTqCigUWH+8= X-Received: by 127.0.0.2 with SMTP id XGjtYY7687511xu6OAS9i8vA; Thu, 25 Jan 2024 05:14:29 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.17161.1706188469151485659 for ; Thu, 25 Jan 2024 05:14:29 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-140-qUZxDY26Ox6LfEM7DIbxLA-1; Thu, 25 Jan 2024 08:14:26 -0500 X-MC-Unique: qUZxDY26Ox6LfEM7DIbxLA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6F67885A58C; Thu, 25 Jan 2024 13:14:26 +0000 (UTC) X-Received: from [10.39.195.100] (unknown [10.39.195.100]) by smtp.corp.redhat.com (Postfix) with ESMTPS id EFDE33C2E; Thu, 25 Jan 2024 13:14:24 +0000 (UTC) Message-ID: <53e77c3c-86ad-b3fb-9474-0a4a7cdcb80f@redhat.com> Date: Thu, 25 Jan 2024 14:14:23 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 02/11] OvmfPkg: add ShellLibs.dsc.inc To: devel@edk2.groups.io, kraxel@redhat.com Cc: Jiewen Yao , Ard Biesheuvel , Michael Roth , Erdem Aktas , Min Xu , Tom Lendacky , Oliver Steffen References: <20240124163802.2160303-1-kraxel@redhat.com> <20240124163802.2160303-3-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240124163802.2160303-3-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: XZ8L8iftzKSc1eEEUZdDHJ8Ex7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=um9pNm0p; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 > --- > 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) =3D=3D 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=3DUEFI_APPLICATION and (b) ENTRY_POINT=3DFunctionName 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=3DUEFI_APPLICATION, (b) ENTRY_POINT=3DShellCEntryLib, 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 =3D 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 ; for example "AppPkg/Applications/Main/Main.inf". Any module qualifies that has MODULE_TYPE=3DUEFI_APPLICATION, ENTRY_POINT=3DShellCEntryLib, 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) =3D=3D TRUE > - ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf > -!endif > - ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLib.= inf > - > S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScr= iptLib.inf > SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf > OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeL= ib/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) =3D=3D TRUE > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig= Dxe.inf > - OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf > + OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf { > + > + ShellCEntryLib|ShellPkg/Library/UefiShellCEntryLib/UefiShellCEntryLi= b.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 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-