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 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. 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.
> +
> + generated = true;
> + }
> +
> + if (!virFileExists(loader->nvram)) {
> + size_t i;
> + ssize_t r;
> +
> + for (i = 0; i < cfg->nloader; i++) {
> + if (STREQ(cfg->loader[i], loader->path))
> + break;
> + }
> +
> + if (i == cfg->nloader) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("unable to find any master var store for "
> + "loader: %s"), loader->path);
> + goto cleanup;
> + }
> +
> + if ((srcFD = virFileOpenAs(cfg->nvram[i], O_RDONLY,
> + 0, -1, -1, 0)) < 0) {
> + virReportSystemError(-srcFD,
> + _("Failed to open file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> + if ((dstFD = virFileOpenAs(loader->nvram,
> + O_WRONLY | O_CREAT | O_EXCL,
> + S_IRUSR | S_IWUSR,
> + cfg->user, cfg->group, 0)) < 0) {
> + virReportSystemError(-dstFD,
> + _("Failed to create file '%s'"),
> + loader->nvram);
> + goto cleanup;
> + }
> + created = true;
> +
> + do {
> + char buf[1024];
> +
> + if ((r = saferead(srcFD, buf, sizeof(buf))) < 0) {
> + virReportSystemError(errno,
> + _("Unable to read from file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> +
> + if (safewrite(dstFD, buf, r) < 0) {
> + virReportSystemError(errno,
> + _("Unable to write to file '%s'"),
> + loader->nvram);
> + goto cleanup;
> + }
> + } while (r);
> +
> + if (VIR_CLOSE(srcFD) < 0) {
> + virReportSystemError(errno,
> + _("Unable to close file '%s'"),
> + cfg->nvram[i]);
> + goto cleanup;
> + }
> + if (VIR_CLOSE(dstFD) < 0) {
> + virReportSystemError(errno,
> + _("Unable to close file '%s'"),
> + loader->nvram);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> + cleanup:
> + /* We successfully generated the nvram path, but failed to
> + * copy the file content. Roll back. */
> + if (ret < 0) {
> + if (created)
> + unlink(loader->nvram);
> + if (generated)
> + VIR_FREE(loader->nvram);
> + }
> +
> + VIR_FORCE_CLOSE(srcFD);
> + VIR_FORCE_CLOSE(dstFD);
> + return ret;
> +}
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list