Re: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files

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

On Tue, 30 May 2023 at 08:03, Ni, Ray <ray.ni@intel.com> wrote:
>
> I don't know why the FFS file is cached. Without knowing the reason of caching, I cannot say it's good to remove the caching logic.
> I will let @Gao, Liming, @Kinney, Michael D to comment.
>

Fair enought.

I found this commit by Start

commit eb1cace292ff0c66ca11eff4703c9fa16219c2a1
Author: Star Zeng <star.zeng@intel.com>
Date:   Wed Aug 27 08:31:44 2014 +0000

    MdeModulePkg DxeCore: Don't cache memory mapped IO FV.

which changes the caching logic to cache FFS files individually
instead of caching the entire firmware volume if it is memory mapped.

AIUI, firmware volumes could be mapped uncached, so if caching is
still needed in some cases, perhaps we might try to disambiguate these
cases based on the underlying memory region? I.e., if it is system
memory, no caching is used.

The decompressed FV does not need caching on any architecture or
platform, afaict.


>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Monday, May 29, 2023 6:17 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; 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: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory
> > mapped FFS files
> >
> > If a firmware volume is memory mapped, it means we can access it
> > contents directly, and so caching serves little purpose beyond
> > potentially a minor performance improvement. However, given that most
> > files are read only a single time, and dispatched from a decompressed
> > firmware volume in DRAM, we can just avoid the redundant caching here.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c | 22 --------------------
> >  1 file changed, 22 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > index 2ff22c93aad48d7e..69df96685d680868 100644
> > --- a/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > +++ b/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c
> > @@ -284,7 +284,6 @@ FvReadFile (
> >    UINT8                   *SrcPtr;
> >
> >    EFI_FFS_FILE_HEADER     *FfsHeader;
> >
> >    UINTN                   InputBufferSize;
> >
> > -  UINTN                   WholeFileSize;
> >
> >
> >
> >    if (NameGuid == NULL) {
> >
> >      return EFI_INVALID_PARAMETER;
> >
> > @@ -316,27 +315,6 @@ FvReadFile (
> >    // Get a pointer to the header
> >
> >    //
> >
> >    FfsHeader = FvDevice->LastKey->FfsHeader;
> >
> > -  if (FvDevice->IsMemoryMapped) {
> >
> > -    //
> >
> > -    // Memory mapped FV has not been cached, so here is to cache by file.
> >
> > -    //
> >
> > -    if (!FvDevice->LastKey->FileCached) {
> >
> > -      //
> >
> > -      // Cache FFS file to memory buffer.
> >
> > -      //
> >
> > -      WholeFileSize = IS_FFS_FILE2 (FfsHeader) ? FFS_FILE2_SIZE (FfsHeader) :
> > FFS_FILE_SIZE (FfsHeader);
> >
> > -      FfsHeader     = AllocateCopyPool (WholeFileSize, FfsHeader);
> >
> > -      if (FfsHeader == NULL) {
> >
> > -        return EFI_OUT_OF_RESOURCES;
> >
> > -      }
> >
> > -
> >
> > -      //
> >
> > -      // Let FfsHeader in FfsFileEntry point to the cached file buffer.
> >
> > -      //
> >
> > -      FvDevice->LastKey->FfsHeader  = FfsHeader;
> >
> > -      FvDevice->LastKey->FileCached = TRUE;
> >
> > -    }
> >
> > -  }
> >
> >
> >
> >    //
> >
> >    // Remember callers buffer size
> >
> > --
> > 2.39.2
>