On 27 August 2014 18:03, Laszlo Ersek <[email protected]> wrote: > some comments > > On 08/26/14 15:03, Ard Biesheuvel wrote: >> This adds support for retaining UEFI environment variables in the second >> emulated NOR flash which resides at phys address 0x04000000 (64 MB). >> >> Note that this requires booting QEMU with two -pflash arguments, each of >> which >> points to a NOR image file of exactly 64 MB in size. The second one will be >> used >> as the variable store. >> >> Signed-off-by: Ard Biesheuvel <[email protected]> >> --- >> .../AArch64Virtualization-KVM.dsc | 16 +++++- >> .../AArch64Virtualization-KVM.fdf | 4 +- >> .../Include/Platform/KVM/ArmPlatform.h | 6 +++ >> .../Library/NorFlashKVM/NorFlashKVM.c | 63 >> ++++++++++++++++++++++ >> .../Library/NorFlashKVM/NorFlashKVM.inf | 35 ++++++++++++ >> 5 files changed, 122 insertions(+), 2 deletions(-) >> create mode 100644 >> ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.c >> create mode 100644 >> ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.inf >> >> diff --git >> a/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.dsc >> b/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.dsc >> index e0817fca927e..78e83e92be2d 100644 >> --- a/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.dsc >> +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.dsc >> @@ -117,6 +117,16 @@ >> # >> gArmTokenSpaceGuid.PcdArmArchTimerFreqInHz|100000000 >> >> + # >> + # NV Storage PCDs. Use base of 0x04000000 for NOR1 >> + # >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x04000000 >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00040000 >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x04040000 >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00040000 >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x04080000 >> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00040000 >> + > > Hrmpf. These look strange. I'll explain why. > > The promise of this patch is to set up a 64 MB flash chip (which is > absolutely huge, at least for varstore purposes). Then, supposedly, > looking at the definition and the use of the QEMU_NOR_BSIZE macro > (SIZE_256KB, NOR_FLASH_DESCRIPTION.BlockSize), this 64MB flash chip > would be handled in blocks of 256KB. > > Which I could imagine, in theory, but the above PCDs mean different > things. These PCD's control the "Fault Tolerant Write" driver > (MdeModulePkg/Universal/FaultTolerantWriteDxe). > > The FTW driver (no, it doesn't mean "for the win", not by far :)), > implements a kind of "journaled storage" in the flash, the point being > that at no point during flash writes should a power loss cause data > corruption. > [...] > > Here's the things that surprise me: > > - Why require a 64MB flash drive, if we only use 768 KB of it? >
The ARM Versatile Express development hardware is set up like this, that is the whole and only reason. I guess we could change it if there is a strong reason to do so, but parity with the other emulated machines makes the maintenance job a bit easier, I imagine. Both the size and the block size are fixed, currently. > - Why set the block size of the flash drive to 256 KB? That will mean > basically that each of the three areas, live, working, and spare, > consists of 1 block only. Since these beasts are addressed on > the block level, any modification to an area will always rewrite the > entire area (== 1 block). > See above > - In comparison, here's how the OVMF varstore looks like. The block > size is 4 KB. > > +------------------------------+-----------------+--------------------+ > | live data, 14 * 4KB == 56 KB | event log, 4 KB | working area, 4 KB | > +------------------------------+-----------------+--------------------+ > | spare area, 16 * 4KB == 64 KB | > +---------------------------------------------------------------------+ > > The 1st difference is that OVMF has much smaller flash block size (4KB > vs. 256 KB), and that OVMF's varstore is actually organized of *several* > of these 4KB blocks. (Whereas ARM's is just 1 block / area.) > > The 2nd difference is that OVMF has an event log area. No clue why it is > useful, but it's interesting why ARM doesn't have one. > > The 3rd difference is that OVMF's working area is just 1 block (a > fraction of the live data area), while on ARM the live and working block > areas are same-sized. I think this is a direct consequence of the flash > block size being huge (256KB). All areas must be an integral multiple of > the block size, and you can't go below 1 block. > > The 4th difference is that OVMF's spare area, 64KB, mirrors not only the > live area (56KB), but also the event log (4KB) and the working area > (4KB). Whereas on ARM, the spare area (256KB) only mirrors the live area > (256KB), and doesn't cover the working area. > > This all seems valid, in both ARM and OVMF, looking at the > MdeModulePkg.dec descriptions. (And anyway the FTW driver is chock-full > of ASSERT()s, so any invalid config would be caught quickly.) I'm just > wondering where these differences in flash layout come from. > Considering this is all emulated anyway, is there a performance penalty we should taken into account when using such a relatively large block size? >> [PcdsDynamicDefault.common] >> # System Memory -- 1 MB initially, actual size will be fetched from DT >> gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000 >> @@ -179,7 +189,7 @@ >> MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf >> MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf >> MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf >> - MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf >> + MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > I'll mention in passing that > "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf" is > good for a non-authenticated variable store only, ie. no secure boot > support. For secure boot support, CryptoPkg / OpensslLib would be > necessary too, and another variable driver from under SecurityPkg, etc > etc etc, but for this series, the patch definitely suffices. > Yes, I am aware of that. I implemented Secure Boot for the Foundation model and VExpress-A15 here: https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/linaro-topic-authenticated-boot I intend to port those changes onto this platform, which will make my remaining work regarding secure boot a lot easier. (The ARM semihosting interface does not support listing directories only opening files directly, which doesn't work with mokmanager/shim) > In total, although I'm surprised by some of the PCDs here, and by the > size of the flash drive(s), I think this patch is correct. Those PCDs > are probably aligned with ArmPkg / ArmPlatformPkg tradition. > Thanks for the elaborate explanation. -- Ard. ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
