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.4749.1685432863453141933 for ; Tue, 30 May 2023 00:47:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=I/+tAPgZ; 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 D0A9862B74 for ; Tue, 30 May 2023 07:47:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 46902C433A1 for ; Tue, 30 May 2023 07:47:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685432862; bh=CcFgZ79FFqc+T/02YRd7ILwxZCPF6+G29F2IJB7vTnI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=I/+tAPgZyTkZ6Xd8d+X/X9V3sanBAEI0SKU262tlycZmi8UleTPJvVr0p3IwZVICp tq2PpS1S+9ZyzYZ9EFtGlNOszIHKfhd3VYVWe/hbNUvS+OLNqktcdMRua3Hib8Wc2C UOkh+7yYfHrsMEPEjUj8Vo7wJE/mEQO464w9fVon4ZEo74Aye3bIs7CoWdXtuYS9Up OAQ0MrgldhjIlTMtr4sletLSopxIxHsxdBqjY+6R5WIN1FShE2T5xhbAWqTWDRcVAa RL9MZmbnX+nPPZhqqpraCswmm5Mm/aLpRbpywb5AG+cOwMusRSqdcMWQClKr8UNqnD hJ8t9/HS3F1ZQ== Received: by mail-lj1-f182.google.com with SMTP id 38308e7fff4ca-2af2d092d7aso43176091fa.2 for ; Tue, 30 May 2023 00:47:42 -0700 (PDT) X-Gm-Message-State: AC+VfDz6dCnGoqrJJGbYoRJ431pzGOt+8XMI2NDwE+JM5XYsHyIRZBp6 uedLLKW5/RjEpmn1Km/j/YmldMkmii82+XrWPyg= X-Google-Smtp-Source: ACHHUZ58FpD4VGcVBws0A4tVA0rk4Z3Z9EwyuU7xCHLf9N0sQWQW/WnXl7c9E6PqK8ZfrW3Z2MtAX42Ia7l9nPflkdE= X-Received: by 2002:a2e:3201:0:b0:29b:80b4:7bf7 with SMTP id y1-20020a2e3201000000b0029b80b47bf7mr355101ljy.41.1685432860307; Tue, 30 May 2023 00:47:40 -0700 (PDT) MIME-Version: 1.0 References: <20230529101705.2476949-1-ardb@kernel.org> <20230529101705.2476949-5-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 30 May 2023 09:47:28 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 04/11] MdeModulePkg/DxeCore: Avoid caching memory mapped FFS files To: "Ni, Ray" Cc: "devel@edk2.groups.io" , "Gao, Liming" , "Kinney, Michael D" , "Yao, Jiewen" , Gerd Hoffmann , Taylor Beebe , Oliver Smith-Denny , "Bi, Dandan" , Leif Lindholm , Michael Kubacki Content-Type: text/plain; charset="UTF-8" On Tue, 30 May 2023 at 08:03, Ni, Ray 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 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 > > 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 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 > > --- > > 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 >