From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.4626.1685432177417245619 for ; Tue, 30 May 2023 00:36:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=FQSDMYRY; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E2F5D622FC for ; Tue, 30 May 2023 07:36:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C8DF9C433A1 for ; Tue, 30 May 2023 07:36:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685432175; bh=SElwsTrjzN4gA0vNbqLzE8NGhb2adaOJhiHOFN+uxAk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=FQSDMYRYg3r8i0iuIbL1f6lBInLGmcR9X2hk2DWu8VJx2w9sE+GGmqRIS5DPZbBnM +sqbr3wR46dX8Nj4XyPbg7GWxhsv91YFNEQOnLxZHwm+st13+Ja1mX3Wm0N48DSylN RvrKWDtD2cOQg5dXm1NRrk5oz6I7vmJPYN+jCornCvRLFLO8ZemBCWjM41aWm52GST xvKXXODAvx61kYXyroI55Nu9hH7jQF5LMzNvpKS49ORNcrSmBHtBM2Lu+Y1rfmYLEO bMk+DCzZKWdmXws83fnXiDXoM7PoeYRcLJrClR4yBXjMX1H0EO0xHpMEt1MWL94D9I o3JCuSBi0pIQw== Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-4f3b9e54338so4678547e87.0 for ; Tue, 30 May 2023 00:36:15 -0700 (PDT) X-Gm-Message-State: AC+VfDzFVjFU+LINUZ8nGmb08yAcoF9oiibfVuMsGRg0iausPxPjmkrN V5E1/9xmWAHqqSr5JLJWH+5uhfpNTU/xybPGeb0= X-Google-Smtp-Source: ACHHUZ6WfW76IOKtdeTFuPj//V/uEueXY+ZXhXirXvUs3mR4nGDVAXwfa6nSCfz6kUDzF4vd/wpkI+WI+rbEnVGLaKk= X-Received: by 2002:a2e:9a8e:0:b0:2b0:5f62:8b8 with SMTP id p14-20020a2e9a8e000000b002b05f6208b8mr342326lji.42.1685432173809; Tue, 30 May 2023 00:36:13 -0700 (PDT) MIME-Version: 1.0 References: <20230529101705.2476949-1-ardb@kernel.org> <20230529101705.2476949-2-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 30 May 2023 09:36:02 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused 'EntryPoint' argument to LoadImage To: "Ni, Ray" Cc: "devel@edk2.groups.io" , "Yao, Jiewen" , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , "Bi, Dandan" , "Gao, Liming" , "Kinney, Michael D" , Leif Lindholm , Michael Kubacki Content-Type: text/plain; charset="UTF-8" On Tue, 30 May 2023 at 07:54, Ni, Ray wrote: > > I really like the code simplification! Just 2 comments below: > > 1. Can you describe the calling flow explicitly in commit message: > CoreLoadImage (public api) -> CoreLoadImageCommon -> CoreLoadPeImage > Ok > 2. You added "static" to CoreLoadImageCommon but didn't add to CoreLoadPeImage. > I think you are trying to guarantee no lib implementation that silently calls to the > internal functions like CoreLoadImageCommon. > But, why? > Without adding "static", the linking still fails if there is such lib implementation because > of the function prototype change. An error such as "parameter mismatch" would appear. This function is not declared in a header, so any existing user would carry its own EXTERN declaration. So the compiler would not notice, and the linker does not verify protoype compatibility, so we would only notice this at runtime. I am not aware of any downstream platforms that do such a thing, so perhaps I am being overly cautious. But generally, functions should be STATIC unless there is a need for them to have external linkage, and changing the prototype is a reasonable justification IMHO for adding it at this point. > And if there is a reason for adding "static", why not add that to CoreLoadPeImage as well? > This is added in the next patch. > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Monday, May 29, 2023 6:17 PM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Ni, Ray ; Yao, Jiewen > > ; Gerd Hoffmann ; Taylor Beebe > > ; Oliver Smith-Denny ; Bi, Dandan > > ; Gao, Liming ; Kinney, > > Michael D ; Leif Lindholm > > ; Michael Kubacki > > Subject: [RFC PATCH 01/11] MdeModulePkg/DxeCore: Remove unused > > 'EntryPoint' argument to LoadImage > > > > CoreLoadImageCommon's only user passes NULL for its EntryPoint argument, > > so it has no purpose and can simply be dropped. While at it, make > > CoreLoadImageCommon STATIC to prevent it from being accessed from other > > translation units. > > > > Signed-off-by: Ard Biesheuvel > > --- > > MdeModulePkg/Core/Dxe/Image/Image.c | 17 +++-------------- > > 1 file changed, 3 insertions(+), 14 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c > > b/MdeModulePkg/Core/Dxe/Image/Image.c > > index 9dbfb2a1fad22ced..2f2dfe5d0496dc4f 100644 > > --- a/MdeModulePkg/Core/Dxe/Image/Image.c > > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c > > @@ -560,7 +560,6 @@ CoreIsImageTypeSupported ( > > @param Pe32Handle The handle of PE32 image > > > > @param Image PE image to be loaded > > > > @param DstBuffer The buffer to store the image > > > > - @param EntryPoint A pointer to the entry point > > > > @param Attribute The bit mask of attributes to set for the load > > > > PE image > > > > > > > > @@ -577,7 +576,6 @@ CoreLoadPeImage ( > > IN VOID *Pe32Handle, > > > > IN LOADED_IMAGE_PRIVATE_DATA *Image, > > > > IN EFI_PHYSICAL_ADDRESS DstBuffer OPTIONAL, > > > > - OUT EFI_PHYSICAL_ADDRESS *EntryPoint OPTIONAL, > > > > IN UINT32 Attribute > > > > ) > > > > { > > > > @@ -810,13 +808,6 @@ CoreLoadPeImage ( > > } > > > > } > > > > > > > > - // > > > > - // Fill in the entry point of the image if it is available > > > > - // > > > > - if (EntryPoint != NULL) { > > > > - *EntryPoint = Image->ImageContext.EntryPoint; > > > > - } > > > > - > > > > // > > > > // Print the load address and the PDB file name if it is available > > > > // > > > > @@ -1111,7 +1102,6 @@ CoreUnloadAndCloseImage ( > > this parameter contains the required number. > > > > @param ImageHandle Pointer to the returned image handle that is > > > > created when the image is successfully loaded. > > > > - @param EntryPoint A pointer to the entry point > > > > @param Attribute The bit mask of attributes to set for the load > > > > PE image > > > > > > > > @@ -1134,6 +1124,7 @@ CoreUnloadAndCloseImage ( > > platform policy specifies that the image should not be started. > > > > > > > > **/ > > > > +STATIC > > > > EFI_STATUS > > > > CoreLoadImageCommon ( > > > > IN BOOLEAN BootPolicy, > > > > @@ -1144,7 +1135,6 @@ CoreLoadImageCommon ( > > IN EFI_PHYSICAL_ADDRESS DstBuffer OPTIONAL, > > > > IN OUT UINTN *NumberOfPages OPTIONAL, > > > > OUT EFI_HANDLE *ImageHandle, > > > > - OUT EFI_PHYSICAL_ADDRESS *EntryPoint OPTIONAL, > > > > IN UINT32 Attribute > > > > ) > > > > { > > > > @@ -1375,9 +1365,9 @@ CoreLoadImageCommon ( > > } > > > > > > > > // > > > > - // Load the image. If EntryPoint is Null, it will not be set. > > > > + // Load the image. > > > > // > > > > - Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, EntryPoint, > > Attribute); > > > > + Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer, Attribute); > > > > if (EFI_ERROR (Status)) { > > > > if ((Status == EFI_BUFFER_TOO_SMALL) || (Status == > > EFI_OUT_OF_RESOURCES)) { > > > > if (NumberOfPages != NULL) { > > > > @@ -1559,7 +1549,6 @@ CoreLoadImage ( > > (EFI_PHYSICAL_ADDRESS)(UINTN)NULL, > > > > NULL, > > > > ImageHandle, > > > > - NULL, > > > > EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION | > > EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRATION > > > > ); > > > > > > > > -- > > 2.39.2 >