| Message ID | 1409058229-28802-9-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > .../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. Let me quote the PCD descriptions from "MdeModulePkg/MdeModulePkg.dec", in the above order, rewrapping the descriptions: (1) PcdFlashNvStorageVariableBase --> 0x04000000 ## Base address of the NV variable range in flash device Okay. (2) PcdFlashNvStorageVariableSize --> 0x00040000 (256 kilobytes) ## Size of the NV variable range. Note that this value should less ## than or equal to PcdFlashNvStorageFtwSpareSize # The root cause is that variable driver will use FTW protocol to # reclaim variable region. If the length of variable region is larger # than FTW spare size, it means the whole variable region can not be # reflushed through the manner of fault tolerant write. I'll try to explain this "reclaim" thing in a minute (in a quite hand-wavy manner), but first, let's just realize that this PCD controls the *complete size* of the *live data* in the varstore. Then, (3) PcdFlashNvStorageFtwWorkingBase --> 0x04040000 ## Base address of the FTW working block range in flash device. This does make sense. The working block is tacked to the end of the raw varstore blocks. Fine. (4) PcdFlashNvStorageFtwWorkingSize --> 0x00040000 (256 KB again) ## Size of the FTW working block range. This seems valid, but I'm not sure why it is so large. (5) PcdFlashNvStorageFtwSpareBase --> 0x04080000 ## Base address of the FTW spare block range in flash device. Note ## that this value should be block size aligned. The spare area starts after the end of the working area, good. (6) PcdFlashNvStorageFtwSpareSize --> 0x00040000 (256 KB) ## Size of the FTW spare block range. Note that this value should ## larger than PcdFlashNvStorageVariableSize and block size aligned. # The root cause is that variable driver will use FTW protocol to # reclaim variable region. If the length of variable region is larger # than FTW spare size, it means the whole variable region can not be # reflushed through the manner of fault tolerant write. So, the first 768 KB of the flash drive in question will look like: 65536 KB +--------------------+----------------------------+ | live data, 256 KB | working block area, 256 KB | 66048 KB +--------------------+----------------------------+ | spare area, 256 KB | +--------------------+ Live variables are stored in the "live data" area. The working block area is used for one-off updates, as far as I know. The spare area is used for "reclaim", ie. compaction of the live area, when it becomes fragmented due to many variable writes. I had investigated that earlier in some detail, but very roughly, there are such data movements between temporary buffers and the live and spare areas, during reclaim (quoting an earlier email of mine, "I'm skipping the erasures and the in-memory buffer manipulations"): - copy LIVE to MyBuffer - copy SPARE to SpareBuffer - copy MyBuffer to SPARE - copy SPARE to Buffer - copy Buffer to LIVE - copy SpareBuffer to SPARE ("MyBuffer" and "SpareBuffer" are temporary buffers in RAM.) Here's the things that surprise me: - Why require a 64MB flash drive, if we only use 768 KB of it? - 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). - 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. > [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. 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, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 27 August 2014 18:03, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org> >> --- >> .../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.
On 08/27/14 18:58, Ard Biesheuvel wrote: > 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. Thanks for enlightening me. Just stick with it then, I'd say. > Considering this is all emulated anyway, is there a performance > penalty we should taken into account when using such a relatively > large block size? It depends on the NOR flash driver (the driver that produces the FirmwareVolumeBlockProtocol, or FirmwareVolumeBlock2Protocol). The FVB (and FVB2) protocols expose a block-centric view of the flash. The performance (when running on QEMU/KVM) largely depends on the CFI command that the driver uses to reprogram or erase the flash. If those commands are block level, then you trap once per block (I think anyway). If you use byte-level programming, then you trap once per byte (this I know for sure). During FTW reclaim, the whole live and spare areas are rewritten several times. OVMF's flash driver currently uses byte-level CFI commands -- and I have a *very* distant plan to update that to block level, given that the FVB(2) interface is block-level anyway --, and therefore the reclaim is *somewhat* noticeable on x86_64 KVM. You get like 100% system load from the VCPU (the virtual BP) that is running edk2, for 1 or 2 seconds. (Reclaim happens very rarely.) Individual non-volatile variable writes are unnoticeable. Given that OVMF's live area is 56K, and its spare area is 64K (and the block size is only 4K), you could see more serious CPU demand with a block size of 256KB. It probably doesn't matter with TCG (which is slow anyway), but it could be noticeable with KVM. This is pure speculation of course. Also, we could actually check out the FVB(2) driver in question, "ArmPlatformPkg/Drivers/NorFlashDxe": FvbWrite() [NorFlashFvbDxe.c] NorFlashWriteSingleBlock() [NorFlashDxe.c] It seems to have a trick for writes <= 128 bytes (operating with 4-byte words --> NorFlashWriteSingleWord()), and for larger writes, it seems to call NorFlashWriteBlocks() --> NorFlashWriteFullBlock(). NorFlashWriteFullBlock() says // To speed up the programming operation, NOR Flash is programmed using the Buffered Programming method. ... I guess you shouldn't see delays even on KVM; the driver appears performance-conscious. There's probably nothing to do until you can notice and measure any tight loops. > >>> [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 Awesome! > 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) Right. With qemu you can use the vvfat block protocol driver for easy semihosting, such as: -drive file=fat:/host/directory/name,id=drive0,if=none,format=raw \ -device virtio-blk-device,drive=drive0 It can expose a host directory as a FAT16 volume (up to "516.06 MB" in size), which should be enough for keys that you want to enrol etc. Alternatively, guestfish (from libguestfs fame) is very good at quickly modifying offline virtual disk images. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 27 August 2014 17:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 27 August 2014 18:03, Laszlo Ersek <lersek@redhat.com> wrote: >> - 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. Yeah, this isn't a strong requirement from the QEMU side, but the thought process went something like: * two flash devices is better than one, so we can allow a split between what kind of backing file they get (treating ROM image differently than non-volatile storage) * might as well be reasonably generous with the sizing, to allow for future expansion in what software uses * having an asymmetric "one small, one large" setup seems kind of weird * UEFI isn't necessarily the only user of this board model If we've done something silly with the way we've instantiated the flash device in QEMU that means it has a stupid large block size then we might be able to fix that. thanks -- PMM ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 27 August 2014 17:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 27 August 2014 18:03, Laszlo Ersek <lersek@redhat.com> wrote: >> 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. That reminds me, what are our plans for potentially supporting EL2 and/or EL3 booting once QEMU eventually supports them in TCG emulation? Would we have one UEFI image that can cope with being started in any of EL1/2/3, or would we need separate images? (EL2/EL3 support is still a little way off, and in any case KVM will always be EL1 only, so the EL1 config is the most important.) > (The ARM semihosting interface does not support listing directories > only opening files directly, which doesn't work with mokmanager/shim) You probably know this already, but note that enabling semihosting lets the guest do a pile of exciting things to the host so it's not really suitable for production configs anyway... thanks -- PMM ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/27/14 20:09, Peter Maydell wrote: > On 27 August 2014 17:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On 27 August 2014 18:03, Laszlo Ersek <lersek@redhat.com> wrote: >>> - 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. > > Yeah, this isn't a strong requirement from the QEMU side, but > the thought process went something like: > * two flash devices is better than one, so we can allow a split > between what kind of backing file they get (treating ROM > image differently than non-volatile storage) ACK for that, positively > * might as well be reasonably generous with the sizing, to > allow for future expansion in what software uses > * having an asymmetric "one small, one large" setup seems > kind of weird > * UEFI isn't necessarily the only user of this board model Okay. Thanks! > If we've done something silly with the way we've instantiated > the flash device in QEMU that means it has a stupid large > block size then we might be able to fix that. The old wisdom says "measure it first" :) I think this should only matter on KVM, if it matters at all. And one way to measure it (if someone really cares) would be to run a Linux guest, and massage some non-volatile variables from there, in a loop. An example program is under <http://blog.fpmurphy.com/2012/12/efivars-and-efivarfs.html>. However I think this investigation can be postponed indefinitely (unless someone notices a perf degradation with the naked eye). The only thing that caught my eye was the block size being *different* from OVMF's and "hw/i386/pc_sysfw.c"'s, but that isn't necessarily a problem. I think this patch is fine unless proven otherwise in practice. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
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 + [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 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf @@ -197,6 +207,10 @@ ArmPkg/Drivers/ArmGic/ArmGicDxe.inf ArmPkg/Drivers/TimerDxe/TimerDxe.inf + ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf { + <LibraryClasses> + NorFlashPlatformLib|ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.inf + } MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf # diff --git a/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.fdf b/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.fdf index a5fe6e57a264..a6760302ce58 100644 --- a/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.fdf +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/AArch64Virtualization-KVM.fdf @@ -109,8 +109,8 @@ READ_LOCK_STATUS = TRUE INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf - INF MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf @@ -128,6 +128,7 @@ READ_LOCK_STATUS = TRUE INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf + INF ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf # @@ -185,6 +186,7 @@ READ_LOCK_STATUS = TRUE INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf INF ArmPkg/Drivers/CpuPei/CpuPei.inf INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf + INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf diff --git a/ArmPlatformPkg/AArch64VirtualizationPkg/Include/Platform/KVM/ArmPlatform.h b/ArmPlatformPkg/AArch64VirtualizationPkg/Include/Platform/KVM/ArmPlatform.h index f6f694f54390..8dddb1dccd63 100644 --- a/ArmPlatformPkg/AArch64VirtualizationPkg/Include/Platform/KVM/ArmPlatform.h +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/Include/Platform/KVM/ArmPlatform.h @@ -29,4 +29,10 @@ #define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8) #define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9) +#define QEMU_NOR_BSIZE SIZE_256KB +#define QEMU_NOR0_BASE 0x0 +#define QEMU_NOR0_SIZE SIZE_64MB +#define QEMU_NOR1_BASE 0x04000000 +#define QEMU_NOR1_SIZE SIZE_64MB + #endif diff --git a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.c b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.c new file mode 100644 index 000000000000..edaffea20095 --- /dev/null +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.c @@ -0,0 +1,63 @@ +/** @file + + Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> + + This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + + **/ + +#ifndef _NORFLASHPLATFORMLIB_H_ +#define _NORFLASHPLATFORMLIB_H_ + +#include <ArmPlatform.h> + +typedef struct { + UINTN DeviceBaseAddress; // Start address of the Device Base Address (DBA) + UINTN RegionBaseAddress; // Start address of one single region + UINTN Size; + UINTN BlockSize; + EFI_GUID Guid; +} NOR_FLASH_DESCRIPTION; + +EFI_STATUS +NorFlashPlatformInitialization ( + VOID + ) +{ + return EFI_SUCCESS; +} + +NOR_FLASH_DESCRIPTION mNorFlashDevices[] = { + { + QEMU_NOR0_BASE, + QEMU_NOR0_BASE, + QEMU_NOR0_SIZE, + QEMU_NOR_BSIZE, + {0xF9B94AE2, 0x8BA6, 0x409B, 0x9D, 0x56, 0xB9, 0xB4, 0x17, 0xF5, 0x3C, 0xB3} + }, { + QEMU_NOR1_BASE, + QEMU_NOR1_BASE, + QEMU_NOR1_SIZE, + QEMU_NOR_BSIZE, + {0x8047DB4B, 0x7E9C, 0x4C0C, 0x8E, 0xBC, 0xDF, 0xBB, 0xAA, 0xCA, 0xCE, 0x8F} + } +}; + +EFI_STATUS +NorFlashPlatformGetDevices ( + OUT NOR_FLASH_DESCRIPTION **NorFlashDescriptions, + OUT UINT32 *Count + ) +{ + *NorFlashDescriptions = mNorFlashDevices; + *Count = sizeof (mNorFlashDevices) / sizeof (mNorFlashDevices[0]); + return EFI_SUCCESS; +} + +#endif /* _NORFLASHPLATFORMLIB_H_ */ diff --git a/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.inf b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.inf new file mode 100644 index 000000000000..294b18327e5e --- /dev/null +++ b/ArmPlatformPkg/AArch64VirtualizationPkg/Library/NorFlashKVM/NorFlashKVM.inf @@ -0,0 +1,35 @@ +#/** @file +# +# Component description file for NorFlashKVM module +# +# Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR> +# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +#**/ + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = NorFlashKVMLib + FILE_GUID = 339B7829-4C5F-4EFC-B2DD-5050E530DECE + MODULE_TYPE = DXE_DRIVER + VERSION_STRING = 1.0 + LIBRARY_CLASS = NorFlashPlatformLib + +[Sources.common] + NorFlashKVM.c + +[Packages] + MdePkg/MdePkg.dec + ArmPlatformPkg/ArmPlatformPkg.dec + +[LibraryClasses] + BaseLib + DebugLib + IoLib
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 <ard.biesheuvel@linaro.org> --- .../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