From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d44; helo=mail-io1-xd44.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0E40021962301 for ; Wed, 28 Nov 2018 15:07:03 -0800 (PST) Received: by mail-io1-xd44.google.com with SMTP id x6so21440509ioa.9 for ; Wed, 28 Nov 2018 15:07:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=HbYbO/8yjF2UYknbRv5gmQCzf1SMPx3Av/hDkZnb6QA=; b=F9CEPU765Ld/oBZ6TgEWUaqgpjHYImNuYDIaotjBxDv7kpNyIDp2Z+1uMReGlNlO2f XGxMM+A2F1kL/QQnLeecItLGqqKTAVxsnJRccLgvy1MIeRV5TaoKQtEDiZhJknr8to26 1p/GJlTqrsoTpKt5YrHN1KWlbfXTFagb/LlKA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=HbYbO/8yjF2UYknbRv5gmQCzf1SMPx3Av/hDkZnb6QA=; b=YjbSx4xHjcEA+5uoR3WOX701z3kRWWSNW9zkmWlSxZb1Gb+k/aYNgT1Me+CBKe8XMv zk8EjHMiUtAF1PaQS0+2JYNOPN9lNz063s2wwCBEeybiesTRXprAbsfP5zjU7FpNwlAn Xva8EUVUdI09iVVPJnO7pTp+jfjwnlOpSJhM1lY7uDhqeNg76Trw+JPcLsmFl1BsMYaA IgXLsvs1HYMSs8PwlYcKP4keNV+37nhYUVXdsfcMDzIDX/1LYeFogvc6AzEOUciUfEZH Och8avJHKIt1STvjmbIocwlJeqDOJ0u9+vdiA9CGeWaQLqfOczAvSOQGzCZzGW9wMDNX A01w== X-Gm-Message-State: AA+aEWb5Tfm5/Umah2qepetCkb0fus9aGbN48oOB3vjQwDSNwD3/zqOj DRiIXQgT/Q43v4nTgRFapHeu5ubEHvEw2xFyJIbCxg== X-Google-Smtp-Source: AFSGD/XkB259raoQI+FgrJKeC3wM2qOYW+cEjZW1eaYHUduAgvlCWCMjxoSaC8dowUfBKXkHtFD6yfqqQpssAqtKZ00= X-Received: by 2002:a6b:5d01:: with SMTP id r1mr28888589iob.170.1543446423162; Wed, 28 Nov 2018 15:07:03 -0800 (PST) MIME-Version: 1.0 References: <20181128191646.31526-1-ard.biesheuvel@linaro.org> <20181128191646.31526-2-ard.biesheuvel@linaro.org> <7c047366-2b63-08fc-079e-98705c7efa6b@redhat.com> In-Reply-To: <7c047366-2b63-08fc-079e-98705c7efa6b@redhat.com> From: Ard Biesheuvel Date: Thu, 29 Nov 2018 00:06:51 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Auger Eric , Andrew Jones , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Subject: Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Nov 2018 23:07:04 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 28 Nov 2018 at 23:54, Laszlo Ersek wrote: > > On 11/28/18 20:16, Ard Biesheuvel wrote: > > The primary FV contains the firmware boot image, which is not > > runtime updatable in our case. So exposing it to the NOR flash > > driver is undesirable, since it may attempt to modify the NOR > > flash contents. > > With you so far. > > > It is also rather pointless, since we don't > > keep anything there that we don't already expose via the FVB > > protocol instances that DXE core creates for us based on the > > FV HOBs > > I don't follow -- the DXE core does rely on the FV HOBs that we create fo= r it, but I don't remember the DXE core creating FVB protocol instances. An= FVB ("firmware volume block") protocol instance is usually created by a fl= ash driver. What am I missing? > > Do you mean handles with MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile= (...) device paths on them? That point into firmware volumes (that have bee= n supposedly decompressed from flash to RAM)? > The DXE core dispatcher calls CoreProcessFvImageFile() for all FV images it encounters, but looking closer, that only happens for embedded FV images, not the primary one. But that still means the primary FV contains nothing we actually need. > > (and so there is nothing the partition or file system > > drivers could potentially attach to via the block I/O and disk > > I/O protocol instances that the NOR flash driver creates) > > Ugh, NorFlashDxe creates BlockIo and DiskIo interfaces itself??? > > Let's see... > > /* > Although DiskIoDxe will automatically install the DiskIO protocol whene= ver > we install the BlockIO protocol, its implementation is sub-optimal as i= t reads > and writes entire blocks using the BlockIO protocol. In fact we can acc= ess > NOR flash with a finer granularity than that, so we can improve perform= ance > by directly producing the DiskIO protocol. > */ > > Umm... this flash driver does a lot more than I thought it did... or shou= ld. :) > > > Anyway I think it should suffice to say in the commit message that we don= 't want to expose the first flash device as an FVB protocol instance, becau= se (a) it's read-only, and (b) in the DXE phase, we don't use anything from= that flash device. It contains: > - the reset vector, > - the SEC module, > - (for ArmVirtQemu) the non-compressed PEI core, and PEIMs, > - and a compressed bunch of DXE modules (incl. the DXE core) which are de= compressed to RAM anyway. > > > So let's disregard the NOR flash block that covers the primary > > FV. > > OK. > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf | 5 +++++ > > ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 13 ++++++++++= +-- > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/A= rmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf > > index d86ff36dbd58..c5752a243e6b 100644 > > --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf > > +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf > > @@ -28,6 +28,7 @@ [Sources.common] > > [Packages] > > MdePkg/MdePkg.dec > > ArmPlatformPkg/ArmPlatformPkg.dec > > + ArmPkg/ArmPkg.dec > > ArmVirtPkg/ArmVirtPkg.dec > > > > [LibraryClasses] > > @@ -40,3 +41,7 @@ [Protocols] > > > > [Depex] > > gFdtClientProtocolGuid > > + > > +[Pcd] > > + gArmTokenSpaceGuid.PcdFvBaseAddress > > + gArmTokenSpaceGuid.PcdFvSize > > diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/Arm= VirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c > > index 2678f57eaaad..72b47bdb5a78 100644 > > --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c > > +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c > > @@ -75,13 +75,22 @@ NorFlashPlatformGetDevices ( > > Size =3D SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2])); > > Reg +=3D 4; > > > > + PropSize -=3D 4 * sizeof (UINT32); > > + > > + // > > + // Disregard any flash devices that overlap with the primary FV. > > + // The firmware is not updatable from inside the guest anyway. > > + // > > + if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) >=3D Bas= e) && > > + (Base + Size) >=3D PcdGet64 (PcdFvBaseAddress)) { > > + continue; > > + } > > + > > The overlap condition is expressed correctly, in general, I think; howeve= r, both subconditions are off-by-one each. In each, we compare an exclusive= limit (one's end) with an inclusive limit (the other's base). And, when ex= clusive equals inclusive, there is no overlap; they are directly adjacent o= nly. I'd drop the equal signs. > > > > mNorFlashDevices[Num].DeviceBaseAddress =3D (UINTN)Base; > > mNorFlashDevices[Num].RegionBaseAddress =3D (UINTN)Base; > > mNorFlashDevices[Num].Size =3D (UINTN)Size; > > mNorFlashDevices[Num].BlockSize =3D QEMU_NOR_BLOCK_SIZE; > > Num++; > > - > > - PropSize -=3D 4 * sizeof (UINT32); > > } > > } > > > > > > Thanks > Laszlo