Re: [libvirt] [PATCH v4 3/3] qemu: Automatically create NVRAM store

On 08/21/14 11:05, Daniel P. Berrange wrote: > On Thu, Aug 21, 2014 at 10:50:24AM +0200, Michal Privoznik wrote: >> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf >> index 7bbbe09..79bba36 100644 >> --- a/src/qemu/qemu.conf >> +++ b/src/qemu/qemu.conf >> @@ -487,3 +487,17 @@ >> # Defaults to 1. >> # >> #log_timestamp = 0 >> + >> + >> +# Location of master nvram file >> +# >> +# When a domain is configured to use UEFI instead of standard >> +# BIOS it may use a separate storage for UEFI variables. If >> +# that's the case libvirt creates the variable store per domain >> +# using this master file as image. Each UEFI firmware can, >> +# however, have different variables store. Therefore the nvram is >> +# a list of strings when a single item is in form of: >> +# ${PATH_TO_UEFI_FW}:${PATH_TO_UEFI_VARS}. >> +# Later, when libvirt creates per domain variable store, this >> +# list is searched for the master image. >> +#nvram = [ "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" ] > > So the user has the ability to specify a arbitrary BIOS in the XML, > but unless it matches one of the ones listed in the libvirt config > they aren't going to be able to start the guest. What can we do > about this, as it doesn't really seem like a great position to be > in. I disagree. Users who use virt-manager (for which patches still have to be written, to expose this feature) won't put arbitrary strings in the <loader> element; virt-manager should offer a minimal choice between "BIOS" vs. "UEFI". Users who are hard-core enough to hack the domain XML by hand are expected to provide good values.

> I wonder if we should have explicitly record the "template" in the > XML too. eg > > <loader>...</loader> > <nvram template="...">...</nvram> > > If no template attribute is given, then don't try to automatically > create the nvram file at all. I don't see how this would help with anything. If the template is not provided, then you immediately branch to the failure case that you don't seem to like above (ie. no varstore file exists for the guest).

And, if the user *wants* to provde a varstore template, then the source of *that* bit of information is precisely the same as the one of the <loader> and <nvram> elements: the user himself. If the user can be trusted (expected) to provide a valid varstore template in nvram/@template, then he can be trusted (expected) just the same to pick a firmware binary that is listed in the config file.

The varstore template is not a VM-level property. The varstore template is a property of the firmware binary.

> Just require the user to pre-create > it. That lets us avoid the global config parameters and any reliance > on OVMF file naming conventions shown below > >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index baa866a..6f79a17 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -67,6 +67,7 @@ >> #include "virstring.h" >> #include "virhostdev.h" >> #include "storage/storage_driver.h" >> +#include "configmake.h" >> >> #define VIR_FROM_THIS VIR_FROM_QEMU >> >> @@ -3734,6 +3735,130 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, >> } >> >> >> +static int >> +qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg, >> + virDomainDefPtr def, >> + bool migrated) >> +{ >> + int ret = -1; >> + int srcFD = -1; >> + int dstFD = -1; >> + virDomainLoaderDefPtr loader = def->os.loader; >> + bool generated = false; >> + bool created = false; >> + >> + /* Unless domain has RO loader of pflash type, we have >> + * nothing to do here. If the loader is RW then it's not >> + * using split code and vars feature, so no nvram file needs >> + * to be created. */ >> + if (!loader || loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH || >> + loader->readonly != VIR_TRISTATE_SWITCH_ON) >> + return 0; >> + >> + /* If the nvram path is configured already, there's nothing >> + * we need to do. Unless we are starting the destination side >> + * of migration in which case nvram is configured in the >> + * domain XML but the file doesn't exist yet. Moreover, after >> + * the migration is completed, qemu will invoke a >> + * synchronization write into the nvram file so we don't have >> + * to take care about transmitting the real data on the other >> + * side. */ >> + if (loader->nvram && !migrated) >> + return 0; >> + >> + /* Autogenerate nvram path if needed.*/ >> + if (!loader->nvram) { >> + if (virAsprintf(&loader->nvram, >> + "%s/lib/libvirt/qemu/nvram/%s_VARS.fd", >> + LOCALSTATEDIR, def->name) < 0) >> + goto cleanup; > > This seems rather x86 OVMF specific in naming. It isn't going to work > for other architectures in QEMU which would use a different NVRAM > format or file naming convention. The code handles solid and split firmwares built from edk2. (Solid: includes firmware code and variable store, hence RW; split: firmware code and varstore are built as separate .fd files, hence firmware code is RO.) The code in general is prepared to handle differences between qemu-system-aarch64 and qemu-system-x86_64 in flash device mapping. I don't think it's useful to try to future-proof this code for architectures for which UEFI support has never even been considered in qemu, let alone in edk2 or the UEFI spec. I think it suffices to adapt this code (if necessary at all) to aarch64 once qemu-system-aarch64 + KVM can boot UEFI at all.