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

On Fri, Aug 22, 2014 at 10:27:29AM +0200, Laszlo Ersek wrote: > 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. The problem I'm raising is that it is *not* sufficient to merely provide good values in the XML here. You can't simply deploy a custom OVMF file and update your XML, because this code is relying on values in the libvirtd.conf configuration file.

>> 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). It helps because it allows you to setup custom OVMF builds without having to edit libvirtd.conf - the information that would go into the libvirtd.conf file is instead provided directly in the XML.

> 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. Nope, that's giving two different experiances. If you use the default OVMF file, then you don't need to care about manually setting up the NVRAM store for each guest becaue libvirt can copy the template. If you use a custom OVMF file apps have to manually setup NVRAM store per guest.

This sucks. The solution of including the nvram template in the XML allows us to have a consistent experiance where libvirt is always able to automatically create the per-guest NVRAM file.

> The varstore template is not a VM-level property. The varstore template > is a property of the firmware binary. Yes, and we're providing the firmware in the XML so it is reasonable to provide the varstore template location in the XML too, instead of having to re-configure libvirtd.conf and restart libvirtd to use it.

> >> 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. I'm not talking about other architectures using UEFI. I'm thinking about the more general case of other architctures using other BIOS entirely which none the less have an NVRAM store. This is why we used a generic XML element of <nvram> instead of some UEFI specific naming. I don't remember what came of it now, but we have been asked for this for either PPC or s390 in the past (can't remember which).