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.web10.4756.1685433117359367750 for ; Tue, 30 May 2023 00:51:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=a1S9xI2o; 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 D73AB6236F for ; Tue, 30 May 2023 07:51:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 48ADBC433D2 for ; Tue, 30 May 2023 07:51:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685433116; bh=c1+Kg3ttg2Bwq7Ce0IsneAW7ESEDTU87APogJu83yVo=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=a1S9xI2oiu+3xMwS0de4U9zu2fwOdu9tJDATzB9KlvtJEJVvux6uRuXqPb4iPIg1W G2im04KqEVFGYfGjC9K2Rn6pTAggsycXgjoTV3jzlbfWPSlL10SmI52JoIjtpvqSp9 KopEIT3N7RRCgTIwiUFYsise9ihA4FEt6kfk6eC4EkTaAeJn4+qajMBGpkUT9GQ0aP 5eDvZip8G+wvRa03hdf3NJlSzVd1hXAic9hdzoq0BoRcRKU355Auy4+UZXbjJTzbnz qUxYTAQC6e9VNi/k86l51i4WmuD2e2OxQCSzi50pUWtT+RRDX7tRliJ2ykDsuL78xx OQuDe0wRntNEg== Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-4f4b2bc1565so4426633e87.2 for ; Tue, 30 May 2023 00:51:56 -0700 (PDT) X-Gm-Message-State: AC+VfDwrQSsNe6TMHiOV9/Y3jYqlAPGxRxv+OYO0LZ3IUbXygoIdd2j3 tiMKHfvCSxKAi//o+iYkb7bBpZaq8qwvnFNICIY= X-Google-Smtp-Source: ACHHUZ7ZjahdScosBHn5NiVOlaNlFflplnlnAraeIXfwFhj5VvyZwooga5527URjSg0An6vVnYXJCuJH8OlwuxRQJd8= X-Received: by 2002:a2e:7d03:0:b0:2ad:988e:7f8e with SMTP id y3-20020a2e7d03000000b002ad988e7f8emr387615ljc.51.1685433114353; Tue, 30 May 2023 00:51:54 -0700 (PDT) MIME-Version: 1.0 References: <20230529101705.2476949-1-ardb@kernel.org> <20230529101705.2476949-6-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 30 May 2023 09:51:42 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy To: devel@edk2.groups.io, ray.ni@intel.com Cc: "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" Content-Transfer-Encoding: quoted-printable On Tue, 30 May 2023 at 08:21, Ni, Ray wrote: > > GetFileBufferByFilePath() always returns a copy of file buffer even when = the file > is in a memory-mapped device. > So, your patch adds a new implementation (abstracted through the new MM F= V protocol) that can directly return the file location in the MMIO device. > > Several comments: > 1. I am not sure if any negative impact due to this change. For example: = old logic reads the MMIO device but doesn't execute in the MMIO device. Doe= s MMIO device always support execution in place? At this point, we are not executing anything in place. The buffer is only used by the PE/COFF loader to access the file contents, but it still creates the sections in memory as before, and copies the data into them. This is similar to how gBS->Loadimage() with a buffer/size only uses the buffer contents to access the file data, it does not execute the image from the buffer. > 2. If the MMFV protocol is only produced by DxeCore and consumed by DxeCo= re, can we just implement a local function instead? The challenge might be = how to pass the FV_DEVICE instance to the local function. Can we "handle" t= he "FirmwareVolume2" protocol from the Handle first and use CR_FROM_THIS() = macro to retrieve the FV_DEVICE pointer? This helps to not add the MMFV pro= tocol, letting the change a pure DxeCore internal thing. > The loader does not know whether the FirmwareVolume2 protocol was produced by DXE core or by some other component, so we cannot assume that CR_FROM_THIS() is usable. > > -----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, Dand= an > > ; Gao, Liming ; Kinney, > > Michael D ; Leif Lindholm > > ; Michael Kubacki > > Subject: [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV > > protocol to avoid image copy > > > > Use the memory mapped FV protocol to obtain the existing location in > > memory and the size of an image being loaded from a firmware volume. > > This removes the need to do a memcopy of the file data. > > > > Signed-off-by: Ard Biesheuvel > > --- > > MdeModulePkg/Core/Dxe/DxeMain.h | 1 + > > MdeModulePkg/Core/Dxe/DxeMain.inf | 3 + > > MdeModulePkg/Core/Dxe/Image/Image.c | 111 +++++++++++++++++= --- > > MdeModulePkg/Include/Protocol/MemoryMappedFv.h | 59 +++++++++++ > > MdeModulePkg/MdeModulePkg.dec | 3 + > > 5 files changed, 163 insertions(+), 14 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h > > b/MdeModulePkg/Core/Dxe/DxeMain.h > > index 43daa037be441150..a695b457c79b65bb 100644 > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > > @@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf > > b/MdeModulePkg/Core/Dxe/DxeMain.inf > > index 35d5bf0dee6f7f3f..a7175cb364b9b5de 100644 > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf > > @@ -153,6 +153,9 @@ [Protocols] > > gEfiLoadedImageDevicePathProtocolGuid ## PRODUCES > > > > gEfiHiiPackageListProtocolGuid ## SOMETIMES_PRODUCES > > > > gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES > > > > + ## PRODUCES > > > > + ## CONSUMES > > > > + gEdkiiMemoryMappedFvProtocolGuid > > > > gEdkiiPeCoffImageEmulatorProtocolGuid ## SOMETIMES_CONSUMES > > > > > > > > # Arch Protocols > > > > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c > > b/MdeModulePkg/Core/Dxe/Image/Image.c > > index f30e369370a09609..3dfab4829b3ca17f 100644 > > --- a/MdeModulePkg/Core/Dxe/Image/Image.c > > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c > > @@ -1043,6 +1043,76 @@ CoreUnloadAndCloseImage ( > > CoreFreePool (Image); > > > > } > > > > > > > > +/** > > > > + Get the image file data and size directly from a memory mapped FV > > > > + > > > > + If FilePath is NULL, then NULL is returned. > > > > + If FileSize is NULL, then NULL is returned. > > > > + If AuthenticationStatus is NULL, then NULL is returned. > > > > + > > > > + @param[in] FvHandle The firmware volume handle > > > > + @param[in] FilePath The pointer to the device path= of the file > > > > + that is abstracted to the file= buffer. > > > > + @param[out] FileSize The pointer to the size of the= abstracted > > > > + file buffer. > > > > + @param[out] AuthenticationStatus Pointer to the authentication = status. > > > > + > > > > + @retval NULL FilePath is NULL, or FileSize is NULL, or Authenticat= ionStatus > > > > + is NULL, or the file is not memory mapped > > > > + @retval other The abstracted file buffer. > > > > +**/ > > > > +STATIC > > > > +VOID * > > > > +GetFileFromMemoryMappedFv ( > > > > + IN EFI_HANDLE FvHandle, > > > > + IN CONST EFI_DEVICE_PATH_PROTOCOL *FilePath, > > > > + OUT UINTN *FileSize, > > > > + OUT UINT32 *AuthenticationStatus > > > > + ) > > > > +{ > > > > + EDKII_MEMORY_MAPPED_FV_PROTOCOL *MemMappedFv; > > > > + CONST EFI_GUID *NameGuid; > > > > + EFI_PHYSICAL_ADDRESS Address; > > > > + EFI_STATUS Status; > > > > + > > > > + if ((FilePath =3D=3D NULL) || > > > > + (FileSize =3D=3D NULL) || > > > > + (AuthenticationStatus =3D=3D NULL)) > > > > + { > > > > + return NULL; > > > > + } > > > > + > > > > + NameGuid =3D EfiGetNameGuidFromFwVolDevicePathNode ( > > > > + (CONST MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)FilePath); > > > > + if (NameGuid =3D=3D NULL) { > > > > + return NULL; > > > > + } > > > > + > > > > + Status =3D gBS->HandleProtocol ( > > > > + FvHandle, > > > > + &gEdkiiMemoryMappedFvProtocolGuid, > > > > + (VOID **)&MemMappedFv > > > > + ); > > > > + if (EFI_ERROR (Status)) { > > > > + ASSERT (Status =3D=3D EFI_UNSUPPORTED); > > > > + return NULL; > > > > + } > > > > + > > > > + Status =3D MemMappedFv->GetLocationAndSize ( > > > > + MemMappedFv, > > > > + NameGuid, > > > > + EFI_SECTION_PE32, > > > > + &Address, > > > > + FileSize, > > > > + AuthenticationStatus > > > > + ); > > > > + if (EFI_ERROR (Status) || (Address > (MAX_ADDRESS - *FileSize))) { > > > > + return NULL; > > > > + } > > > > + > > > > + return (VOID *)(UINTN)Address; > > > > +} > > > > + > > > > /** > > > > Loads an EFI image into memory and returns a handle to the image. > > > > > > > > @@ -1164,6 +1234,16 @@ CoreLoadImageCommon ( > > Status =3D CoreLocateDevicePath (&gEfiFirmwareVolume2ProtocolGuid, > > &HandleFilePath, &DeviceHandle); > > > > if (!EFI_ERROR (Status)) { > > > > ImageIsFromFv =3D TRUE; > > > > + > > > > + // > > > > + // If possible, use the memory mapped file image directly, rathe= r than > > copying it into a buffer > > > > + // > > > > + FHand.Source =3D GetFileFromMemoryMappedFv ( > > > > + DeviceHandle, > > > > + HandleFilePath, > > > > + &FHand.SourceSize, > > > > + &AuthenticationStatus > > > > + ); > > > > } else { > > > > HandleFilePath =3D FilePath; > > > > Status =3D CoreLocateDevicePath (&gEfiSimpleFileSystemPr= otocolGuid, > > &HandleFilePath, &DeviceHandle); > > > > @@ -1187,21 +1267,24 @@ CoreLoadImageCommon ( > > // > > > > // Get the source file buffer by its device path. > > > > // > > > > - FHand.Source =3D GetFileBufferByFilePath ( > > > > - BootPolicy, > > > > - FilePath, > > > > - &FHand.SourceSize, > > > > - &AuthenticationStatus > > > > - ); > > > > if (FHand.Source =3D=3D NULL) { > > > > - Status =3D EFI_NOT_FOUND; > > > > - } else { > > > > - FHand.FreeBuffer =3D TRUE; > > > > - if (ImageIsFromLoadFile) { > > > > - // > > > > - // LoadFile () may cause the device path of the Handle be upda= ted. > > > > - // > > > > - OriginalFilePath =3D AppendDevicePath (DevicePathFromHandle > > (DeviceHandle), Node); > > > > + FHand.Source =3D GetFileBufferByFilePath ( > > > > + BootPolicy, > > > > + FilePath, > > > > + &FHand.SourceSize, > > > > + &AuthenticationStatus > > > > + ); > > > > + > > > > + if (FHand.Source =3D=3D NULL) { > > > > + Status =3D EFI_NOT_FOUND; > > > > + } else { > > > > + FHand.FreeBuffer =3D TRUE; > > > > + if (ImageIsFromLoadFile) { > > > > + // > > > > + // LoadFile () may cause the device path of the Handle be up= dated. > > > > + // > > > > + OriginalFilePath =3D AppendDevicePath (DevicePathFromHandle > > (DeviceHandle), Node); > > > > + } > > > > } > > > > } > > > > } > > > > diff --git a/MdeModulePkg/Include/Protocol/MemoryMappedFv.h > > b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h > > new file mode 100644 > > index 0000000000000000..821009122113a658 > > --- /dev/null > > +++ b/MdeModulePkg/Include/Protocol/MemoryMappedFv.h > > @@ -0,0 +1,59 @@ > > +/** @file > > > > + Protocol to obtain information about files in memory mapped firmware > > volumes > > > > + > > > > + Copyright (c) 2023, Google LLC. All rights reserved.
> > > > + > > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > > + > > > > +**/ > > > > + > > > > +#ifndef EDKII_MEMORY_MAPPED_FV_H_ > > > > +#define EDKII_MEMORY_MAPPED_FV_H_ > > > > + > > > > +#define EDKII_MEMORY_MAPPED_FV_PROTOCOL_GUID \ > > > > + { 0xb9bfa973, 0x5384, 0x441e, { 0xa4, 0xe7, 0x20, 0xe6, 0x5d, 0xaf, = 0x2e, > > 0x0f } } > > > > + > > > > +typedef struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL > > EDKII_MEMORY_MAPPED_FV_PROTOCOL; > > > > + > > > > +// > > > > +// Function Prototypes > > > > +// > > > > + > > > > +/** > > > > + Get the physical address and size of a file's section in a memory ma= pped FV > > > > + > > > > + @param[in] This The protocol pointer > > > > + @param[in] NameGuid The name GUID of the file > > > > + @param[in] SectionType The file section from which to retrieve ad= dress and > > size > > > > + @param[out] FileAddress The physical address of the file > > > > + @param[out] FileSize The size of the file > > > > + @param[out] AuthStatus The authentication status associated with = the file > > > > + > > > > + @retval EFI_SUCCESS Information about the file was retrie= ved > > successfully. > > > > + @retval EFI_INVALID_PARAMETER FileAddress was NULL, FileSize was NU= LL, > > AuthStatus > > > > + was NULL. > > > > + @retval EFI_NOT_FOUND No section of the specified type coul= d be > > located in > > > > + the specified file. > > > > + > > > > +**/ > > > > +typedef > > > > +EFI_STATUS > > > > +(EFIAPI *GET_LOCATION_AND_SIZE)( > > > > + IN EDKII_MEMORY_MAPPED_FV_PROTOCOL *This, > > > > + IN CONST EFI_GUID *NameGuid, > > > > + IN EFI_SECTION_TYPE SectionType, > > > > + OUT EFI_PHYSICAL_ADDRESS *FileAddress, > > > > + OUT UINTN *FileSize, > > > > + OUT UINT32 *AuthStatus > > > > + ); > > > > + > > > > +// > > > > +// Protocol interface structure > > > > +// > > > > +struct _EDKII_MEMORY_MAPPED_FV_PROTOCOL { > > > > + GET_LOCATION_AND_SIZE GetLocationAndSize; > > > > +}; > > > > + > > > > +extern EFI_GUID gEdkiiMemoryMappedFvProtocolGuid; > > > > + > > > > +#endif > > > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > b/MdeModulePkg/MdeModulePkg.dec > > index d65dae18aa81e569..2d72ac733d82195e 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -679,6 +679,9 @@ [Protocols] > > ## Include/Protocol/PlatformBootManager.h > > > > gEdkiiPlatformBootManagerProtocolGuid =3D { 0xaa17add4, 0x756c, 0x46= 0d, > > { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 0x59 } } > > > > > > > > + ## Include/Protocol/MemoryMappedFv.h > > > > + gEdkiiMemoryMappedFvProtocolGuid =3D { 0xb9bfa973, 0x5384, 0x441e, {= 0xa4, > > 0xe7, 0x20, 0xe6, 0x5d, 0xaf, 0x2e, 0x0f } } > > > > + > > > > # > > > > # [Error.gEfiMdeModulePkgTokenSpaceGuid] > > > > # 0x80000001 | Invalid value provided. > > > > -- > > 2.39.2 > > > >=20 > >