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

[Laszlo Ersek]

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.

In addition, users are already able to specify an arbitrary firmware
file in the <loader> element, and if the value doesn't match a valid
firmware binary file on the host, then the guest won't start. Same
responsibility for the user.

> 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.

If the user wants a completely custom firmware, then he can provide both
<loader> and <nvram> up-front, in which case the config file won't be
consulted.

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.

Laszlo