Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On Thu, Nov 21, 2013 at 2:21 PM, Laszlo Ersek ler...@redhat.com wrote:
Split the variable store off to a separate file when SPLIT_VARSTORE is
defined.
Is the goal to allow the central OVMF to be updated so the VM will
automatically be running the newest firmware? (While preserving
variables)
I think in your scenario, you are assuming some VM manager will build
the command line parameters. But, couldn't the VM manager also merge
flash updates into the *single* VM specific flash image? Then QEMU
would not need to be changed.
This might also enable a 'feature' where the particular VM instance
can choose to not update the firmware when the central OVMF is
updated.
If we decided splitting was a good thing, then it would be nice to
just always build the split and full images. I'm not sure .fdf can
handle this though. Seems like build.sh could be tweaked if .fdf is
not up to the task.
-Jordan
Even in this case, the preexistent PCDs' values don't change. Qemu must
take care of contiguously mapping NVVARSTORE.fd + OVMF.fd so that when
concatenated they end exactly at 4GB.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
Generated with 8 lines of context for easier in-patch verification with
the help of the clipboard.
OvmfPkg/OvmfPkgIa32.fdf| 48
++
OvmfPkg/OvmfPkgIa32X64.fdf | 48
++
OvmfPkg/OvmfPkgX64.fdf | 48
++
OvmfPkg/README | 16
4 files changed, 160 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index c50709c..310d6a9 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -23,31 +23,51 @@
#
[Defines]
!if $(TARGET) == RELEASE
!ifndef $(FD_SIZE_2MB)
DEFINE FD_SIZE_1MB=
!endif
!endif
+!ifdef $(SPLIT_VARSTORE)
+!ifdef $(FD_SIZE_1MB)
+[FD.NvVarStore]
+BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0010
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!else
+[FD.NvVarStore]
+BaseAddress = 0xFFE0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0020
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!endif
+!else
!ifdef $(FD_SIZE_1MB)
[FD.OVMF]
BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
Size = 0x0010|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
ErasePolarity = 1
BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
NumBlocks = 0x100
!else
[FD.OVMF]
BaseAddress = 0xFFE0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
Size = 0x0020|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
ErasePolarity = 1
BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
NumBlocks = 0x200
!endif
+!endif
0x|0xe000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
#NV_VARIABLE_STORE
DATA = {
## This is the EFI_FIRMWARE_VOLUME_HEADER
# ZeroVector []
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -106,30 +126,58 @@ DATA = {
# WriteQueueSize: UINT64
0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
}
0x0001|0x0001
#NV_FTW_SPARE
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+!ifdef $(SPLIT_VARSTORE)
+!ifdef $(FD_SIZE_1MB)
+[FD.OVMF]
+BaseAddress = 0xFFF2
+Size = 0x000E
+ErasePolarity = 1
+BlockSize = 0x1000
+NumBlocks = 0xE0
+
+0x|0x000CC000
+FV = FVMAIN_COMPACT
+0x000CC000|0x14000
+FV = SECFV
+!else
+[FD.OVMF]
+BaseAddress = 0xFFE2
+Size = 0x001E
+ErasePolarity = 1
+BlockSize = 0x1000
+NumBlocks = 0x1E0
+
+0x|0x001AC000
+FV = FVMAIN_COMPACT
+0x001AC000|0x34000
+FV = SECFV
+!endif
+!else
!ifdef $(FD_SIZE_1MB)
0x0002|0x000CC000
FV = FVMAIN_COMPACT
0x000EC000|0x14000
FV = SECFV
!else
0x0002|0x001AC000
FV = FVMAIN_COMPACT
0x001CC000|0x34000
FV = SECFV
!endif
+!endif
[FD.MEMFD]
BaseAddress = 0x80|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvBase
Size = 0x80|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvSize
ErasePolarity = 1
BlockSize = 0x1
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index d65f40f..8877a89 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On 11/22/13 18:37, Jordan Justen wrote: On Thu, Nov 21, 2013 at 2:21 PM, Laszlo Ersek ler...@redhat.com wrote: Split the variable store off to a separate file when SPLIT_VARSTORE is defined. Is the goal to allow the central OVMF to be updated so the VM will automatically be running the newest firmware? (While preserving variables) Yes. In some distros this is what happens (*) when you uprage the seabios(-bin) package on the host system. When you reboot any VM on it, it will see the upgraded bios. (*) Except of course you have no variable store. The bios binary (the file belonging to its containing package) is also owned by root and has some nice file mode bits and SELinux permissions: $ ls -lL /usr/share/qemu-kvm/bios.bin -rw-r--r--. 1 root root 131072 2013-11-20 14:55:29 +0100 /usr/share/qemu-kvm/bios.bin $ ls -lLZ /usr/share/qemu-kvm/bios.bin -rw-r--r--. root root system_u:object_r:usr_t:s0 /usr/share/qemu-kvm/bios.bin These are distinctly different from what libvirt sets for the private image files that underneath the VM's disks. OVMF.fd would correspond to bios.bin above, and NVVARSTORE.fd is a private disk file. I think in your scenario, you are assuming some VM manager will build the command line parameters. But, couldn't the VM manager also merge flash updates into the *single* VM specific flash image? Then QEMU would not need to be changed. Correct. I floated this idea to the libvirt guys and Cole (of virt-manager fame). The merging proposal was frowned upon. (Also, if we're talking explicit reflashing, maybe the user should click a button on virt-manager to request the update... Not sure.) So basically these patches are just the non-merging alternative that is perceived as more suitable for some distros. This might also enable a 'feature' where the particular VM instance can choose to not update the firmware when the central OVMF is updated. Correct. (See the click the button thing above.) However right now VMs have no choice, and auto-upgrading their OVMF wouldn't be a step back. But, your remark is 100% valid. If we decided splitting was a good thing, then it would be nice to just always build the split and full images. I'm not sure .fdf can handle this though. I think it can, if we add all three FDs with different names (like OVMF.fd, OVMF_SPLIT.fd, NVVARSTORE.fd). I think the build process reuses the FV files if there are multiple referring FDs -- I seem to recall that's already happening between MEMFD.fd and OVMF.fd, no? Seems like build.sh could be tweaked if .fdf is not up to the task. Certainly -- just invoke it twice with different params. OTOH building all three FDs per default might be confusing for end-users. We know for a fact that they usually don't read the README (or not thoroughly enough). If we only give them one output file per default, that's less potential for confusion. But I can certainly post a version where all three files are built per default, if that's what you prefer (and aren't opposed to the idea in general). Thanks! Laszlo
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On Fri, Nov 22, 2013 at 10:43 AM, Laszlo Ersek ler...@redhat.com wrote: OTOH building all three FDs per default might be confusing for end-users. We know for a fact that they usually don't read the README (or not thoroughly enough). If we only give them one output file per default, that's less potential for confusion. If it will just add confusion, then perhaps it is something best left out of the README. And at that point, since splitting it is so easy, the value of having it in OVMF is debatable. But I can certainly post a version where all three files are built per default, if that's what you prefer (and aren't opposed to the idea in general). I'm not opposed to it, but I would like to wait to see what QEMU does. Many of these scenarios were discussed in the past on qemu-devel, but a single -pflash was the only thing that stuck. This has made me just focus on making the single file pflash work. You also can't deny that the QEMU command line gets ugly real fast. Tangentially related idea: if the user specifies -pflash to a non-existent file, then QEMU could copy 'pflash-$(arch).bin' from the roms folder into that file, and then the use it for the flash. It could make using a flash almost as easy as using the default bios. -Jordan
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On 11/22/2013 01:51 PM, Jordan Justen wrote: Tangentially related idea: if the user specifies -pflash to a non-existent file, then QEMU could copy 'pflash-$(arch).bin' from the roms folder into that file, and then the use it for the flash. It could make using a flash almost as easy as using the default bios. But that image won't be auto-updated the next time the master is updated. Besides, under sVirt protections of libvirt, libvirt would have to pre-create the file (because libvirt intentionally denies qemu the ability to create files), so libvirt would have to be aware of the default. It's not much harder to make libvirt aware of handling a split image, and a split image is easier to handle than having to copy the default into a destination. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On Fri, Nov 22, 2013 at 12:54 PM, Eric Blake ebl...@redhat.com wrote: On 11/22/2013 01:51 PM, Jordan Justen wrote: Tangentially related idea: if the user specifies -pflash to a non-existent file, then QEMU could copy 'pflash-$(arch).bin' from the roms folder into that file, and then the use it for the flash. It could make using a flash almost as easy as using the default bios. But that image won't be auto-updated the next time the master is updated. Yes. We've decided to call that a feature. :) https://lists.gnu.org/archive/html/qemu-devel/2011-07/msg02507.html Besides, under sVirt protections of libvirt, libvirt would have to pre-create the file (because libvirt intentionally denies qemu the ability to create files), so libvirt would have to be aware of the default. It's not much harder to make libvirt aware of handling a split image, and a split image is easier to handle than having to copy the default into a destination. Well, admittedly, this idea is to optimize for qemu command line ease-of-use. So I seem to be targeting a niche market. :) But I still like to run qemu directly. -Jordan
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On 11/22/13 21:51, Jordan Justen wrote: On Fri, Nov 22, 2013 at 10:43 AM, Laszlo Ersek ler...@redhat.com wrote: OTOH building all three FDs per default might be confusing for end-users. We know for a fact that they usually don't read the README (or not thoroughly enough). If we only give them one output file per default, that's less potential for confusion. If it will just add confusion, then perhaps it is something best left out of the README. And at that point, since splitting it is so easy, the value of having it in OVMF is debatable. But I can certainly post a version where all three files are built per default, if that's what you prefer (and aren't opposed to the idea in general). I'm not opposed to it, but I would like to wait to see what QEMU does. OK. Let's see where the qemu patch goes (I'll have to fix the comment typo that Eric found), and if it is accepted (maybe with modifications), we can return to the OVMF patch. Many of these scenarios were discussed in the past on qemu-devel, but a single -pflash was the only thing that stuck. But (thanks to you) we now have a flash driver in OVMF that works *in practice*. The discussion about multiple -pflash flags is not academic any longer. (Hm, sorry, that's maybe too strong -- I can't really say if the discussion used to be academic before. But it cannot have been this practical.) We also know that the libvirt developers prefer the split file. Plus: This has made me just focus on making the single file pflash work. You also can't deny that the QEMU command line gets ugly real fast. We're in violent agreement on that -- which emphasizes the need for good libvirt support all the more. Seriously, I refuse to work with the qemu command line day to day. I have an elaborate wrapper script that I insert between qemu and libvirt (specified in the emulator libvirt domain XML) that post-processes the arguments composed by libvirt. I need this to test out features that libvirt doesn't yet support. For example, I can set various filenames in the loader element -- it specifies the argument to the -bios option. In the script I check the argument against various patterns, and I can turn it into: - -bios $ARG - -pflash $ARG - -pflash $ARG -pflash $(manipulate $ARG) The script handles CSM vs. pure-UEFI for Windows 2008, it can honor upstream vs. RHEL qemu differences, it can turn off the iPXE virtio-net driver. In the single -pflash option case, it does refresh the code part in the VM's copy of OVMF (with dd) from my most recent build. It sets a name-dependent output file for the debug port. Etc. Maintaining this script over months has paid off orders of magnitude better than working with the raw qemu command line would have. (I can compare: on my (occasionally used) AMD desktop that runs Debian I have not bothered to install libvirt, and each time I need to modify the forty line qemu command that I keep in a script file, I cringe. And I can't bring myself to install a second guest.) The convenience/efficiency libvirt gives me is critical. Changing boot order, adding or removing devices, starting stopping -- these are very fast in virt-manager. Editing the configuration with virsh is nice (and it has some level of automatic verification when you save and exit). Comparing guest configs against each other (using the XMLs) is very convenient. And so on. I depend on these every day, but I need to modify the wrapper script only occasionally. Libvirt still allows me to send custom monitor commands, and I can set it to log all of its *own* commands that it sends to the monitor or the guest agent. I've always wondered how other developers (for example, you :)) can exist without libvirt at all! That is why keep the qemu command line simple (== approx. two -pflash options are inconvenient) is no concern of mine. That ship has sailed. Thanks, Laszlo
Re: [Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
On 11/22/13 21:51, Jordan Justen wrote: Many of these scenarios were discussed in the past on qemu-devel, but a single -pflash was the only thing that stuck. This has made me just focus on making the single file pflash work. I almost forgot to reflect on this -- I'm extremely grateful to you that you implemented the flash driver. I tried to be as non-intrusive in my OVMF patch as possible. Keeping the PCD values and the original memory layout was crucial -- I wanted the driver to continue working as-is. Everything else (the qemu patch, for example) came from that. In fact I wrote the OVMF patch first, and then said let's see how we can accommodate this in qemu. (Sorry about answering twice.) Laszlo
[Qemu-devel] [edk2 PATCH] OvmfPkg: split the variable store to a separate file
Split the variable store off to a separate file when SPLIT_VARSTORE is
defined.
Even in this case, the preexistent PCDs' values don't change. Qemu must
take care of contiguously mapping NVVARSTORE.fd + OVMF.fd so that when
concatenated they end exactly at 4GB.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek ler...@redhat.com
---
Generated with 8 lines of context for easier in-patch verification with
the help of the clipboard.
OvmfPkg/OvmfPkgIa32.fdf| 48 ++
OvmfPkg/OvmfPkgIa32X64.fdf | 48 ++
OvmfPkg/OvmfPkgX64.fdf | 48 ++
OvmfPkg/README | 16
4 files changed, 160 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index c50709c..310d6a9 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -23,31 +23,51 @@
#
[Defines]
!if $(TARGET) == RELEASE
!ifndef $(FD_SIZE_2MB)
DEFINE FD_SIZE_1MB=
!endif
!endif
+!ifdef $(SPLIT_VARSTORE)
+!ifdef $(FD_SIZE_1MB)
+[FD.NvVarStore]
+BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0010
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!else
+[FD.NvVarStore]
+BaseAddress = 0xFFE0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0020
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!endif
+!else
!ifdef $(FD_SIZE_1MB)
[FD.OVMF]
BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
Size = 0x0010|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
ErasePolarity = 1
BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
NumBlocks = 0x100
!else
[FD.OVMF]
BaseAddress = 0xFFE0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
Size = 0x0020|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
ErasePolarity = 1
BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
NumBlocks = 0x200
!endif
+!endif
0x|0xe000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
#NV_VARIABLE_STORE
DATA = {
## This is the EFI_FIRMWARE_VOLUME_HEADER
# ZeroVector []
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -106,30 +126,58 @@ DATA = {
# WriteQueueSize: UINT64
0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
}
0x0001|0x0001
#NV_FTW_SPARE
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+!ifdef $(SPLIT_VARSTORE)
+!ifdef $(FD_SIZE_1MB)
+[FD.OVMF]
+BaseAddress = 0xFFF2
+Size = 0x000E
+ErasePolarity = 1
+BlockSize = 0x1000
+NumBlocks = 0xE0
+
+0x|0x000CC000
+FV = FVMAIN_COMPACT
+0x000CC000|0x14000
+FV = SECFV
+!else
+[FD.OVMF]
+BaseAddress = 0xFFE2
+Size = 0x001E
+ErasePolarity = 1
+BlockSize = 0x1000
+NumBlocks = 0x1E0
+
+0x|0x001AC000
+FV = FVMAIN_COMPACT
+0x001AC000|0x34000
+FV = SECFV
+!endif
+!else
!ifdef $(FD_SIZE_1MB)
0x0002|0x000CC000
FV = FVMAIN_COMPACT
0x000EC000|0x14000
FV = SECFV
!else
0x0002|0x001AC000
FV = FVMAIN_COMPACT
0x001CC000|0x34000
FV = SECFV
!endif
+!endif
[FD.MEMFD]
BaseAddress = 0x80|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvBase
Size = 0x80|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfMemFvSize
ErasePolarity = 1
BlockSize = 0x1
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index d65f40f..8877a89 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -23,31 +23,51 @@
#
[Defines]
!if $(TARGET) == RELEASE
!ifndef $(FD_SIZE_2MB)
DEFINE FD_SIZE_1MB=
!endif
!endif
+!ifdef $(SPLIT_VARSTORE)
+!ifdef $(FD_SIZE_1MB)
+[FD.NvVarStore]
+BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0010
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!else
+[FD.NvVarStore]
+BaseAddress = 0xFFE0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = 0x0020
+Size = 0x2
+ErasePolarity = 1
+BlockSize = 0x1000|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
+NumBlocks = 0x20
+!endif
+!else
!ifdef $(FD_SIZE_1MB)
[FD.OVMF]
BaseAddress = 0xFFF0|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
Size
