Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use memory mapped FV protocol to avoid image copy

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 30 May 2023 at 10:41, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, May 30, 2023 3:52 PM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-
> > denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Leif Lindholm <quic_llindhol@quicinc.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>
> > Subject: Re: [edk2-devel] [RFC PATCH 05/11] MdeModulePkg/DxeCore: Use
> > memory mapped FV protocol to avoid image copy
> >
> > On Tue, 30 May 2023 at 08:21, Ni, Ray <ray.ni@intel.com> 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 FV
> > 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. Does
> > 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.
>
> OK.
>
> >
> > > 2. If the MMFV protocol is only produced by DxeCore and consumed by
> > DxeCore, 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" the
> > "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
> > protocol, 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.
>
> I see. How about:
> a. Define a GUID in DxeCore module internal
> b. Install that GUID in the FV handle (the accordingly instance of that GUID can be simply NULL)
>     as a signature to tell the FV is produced by DxeCore
> c. Implement a local function that return location inside the FV when the FvHandle has the
>     private GUID installed.
>

How is this any different from implementing a protocol? Installing a
GUID on the handle and associating it with some code is exactly what
this code does, but in the 'official' way.

I'm not sure what we will gain by doing the same thing using local
routines, other than making the code more difficult to follow. At
least the protocol has a clearly defined interface, whereas the
private GUID has no intrinsic meaning associated with it.