On Wed, 28 Nov 2018 at 23:54, Laszlo Ersek <[email protected]> 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 for > 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 flash > 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 been 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 whenever > we install the BlockIO protocol, its implementation is sub-optimal as it > reads > and writes entire blocks using the BlockIO protocol. In fact we can access > NOR flash with a finer granularity than that, so we can improve performance > by directly producing the DiskIO protocol. > */ > > Umm... this flash driver does a lot more than I thought it did... or should. > :) > > > 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, because > (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 > decompressed 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 <[email protected]> > > --- > > 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/ArmVirtPkg/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/ArmVirtPkg/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 = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2])); > > Reg += 4; > > > > + PropSize -= 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) >= Base) && > > + (Base + Size) >= PcdGet64 (PcdFvBaseAddress)) { > > + continue; > > + } > > + > > The overlap condition is expressed correctly, in general, I think; however, > 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 > exclusive equals inclusive, there is no overlap; they are directly adjacent > only. I'd drop the equal signs. > > > > mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base; > > mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base; > > mNorFlashDevices[Num].Size = (UINTN)Size; > > mNorFlashDevices[Num].BlockSize = QEMU_NOR_BLOCK_SIZE; > > Num++; > > - > > - PropSize -= 4 * sizeof (UINT32); > > } > > } > > > > > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
