[libvirt] [PATCH v4 0/3] OVMF exposure
diff to v3: -Pure rebae & resend diff to v2: -Adapted to Laszlo's review on v2 Michal Privoznik (3): conf: Extend <loader/> and introduce <nvram/> qemu: Implement extended loader and nvram qemu: Automatically create NVRAM store docs/formatdomain.html.in | 19 ++- docs/schemas/domaincommon.rng | 21 ++++ libvirt.spec.in | 2 + src/Makefile.am | 1 + src/conf/domain_conf.c | 87 +++++++++++++- src/conf/domain_conf.h | 22 +++- src/libvirt_private.syms | 3 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 14 +++ src/qemu/qemu_command.c | 97 ++++++++++++++- src/qemu/qemu_conf.c | 93 +++++++++++++++ src/qemu/qemu_conf.h | 5 + src/qemu/qemu_process.c | 132 +++++++++++++++++++++ src/qemu/test_libvirtd_qemu.aug.in | 3 + src/security/security_dac.c | 8 ++ src/security/security_selinux.c | 8 ++ src/security/virt-aa-helper.c | 4 +- src/vbox/vbox_common.c | 7 +- src/xenapi/xenapi_driver.c | 3 +- src/xenconfig/xen_common.c | 7 +- src/xenconfig/xen_sxpr.c | 16 +-- .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args | 10 ++ tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml | 40 +++++++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-pci-bridge-many-disks.xml | 2 +- tests/qemuxml2xmltest.c | 2 + tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml | 2 +- .../sexpr2xml-fv-serial-dev-2-ports.xml | 2 +- .../sexpr2xml-fv-serial-dev-2nd-port.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml | 2 +- .../sexpr2xml-fv-serial-tcp-telnet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-utc.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-v2.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-escape-paths.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-hpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-nohpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-localtime.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-ioemu.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-netfront.xml | 2 +- tests/xmconfigdata/test-fullvirt-new-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-old-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-parallel-tcp.xml | 2 +- .../test-fullvirt-serial-dev-2-ports.xml | 2 +- .../test-fullvirt-serial-dev-2nd-port.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-file.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-null.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pipe.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pty.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-stdio.xml | 2 +- .../test-fullvirt-serial-tcp-telnet.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-tcp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-udp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-unix.xml | 2 +- tests/xmconfigdata/test-fullvirt-sound.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbmouse.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbtablet.xml | 2 +- tests/xmconfigdata/test-fullvirt-utc.xml | 2 +- tests/xmconfigdata/test-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-pci-devs.xml | 2 +- 81 files changed, 639 insertions(+), 82 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml -- 1.8.5.5
Up to now, users can configure BIOS via the <loader/> element. With the upcoming implementation of UEFI this is not enough as BIOS and UEFI are conceptually different. For instance, while BIOS is ROM, UEFI is programmable flash (although all writes to code section are denied). Therefore we need new attribute @type which will differentiate the two. Then, new attribute @readonly is introduced to reflect the fact that some images are RO. Moreover, the OVMF (which is going to be used mostly), works in two modes: 1) Code and UEFI variable store is mixed in one file. 2) Code and UEFI variable store is separated in two files The latter has advantage of updating the UEFI code without losing the configuration. However, in order to represent the latter case we need yet another XML element: <nvram/>. Currently, it has no additional attributes, it's just a bare element containing path to the variable store file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 19 ++++- docs/schemas/domaincommon.rng | 21 ++++++ src/conf/domain_conf.c | 87 +++++++++++++++++++++- src/conf/domain_conf.h | 22 +++++- src/libvirt_private.syms | 3 + src/qemu/qemu_command.c | 5 +- src/security/virt-aa-helper.c | 4 +- src/vbox/vbox_common.c | 7 +- src/xenapi/xenapi_driver.c | 3 +- src/xenconfig/xen_common.c | 7 +- src/xenconfig/xen_sxpr.c | 16 ++-- tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml | 40 ++++++++++ .../qemuxml2xmlout-pci-bridge-many-disks.xml | 2 +- tests/qemuxml2xmltest.c | 2 + tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml | 2 +- .../sexpr2xml-fv-serial-dev-2-ports.xml | 2 +- .../sexpr2xml-fv-serial-dev-2nd-port.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml | 2 +- .../sexpr2xml-fv-serial-tcp-telnet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-utc.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-v2.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-escape-paths.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-hpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-nohpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-localtime.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-ioemu.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-netfront.xml | 2 +- tests/xmconfigdata/test-fullvirt-new-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-old-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-parallel-tcp.xml | 2 +- .../test-fullvirt-serial-dev-2-ports.xml | 2 +- .../test-fullvirt-serial-dev-2nd-port.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-file.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-null.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pipe.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pty.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-stdio.xml | 2 +- .../test-fullvirt-serial-tcp-telnet.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-tcp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-udp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-unix.xml | 2 +- tests/xmconfigdata/test-fullvirt-sound.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbmouse.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbtablet.xml | 2 +- tests/xmconfigdata/test-fullvirt-utc.xml | 2 +- tests/xmconfigdata/test-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-pci-devs.xml | 2 +- 69 files changed, 269 insertions(+), 79 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ed17389..589ca0a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -102,7 +102,8 @@ ... <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> <boot dev='cdrom'/> <bootmenu enable='yes'/> @@ -129,7 +130,21 @@ used to assist the domain creation process. It is used by Xen fully virtualized domains as well as setting the QEMU BIOS file path for QEMU/KVM domains. <span class="since">Xen since 0.1.0, - QEMU/KVM since 0.9.12</span></dd> + QEMU/KVM since 0.9.12</span> Then, <span class="since">Since + 1.2.8</span> it's possible for the element to have two + optional attributes: <code>readonly</code> (accepted values are + <code>on</code> and <code>off</code>) to reflect the fact that the + image should be writable or read-only. The second attribute + <code>type</code> accepts values <code>rom</code> and + <code>pflash</code>. It tells the hypervisor where in the guest + memory the file should be mapped. For instance, if the loader + path points to an UEFI image, <code>type</code> should be + <code>pflash</code>.</dd> + <dt><code>nvram</code></dt> + <dd>Some UEFI firmwares may want to use a non-volatile memory to store + some variables. In the host, this is represented as a file and the + path to the file is stored in this element. <span class="since">Since + 1.2.8</span></dd> <dt><code>boot</code></dt> <dd>The <code>dev</code> attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 033f2f6..69fce46 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -242,6 +242,27 @@ <interleave> <optional> <element name="loader"> + <optional> + <attribute name="readonly"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="type"> + <choice> + <value>rom</value> + <value>pflash</value> + </choice> + </attribute> + </optional> + <ref name="absFilePath"/> + </element> + </optional> + <optional> + <element name="nvram"> <ref name="absFilePath"/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9557020..1ed7315 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -759,6 +759,11 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "abort", "pivot") +VIR_ENUM_IMPL(virDomainLoader, + VIR_DOMAIN_LOADER_TYPE_LAST, + "rom", + "pflash") + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob) @@ -1992,6 +1997,17 @@ virDomainPanicDefFree(virDomainPanicDefPtr panic) VIR_FREE(panic); } +void +virDomainLoaderDefFree(virDomainLoaderDefPtr loader) +{ + if (!loader) + return; + + VIR_FREE(loader->path); + VIR_FREE(loader->nvram); + VIR_FREE(loader); +} + void virDomainDefFree(virDomainDefPtr def) { size_t i; @@ -2097,7 +2113,7 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->os.cmdline); VIR_FREE(def->os.dtb); VIR_FREE(def->os.root); - VIR_FREE(def->os.loader); + virDomainLoaderDefFree(def->os.loader); VIR_FREE(def->os.bootloader); VIR_FREE(def->os.bootloaderArgs); @@ -11581,6 +11597,42 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) return 0; } +static int +virDomainLoaderDefParseXML(xmlNodePtr node, + virDomainLoaderDefPtr loader) +{ + int ret = -1; + char *readonly_str = NULL; + char *type_str = NULL; + + readonly_str = virXMLPropString(node, "readonly"); + type_str = virXMLPropString(node, "type"); + loader->path = (char *) xmlNodeGetContent(node); + + if (readonly_str && + (loader->readonly = virTristateSwitchTypeFromString(readonly_str)) <= 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown readonly value: %s"), readonly_str); + goto cleanup; + } + + if (type_str) { + int type; + if ((type = virDomainLoaderTypeFromString(type_str)) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown type value: %s"), type_str); + goto cleanup; + } + loader->type = type; + } + + ret = 0; + cleanup: + VIR_FREE(readonly_str); + VIR_FREE(type_str); + return ret; +} + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -12558,12 +12610,22 @@ virDomainDefParseXML(xmlDocPtr xml, if (STREQ(def->os.type, "xen") || STREQ(def->os.type, "hvm") || STREQ(def->os.type, "uml")) { + xmlNodePtr loader_node; + def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt); def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt); def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt); def->os.dtb = virXPathString("string(./os/dtb[1])", ctxt); def->os.root = virXPathString("string(./os/root[1])", ctxt); - def->os.loader = virXPathString("string(./os/loader[1])", ctxt); + if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { + if (VIR_ALLOC(def->os.loader) < 0) + goto error; + + if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) + goto error; + + def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); + } } if (STREQ(def->os.type, "hvm")) { @@ -17605,6 +17667,23 @@ virDomainHugepagesFormat(virBufferPtr buf, virBufferAddLit(buf, "</hugepages>\n"); } +static void +virDomainLoaderDefFormat(virBufferPtr buf, + virDomainLoaderDefPtr loader) +{ + const char *readonly = virTristateSwitchTypeToString(loader->readonly); + const char *type = virDomainLoaderTypeToString(loader->type); + + virBufferAddLit(buf, "<loader"); + + if (loader->readonly) + virBufferAsprintf(buf, " readonly='%s'", readonly); + + virBufferAsprintf(buf, " type='%s'>", type); + + virBufferEscapeString(buf, "%s</loader>\n", loader->path); + virBufferEscapeString(buf, "<nvram>%s</nvram>\n", loader->nvram); +} #define DUMPXML_FLAGS \ (VIR_DOMAIN_XML_SECURE | \ @@ -17934,8 +18013,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, for (i = 0; def->os.initargv && def->os.initargv[i]; i++) virBufferEscapeString(buf, "<initarg>%s</initarg>\n", def->os.initargv[i]); - virBufferEscapeString(buf, "<loader>%s</loader>\n", - def->os.loader); + if (def->os.loader) + virDomainLoaderDefFormat(buf, def->os.loader); virBufferEscapeString(buf, "<kernel>%s</kernel>\n", def->os.kernel); virBufferEscapeString(buf, "<initrd>%s</initrd>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f2df4eb..3f26423 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1620,6 +1620,26 @@ struct _virDomainBIOSDef { int rt_delay; }; +typedef enum { + VIR_DOMAIN_LOADER_TYPE_ROM = 0, + VIR_DOMAIN_LOADER_TYPE_PFLASH, + + VIR_DOMAIN_LOADER_TYPE_LAST +} virDomainLoader; + +VIR_ENUM_DECL(virDomainLoader) + +typedef struct _virDomainLoaderDef virDomainLoaderDef; +typedef virDomainLoaderDef *virDomainLoaderDefPtr; +struct _virDomainLoaderDef { + char *path; + int readonly; /* enum virTristateSwitch */ + virDomainLoader type; + char *nvram; /* path to non-volatile RAM */ +}; + +void virDomainLoaderDefFree(virDomainLoaderDefPtr loader); + /* Operating system configuration data & machine / arch */ typedef struct _virDomainOSDef virDomainOSDef; typedef virDomainOSDef *virDomainOSDefPtr; @@ -1637,7 +1657,7 @@ struct _virDomainOSDef { char *cmdline; char *dtb; char *root; - char *loader; + virDomainLoaderDefPtr loader; char *bootloader; char *bootloaderArgs; int smbios_mode; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 08111d4..dd4c46f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -312,6 +312,9 @@ virDomainLifecycleCrashTypeToString; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; virDomainLiveConfigHelperMethod; +virDomainLoaderDefFree; +virDomainLoaderTypeFromString; +virDomainLoaderTypeToString; virDomainLockFailureTypeFromString; virDomainLockFailureTypeToString; virDomainMemballoonModelTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 903dafd..d03c4d5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7439,7 +7439,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (def->os.loader) { virCommandAddArg(cmd, "-bios"); - virCommandAddArg(cmd, def->os.loader); + virCommandAddArg(cmd, def->os.loader->path); } /* Set '-m MB' based on maxmem, because the lower 'memory' limit @@ -11229,7 +11229,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; } else if (STREQ(arg, "-bios")) { WANT_VALUE(); - if (VIR_STRDUP(def->os.loader, val) < 0) + if (VIR_ALLOC(def->os.loader) < 0 || + VIR_STRDUP(def->os.loader->path, val) < 0) goto error; } else if (STREQ(arg, "-initrd")) { WANT_VALUE(); diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a0b104c..311ce3b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1006,8 +1006,8 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.dtb, "r") != 0) goto cleanup; - if (ctl->def->os.loader && ctl->def->os.loader) - if (vah_add_file(&buf, ctl->def->os.loader, "r") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->path) + if (vah_add_file(&buf, ctl->def->os.loader->path, "r") != 0) goto cleanup; for (i = 0; i < ctl->def->ngraphics; i++) { diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b186ea8..6f56c59 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -988,7 +988,12 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxGlobalData *data, VIR_DEBUG("def->os.initrd %s", def->os.initrd); VIR_DEBUG("def->os.cmdline %s", def->os.cmdline); VIR_DEBUG("def->os.root %s", def->os.root); - VIR_DEBUG("def->os.loader %s", def->os.loader); + if (def->os.loader) { + VIR_DEBUG("def->os.loader->path %s", def->os.loader->path); + VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly); + VIR_DEBUG("def->os.loader->type %d", def->os.loader->type); + VIR_DEBUG("def->os.loader->nvram %s", def->os.loader->nvram); + } VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 50331c9..a6eaccc 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1427,7 +1427,8 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) VIR_FREE(boot_policy); goto error; } - if (VIR_STRDUP(defPtr->os.loader, "pygrub") < 0) { + if (VIR_ALLOC(defPtr->os.loader) < 0 || + VIR_STRDUP(defPtr->os.loader->path, "pygrub") < 0) { VIR_FREE(boot_policy); goto error; } diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 398e9ec..df74c41 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -1065,7 +1065,8 @@ xenParseOS(virConfPtr conf, virDomainDefPtr def) if (STREQ(def->os.type, "hvm")) { const char *boot; - if (xenConfigCopyString(conf, "kernel", &def->os.loader) < 0) + if (VIR_ALLOC(def->os.loader) < 0 || + xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0) return -1; if (xenConfigGetString(conf, "boot", &boot, "c") < 0) @@ -1740,8 +1741,8 @@ xenFormatOS(virConfPtr conf, virDomainDefPtr def) if (xenXMConfigSetString(conf, "builder", "hvm") < 0) return -1; - if (def->os.loader && - xenXMConfigSetString(conf, "kernel", def->os.loader) < 0) + if (def->os.loader && def->os.loader->path && + xenXMConfigSetString(conf, "kernel", def->os.loader->path) < 0) return -1; for (i = 0; i < def->os.nBootDevs; i++) { diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index ff81c36..e8b9f59 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -93,13 +93,15 @@ xenParseSxprOS(const struct sexpr *node, int hvm) { if (hvm) { - if (sexpr_node_copy(node, "domain/image/hvm/loader", &def->os.loader) < 0) + if (VIR_ALLOC(def->os.loader) < 0) goto error; - if (def->os.loader == NULL) { - if (sexpr_node_copy(node, "domain/image/hvm/kernel", &def->os.loader) < 0) + if (sexpr_node_copy(node, "domain/image/hvm/loader", &def->os.loader->path) < 0) + goto error; + if (def->os.loader->path == NULL) { + if (sexpr_node_copy(node, "domain/image/hvm/kernel", &def->os.loader->path) < 0) goto error; - if (def->os.loader == NULL) { + if (def->os.loader->path == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain information incomplete, missing HVM loader")); return -1; @@ -128,7 +130,7 @@ xenParseSxprOS(const struct sexpr *node, /* If HVM kenrel == loader, then old xend, so kill off kernel */ if (hvm && def->os.kernel && - STREQ(def->os.kernel, def->os.loader)) { + STREQ(def->os.kernel, def->os.loader->path)) { VIR_FREE(def->os.kernel); } /* Drop kernel argument that has no value */ @@ -2279,9 +2281,9 @@ xenFormatSxpr(virConnectPtr conn, if (hvm) { char bootorder[VIR_DOMAIN_BOOT_LAST+1]; if (def->os.kernel) - virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader->path); else - virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader->path); virBufferAsprintf(&buf, "(vcpus %u)", def->maxvcpus); if (def->vcpus < def->maxvcpus) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml new file mode 100644 index 0000000..b11ee0a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>test-bios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <loader readonly='on' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram>/usr/share/OVMF/OVMF_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='tablet' bus='usb'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml index d469b8b..d49f5f4 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type> - <loader>/usr/share/seabios/bios.bin</loader> + <loader type='rom'>/usr/share/seabios/bios.bin</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 5941323..673aa87 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -388,6 +388,8 @@ mymain(void) DO_TEST_DIFFERENT("numatune-memnode"); DO_TEST("numatune-memnode-no-memory"); + DO_TEST("bios-nvram"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml b/tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml index 69fe9ef..761952c 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml b/tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml index 3c3147d..2898098 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml b/tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml index 716f16b..a0fe30d 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml b/tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml index 3dd648b..851797d 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml index 29c1335..09cfe19 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>2</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml b/tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml index 9c59644..44c0f61 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml b/tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml index 67b0b95..29007f0 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml b/tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml index 86b32e9..3dbc999 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml b/tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml index ed7da80..d96350e 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml b/tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml index ed3fde6..7ad377c 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml index 7f5a729..adba6cb 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml index 10f84dc..b6c3601 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml index a3fd231..dabe679 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml index b3f77c9..fb19d74 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml index e217161..5aa425b 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml index 3ad2264..3c2ca21 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml index 001df56..160edbd 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp-telnet.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp-telnet.xml index c2496fd..4396efc 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp-telnet.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp-telnet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml index 6dc047e..3d17b58 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml index 7ccaeac..fc3d457 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml index b5ad413..14b54f1 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml b/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml index 7183e79..912df56 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-sound.xml b/tests/sexpr2xmldata/sexpr2xml-fv-sound.xml index 7183e79..912df56 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-sound.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-sound.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml b/tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml index ae90e33..19eac3b 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml b/tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml index f81c47a..40ac8a9 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-utc.xml b/tests/sexpr2xmldata/sexpr2xml-fv-utc.xml index c783d93..97f2beb 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-utc.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-utc.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-v2.xml b/tests/sexpr2xmldata/sexpr2xml-fv-v2.xml index bd3b107..493d1b5 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-v2.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-v2.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv.xml b/tests/sexpr2xmldata/sexpr2xml-fv.xml index c783d93..97f2beb 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml b/tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml index 00d18ce..a3cd7be 100644 --- a/tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml +++ b/tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/xmconfigdata/test-escape-paths.xml b/tests/xmconfigdata/test-escape-paths.xml index de3a7e2..623eaa1 100644 --- a/tests/xmconfigdata/test-escape-paths.xml +++ b/tests/xmconfigdata/test-escape-paths.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader&test</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader&test</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-force-hpet.xml b/tests/xmconfigdata/test-fullvirt-force-hpet.xml index 75f8724..57a6531 100644 --- a/tests/xmconfigdata/test-fullvirt-force-hpet.xml +++ b/tests/xmconfigdata/test-fullvirt-force-hpet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-force-nohpet.xml b/tests/xmconfigdata/test-fullvirt-force-nohpet.xml index e5741b6..f6ebcf6 100644 --- a/tests/xmconfigdata/test-fullvirt-force-nohpet.xml +++ b/tests/xmconfigdata/test-fullvirt-force-nohpet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-localtime.xml b/tests/xmconfigdata/test-fullvirt-localtime.xml index 8b97e5b..36ab389 100644 --- a/tests/xmconfigdata/test-fullvirt-localtime.xml +++ b/tests/xmconfigdata/test-fullvirt-localtime.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-net-ioemu.xml b/tests/xmconfigdata/test-fullvirt-net-ioemu.xml index f22c085..3618bae 100644 --- a/tests/xmconfigdata/test-fullvirt-net-ioemu.xml +++ b/tests/xmconfigdata/test-fullvirt-net-ioemu.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-net-netfront.xml b/tests/xmconfigdata/test-fullvirt-net-netfront.xml index 177bb6a..6a2a439 100644 --- a/tests/xmconfigdata/test-fullvirt-net-netfront.xml +++ b/tests/xmconfigdata/test-fullvirt-net-netfront.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-new-cdrom.xml b/tests/xmconfigdata/test-fullvirt-new-cdrom.xml index f22c085..3618bae 100644 --- a/tests/xmconfigdata/test-fullvirt-new-cdrom.xml +++ b/tests/xmconfigdata/test-fullvirt-new-cdrom.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-old-cdrom.xml b/tests/xmconfigdata/test-fullvirt-old-cdrom.xml index a592630..7d6014d 100644 --- a/tests/xmconfigdata/test-fullvirt-old-cdrom.xml +++ b/tests/xmconfigdata/test-fullvirt-old-cdrom.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-parallel-tcp.xml b/tests/xmconfigdata/test-fullvirt-parallel-tcp.xml index 738e5ab..9b1fd26 100644 --- a/tests/xmconfigdata/test-fullvirt-parallel-tcp.xml +++ b/tests/xmconfigdata/test-fullvirt-parallel-tcp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml index 753831a..a64d40b 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml index 1a55080..ce2cddb 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-file.xml b/tests/xmconfigdata/test-fullvirt-serial-file.xml index 0d2ac79..36883de 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-file.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-file.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-null.xml b/tests/xmconfigdata/test-fullvirt-serial-null.xml index d4b4ae9..982f9d6 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-null.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-null.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-pipe.xml b/tests/xmconfigdata/test-fullvirt-serial-pipe.xml index 6596dfc..82a1d9b 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-pipe.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-pipe.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-pty.xml b/tests/xmconfigdata/test-fullvirt-serial-pty.xml index 6c55abb..56ccbea 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-pty.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-pty.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-stdio.xml b/tests/xmconfigdata/test-fullvirt-serial-stdio.xml index 461f143..e2e9330 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-stdio.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-stdio.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-tcp-telnet.xml b/tests/xmconfigdata/test-fullvirt-serial-tcp-telnet.xml index d2fa7bf..d68d77c 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-tcp-telnet.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-tcp-telnet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-tcp.xml b/tests/xmconfigdata/test-fullvirt-serial-tcp.xml index 60ab8bd..aa3ed5c 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-tcp.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-tcp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-udp.xml b/tests/xmconfigdata/test-fullvirt-serial-udp.xml index 6c21cd2..256c722 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-udp.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-udp.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-serial-unix.xml b/tests/xmconfigdata/test-fullvirt-serial-unix.xml index f21534e..235c8d4 100644 --- a/tests/xmconfigdata/test-fullvirt-serial-unix.xml +++ b/tests/xmconfigdata/test-fullvirt-serial-unix.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-sound.xml b/tests/xmconfigdata/test-fullvirt-sound.xml index f09c16d..1429d10 100644 --- a/tests/xmconfigdata/test-fullvirt-sound.xml +++ b/tests/xmconfigdata/test-fullvirt-sound.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-usbmouse.xml b/tests/xmconfigdata/test-fullvirt-usbmouse.xml index 18a7ff0..25857f1 100644 --- a/tests/xmconfigdata/test-fullvirt-usbmouse.xml +++ b/tests/xmconfigdata/test-fullvirt-usbmouse.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-usbtablet.xml b/tests/xmconfigdata/test-fullvirt-usbtablet.xml index 5cbb007..31b1176 100644 --- a/tests/xmconfigdata/test-fullvirt-usbtablet.xml +++ b/tests/xmconfigdata/test-fullvirt-usbtablet.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-fullvirt-utc.xml b/tests/xmconfigdata/test-fullvirt-utc.xml index f22c085..3618bae 100644 --- a/tests/xmconfigdata/test-fullvirt-utc.xml +++ b/tests/xmconfigdata/test-fullvirt-utc.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='cdrom'/> </os> <features> diff --git a/tests/xmconfigdata/test-no-source-cdrom.xml b/tests/xmconfigdata/test-no-source-cdrom.xml index 2a457b2..74f1be1 100644 --- a/tests/xmconfigdata/test-no-source-cdrom.xml +++ b/tests/xmconfigdata/test-no-source-cdrom.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> diff --git a/tests/xmconfigdata/test-pci-devs.xml b/tests/xmconfigdata/test-pci-devs.xml index f828056..1911734 100644 --- a/tests/xmconfigdata/test-pci-devs.xml +++ b/tests/xmconfigdata/test-pci-devs.xml @@ -6,7 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='i686' machine='xenfv'>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> <boot dev='hd'/> </os> <features> -- 1.8.5.5
On 08/21/14 10:50, Michal Privoznik wrote:
Up to now, users can configure BIOS via the <loader/> element. With the upcoming implementation of UEFI this is not enough as BIOS and UEFI are conceptually different. For instance, while BIOS is ROM, UEFI is programmable flash (although all writes to code section are denied). Therefore we need new attribute @type which will differentiate the two. Then, new attribute @readonly is introduced to reflect the fact that some images are RO.
Moreover, the OVMF (which is going to be used mostly), works in two modes: 1) Code and UEFI variable store is mixed in one file. 2) Code and UEFI variable store is separated in two files
The latter has advantage of updating the UEFI code without losing the configuration. However, in order to represent the latter case we need yet another XML element: <nvram/>. Currently, it has no additional attributes, it's just a bare element containing path to the variable store file.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 19 ++++- docs/schemas/domaincommon.rng | 21 ++++++ src/conf/domain_conf.c | 87 +++++++++++++++++++++- src/conf/domain_conf.h | 22 +++++- src/libvirt_private.syms | 3 + src/qemu/qemu_command.c | 5 +- src/security/virt-aa-helper.c | 4 +- src/vbox/vbox_common.c | 7 +- src/xenapi/xenapi_driver.c | 3 +- src/xenconfig/xen_common.c | 7 +- src/xenconfig/xen_sxpr.c | 16 ++-- tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml | 40 ++++++++++ .../qemuxml2xmlout-pci-bridge-many-disks.xml | 2 +- tests/qemuxml2xmltest.c | 2 + tests/sexpr2xmldata/sexpr2xml-fv-autoport.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-empty-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-hpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-force-nohpet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-legacy-vfb.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-localtime.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-ioemu.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-net-netfront.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-parallel-tcp.xml | 2 +- .../sexpr2xml-fv-serial-dev-2-ports.xml | 2 +- .../sexpr2xml-fv-serial-dev-2nd-port.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-file.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-null.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pipe.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-pty.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-stdio.xml | 2 +- .../sexpr2xml-fv-serial-tcp-telnet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-tcp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-udp.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-serial-unix.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound-all.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-sound.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbmouse.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-usbtablet.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-utc.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv-v2.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-fv.xml | 2 +- tests/sexpr2xmldata/sexpr2xml-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-escape-paths.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-hpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-force-nohpet.xml | 2 +- tests/xmconfigdata/test-fullvirt-localtime.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-ioemu.xml | 2 +- tests/xmconfigdata/test-fullvirt-net-netfront.xml | 2 +- tests/xmconfigdata/test-fullvirt-new-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-old-cdrom.xml | 2 +- tests/xmconfigdata/test-fullvirt-parallel-tcp.xml | 2 +- .../test-fullvirt-serial-dev-2-ports.xml | 2 +- .../test-fullvirt-serial-dev-2nd-port.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-file.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-null.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pipe.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-pty.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-stdio.xml | 2 +- .../test-fullvirt-serial-tcp-telnet.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-tcp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-udp.xml | 2 +- tests/xmconfigdata/test-fullvirt-serial-unix.xml | 2 +- tests/xmconfigdata/test-fullvirt-sound.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbmouse.xml | 2 +- tests/xmconfigdata/test-fullvirt-usbtablet.xml | 2 +- tests/xmconfigdata/test-fullvirt-utc.xml | 2 +- tests/xmconfigdata/test-no-source-cdrom.xml | 2 +- tests/xmconfigdata/test-pci-devs.xml | 2 +- 69 files changed, 269 insertions(+), 79 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.xml
Acked-by: Laszlo Ersek <lersek@redhat.com> (Please append this line to the commit message in any further reposts.)
On 08/21/2014 02:50 AM, Michal Privoznik wrote:
Up to now, users can configure BIOS via the <loader/> element. With the upcoming implementation of UEFI this is not enough as BIOS and UEFI are conceptually different. For instance, while BIOS is ROM, UEFI is programmable flash (although all writes to code section are denied). Therefore we need new attribute @type which will differentiate the two. Then, new attribute @readonly is introduced to reflect the fact that some images are RO.
Moreover, the OVMF (which is going to be used mostly), works in two modes: 1) Code and UEFI variable store is mixed in one file. 2) Code and UEFI variable store is separated in two files
The latter has advantage of updating the UEFI code without losing the configuration. However, in order to represent the latter case we need yet another XML element: <nvram/>. Currently, it has no additional attributes, it's just a bare element containing path to the variable store file.
+++ b/docs/formatdomain.html.in @@ -102,7 +102,8 @@ ... <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling.
<boot dev='hd'/> <boot dev='cdrom'/> <bootmenu enable='yes'/> @@ -129,7 +130,21 @@ used to assist the domain creation process. It is used by Xen fully virtualized domains as well as setting the QEMU BIOS file path for QEMU/KVM domains. <span class="since">Xen since 0.1.0, - QEMU/KVM since 0.9.12</span></dd> + QEMU/KVM since 0.9.12</span> Then, <span class="since">Since
s/Since/since/, because you are using it in the middle of a sentence
+ 1.2.8</span> it's possible for the element to have two + optional attributes: <code>readonly</code> (accepted values are + <code>on</code> and <code>off</code>) to reflect the fact that the + image should be writable or read-only.
Again, yes/no might be more consistent than on/off
The second attribute + <code>type</code> accepts values <code>rom</code> and + <code>pflash</code>. It tells the hypervisor where in the guest + memory the file should be mapped. For instance, if the loader + path points to an UEFI image, <code>type</code> should be + <code>pflash</code>.</dd> + <dt><code>nvram</code></dt> + <dd>Some UEFI firmwares may want to use a non-volatile memory to store + some variables. In the host, this is represented as a file and the + path to the file is stored in this element. <span class="since">Since + 1.2.8</span></dd>
Is this going to bite us in the future? What if we want to store the file on a networked device, such as via gluster or nbd? I'm wondering if: <nvram type='file'> <source file='/path/to/storage'/> </nvram> is a better representation, because that way, we could also do: <nvram type='network'> <source protocol='gluster'> ... <!-- all the stuff we do for <disk type='network'> --> </source> </nvram> Also, by reusing a virStorageSourcePtr to store the nvram location, rather than limiting to just a file name, we can ensure we do proper SELinux labeling of the file, and allow the user the ability to overwrite what label we would otherwise generate.
+++ b/docs/schemas/domaincommon.rng @@ -242,6 +242,27 @@ <interleave> <optional> <element name="loader"> + <optional> + <attribute name="readonly"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="type"> + <choice> + <value>rom</value> + <value>pflash</value> + </choice> + </attribute> + </optional> + <ref name="absFilePath"/> + </element> + </optional> + <optional> + <element name="nvram"> <ref name="absFilePath"/>
This matches your docs, whether or not we decide to change the design. And at any rate, the existing <loader> element is just as hard-coded to a local filename as your new nvram, so if we add type='file' support, we'd want it on both places at once, so probably fine to defer that to a later day if someone actually needs it. So for now, I'm okay living with the design of just a string (but would still like to see the on/off changed to yes/no), since it's better to get OVMF usage enabled than to wait for a more complicated virStorageSource solution.
+static int +virDomainLoaderDefParseXML(xmlNodePtr node, + virDomainLoaderDefPtr loader) +{ + int ret = -1; + char *readonly_str = NULL; + char *type_str = NULL; + + readonly_str = virXMLPropString(node, "readonly"); + type_str = virXMLPropString(node, "type"); + loader->path = (char *) xmlNodeGetContent(node); + + if (readonly_str && + (loader->readonly = virTristateSwitchTypeFromString(readonly_str)) <= 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown readonly value: %s"), readonly_str);
Oh, so the on/off decision was because we reused an existing common function! In that case, in the .rng, I think it would be nice to replace: <attribute name="readonly"> <choice> <value>on</value> <value>off</value> </choice> </attribute> with <attribute name="readonly"> <ref name="tristateSwitch"/> </attribute> I didn't check if we already have a define for on/off, and I'm fairly certain that if we do, it's not named tristateSwitch. But my point is that it would be nice to have ALL on/off clients go through the same define, instead of open-coding it. That way, if we ever change our minds and allow virTristateSwitchTypeFromString to handle synonyms, we only have to update one spot in the rng to accept both spelling variants of "on". On the other hand, doing that cleanup is a separate patch, if we are going to convert all existing tri-state users to reuse a common define.
- def->os.loader = virXPathString("string(./os/loader[1])", ctxt); + if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { + if (VIR_ALLOC(def->os.loader) < 0) + goto error; + + if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) + goto error; + + def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
Ah - evidence that you only parse <nvram> if <loader> was present; and an argument in favor of my suggestion to make nvram a child rather than sibling. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 22, 2014 at 10:41:13AM -0600, Eric Blake wrote:
On 08/21/2014 02:50 AM, Michal Privoznik wrote:
Up to now, users can configure BIOS via the <loader/> element. With the upcoming implementation of UEFI this is not enough as BIOS and UEFI are conceptually different. For instance, while BIOS is ROM, UEFI is programmable flash (although all writes to code section are denied). Therefore we need new attribute @type which will differentiate the two. Then, new attribute @readonly is introduced to reflect the fact that some images are RO.
Moreover, the OVMF (which is going to be used mostly), works in two modes: 1) Code and UEFI variable store is mixed in one file. 2) Code and UEFI variable store is separated in two files
The latter has advantage of updating the UEFI code without losing the configuration. However, in order to represent the latter case we need yet another XML element: <nvram/>. Currently, it has no additional attributes, it's just a bare element containing path to the variable store file.
+++ b/docs/formatdomain.html.in @@ -102,7 +102,8 @@ ... <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling.
<loader> is a long standing element whose contents is a string path. So from a back compatibility POV we can't make <nvram> be a child of that, even though it would make sense. 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 :|
On 08/22/2014 10:43 AM, Daniel P. Berrange wrote:
<os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling.
<loader> is a long standing element whose contents is a string path. So from a back compatibility POV we can't make <nvram> be a child of that, even though it would make sense.
Hmm. But what if we allow a choice between: <loader type='rom'>/path/to/rom</loader> and <loader type='pflash'> <config>/path/to/config</config> <nvram>/path/to/nvram</nvram> </loader> that is, use the (optional) type='rom|pflash' for gating whether the rest of the <loader> element is a single name (old style) or structured layout (new style)? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Aug 22, 2014 at 10:51:18AM -0600, Eric Blake wrote:
On 08/22/2014 10:43 AM, Daniel P. Berrange wrote:
<os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling.
<loader> is a long standing element whose contents is a string path. So from a back compatibility POV we can't make <nvram> be a child of that, even though it would make sense.
Hmm. But what if we allow a choice between:
<loader type='rom'>/path/to/rom</loader>
and
<loader type='pflash'> <config>/path/to/config</config> <nvram>/path/to/nvram</nvram> </loader>
that is, use the (optional) type='rom|pflash' for gating whether the rest of the <loader> element is a single name (old style) or structured layout (new style)?
That is still going to cause existing apps which are parsing the XML to fail due to the change in child content. Changing content from a text node to elements is something I don't think we can do with the XML schemas. 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 :|
On 22.08.2014 18:41, Eric Blake wrote:
On 08/21/2014 02:50 AM, Michal Privoznik wrote:
Up to now, users can configure BIOS via the <loader/> element. With the upcoming implementation of UEFI this is not enough as BIOS and UEFI are conceptually different. For instance, while BIOS is ROM, UEFI is programmable flash (although all writes to code section are denied). Therefore we need new attribute @type which will differentiate the two. Then, new attribute @readonly is introduced to reflect the fact that some images are RO.
Moreover, the OVMF (which is going to be used mostly), works in two modes: 1) Code and UEFI variable store is mixed in one file. 2) Code and UEFI variable store is separated in two files
The latter has advantage of updating the UEFI code without losing the configuration. However, in order to represent the latter case we need yet another XML element: <nvram/>. Currently, it has no additional attributes, it's just a bare element containing path to the variable store file.
+++ b/docs/formatdomain.html.in @@ -102,7 +102,8 @@ ... <os> <type>hvm</type> - <loader>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
I didn't know that. Everything I found was just a bare <readonly/> element. But okay, I'm fine with 'yes|no' too.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling.
<boot dev='hd'/> <boot dev='cdrom'/> <bootmenu enable='yes'/> @@ -129,7 +130,21 @@ used to assist the domain creation process. It is used by Xen fully virtualized domains as well as setting the QEMU BIOS file path for QEMU/KVM domains. <span class="since">Xen since 0.1.0, - QEMU/KVM since 0.9.12</span></dd> + QEMU/KVM since 0.9.12</span> Then, <span class="since">Since
s/Since/since/, because you are using it in the middle of a sentence
+ 1.2.8</span> it's possible for the element to have two + optional attributes: <code>readonly</code> (accepted values are + <code>on</code> and <code>off</code>) to reflect the fact that the + image should be writable or read-only.
Again, yes/no might be more consistent than on/off
The second attribute + <code>type</code> accepts values <code>rom</code> and + <code>pflash</code>. It tells the hypervisor where in the guest + memory the file should be mapped. For instance, if the loader + path points to an UEFI image, <code>type</code> should be + <code>pflash</code>.</dd> + <dt><code>nvram</code></dt> + <dd>Some UEFI firmwares may want to use a non-volatile memory to store + some variables. In the host, this is represented as a file and the + path to the file is stored in this element. <span class="since">Since + 1.2.8</span></dd>
Is this going to bite us in the future? What if we want to store the file on a networked device, such as via gluster or nbd? I'm wondering if:
<nvram type='file'> <source file='/path/to/storage'/> </nvram>
is a better representation, because that way, we could also do:
<nvram type='network'> <source protocol='gluster'> ... <!-- all the stuff we do for <disk type='network'> --> </source> </nvram>
Who would ever want to store UEFI image/varstore on gluster? It's 2M in total after all from which the nvram is 128KiB.
Also, by reusing a virStorageSourcePtr to store the nvram location, rather than limiting to just a file name, we can ensure we do proper SELinux labeling of the file, and allow the user the ability to overwrite what label we would otherwise generate.
It doesn't make much sense to me to have different security label on the nvram file than the qemu process is going to have. Thus I don't think we should even allow arbitrary labels here.
+++ b/docs/schemas/domaincommon.rng @@ -242,6 +242,27 @@ <interleave> <optional> <element name="loader"> + <optional> + <attribute name="readonly"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="type"> + <choice> + <value>rom</value> + <value>pflash</value> + </choice> + </attribute> + </optional> + <ref name="absFilePath"/> + </element> + </optional> + <optional> + <element name="nvram"> <ref name="absFilePath"/>
This matches your docs, whether or not we decide to change the design. And at any rate, the existing <loader> element is just as hard-coded to a local filename as your new nvram, so if we add type='file' support, we'd want it on both places at once, so probably fine to defer that to a later day if someone actually needs it. So for now, I'm okay living with the design of just a string (but would still like to see the on/off changed to yes/no), since it's better to get OVMF usage enabled than to wait for a more complicated virStorageSource solution.
+static int +virDomainLoaderDefParseXML(xmlNodePtr node, + virDomainLoaderDefPtr loader) +{ + int ret = -1; + char *readonly_str = NULL; + char *type_str = NULL; + + readonly_str = virXMLPropString(node, "readonly"); + type_str = virXMLPropString(node, "type"); + loader->path = (char *) xmlNodeGetContent(node); + + if (readonly_str && + (loader->readonly = virTristateSwitchTypeFromString(readonly_str)) <= 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("unknown readonly value: %s"), readonly_str);
Oh, so the on/off decision was because we reused an existing common function! In that case, in the .rng, I think it would be nice to replace:
<attribute name="readonly"> <choice> <value>on</value> <value>off</value> </choice> </attribute>
with
<attribute name="readonly"> <ref name="tristateSwitch"/> </attribute>
I didn't check if we already have a define for on/off, and I'm fairly certain that if we do, it's not named tristateSwitch. But my point is that it would be nice to have ALL on/off clients go through the same define, instead of open-coding it. That way, if we ever change our minds and allow virTristateSwitchTypeFromString to handle synonyms, we only have to update one spot in the rng to accept both spelling variants of "on". On the other hand, doing that cleanup is a separate patch, if we are going to convert all existing tri-state users to reuse a common define.
Sure, but that would be a much greater cleanup. I think if this concrete patch shows something, then it is that we should start treating "on|off" and "yes|no" as synonyms. Yet again, that's different story that can be patched later.
- def->os.loader = virXPathString("string(./os/loader[1])", ctxt); + if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { + if (VIR_ALLOC(def->os.loader) < 0) + goto error; + + if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) + goto error; + + def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
Ah - evidence that you only parse <nvram> if <loader> was present; and an argument in favor of my suggestion to make nvram a child rather than sibling.
Michal
QEMU now supports UEFI with the following command line: -drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \ where the first line reflects <loader> and the second one <nvram>. Moreover, these two lines obsoletes the -bios argument. Note that UEFI is unusable without ACPI. This is handled properly now. Among with this extension, the variable file is expected to be writable and hence we need security drivers to label it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 94 +++++++++++++++++++++- src/security/security_dac.c | 8 ++ src/security/security_selinux.c | 8 ++ .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args | 10 +++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d03c4d5..2a6eee5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7282,6 +7282,94 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd, return 0; } +static int +qemuBuilDomainLoaderCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + virDomainLoaderDefPtr loader = def->os.loader; + virBuffer buf = VIR_BUFFER_INITIALIZER; + int unit = 0; + + if (!loader) + return 0; + + switch ((virDomainLoader) loader->type) { + case VIR_DOMAIN_LOADER_TYPE_ROM: + virCommandAddArg(cmd, "-bios"); + virCommandAddArg(cmd, loader->path); + break; + + case VIR_DOMAIN_LOADER_TYPE_PFLASH: + /* UEFI is supported only for x86_64 currently */ + if (def->os.arch != VIR_ARCH_X86_64) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pflash is not supported for %s guest achitecture"), + virArchToString(def->os.arch)); + goto cleanup; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support -drive")); + goto cleanup; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support passing " + "drive format")); + goto cleanup; + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI) && + def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ACPI must be enabled in order to use UEFI")); + goto cleanup; + } + + virBufferAsprintf(&buf, + "file=%s,if=pflash,format=raw,unit=%d", + loader->path, unit); + unit++; + + if (loader->readonly) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support passing " + "readonly attribute")); + goto cleanup; + } + + virBufferAsprintf(&buf, ",readonly=%s", + virTristateSwitchTypeToString(loader->readonly)); + } + + virCommandAddArg(cmd, "-drive"); + virCommandAddArgBuffer(cmd, &buf); + + if (loader->nvram) { + virBufferFreeAndReset(&buf); + virBufferAsprintf(&buf, + "file=%s,if=pflash,format=raw,unit=%d", + loader->nvram, unit); + + virCommandAddArg(cmd, "-drive"); + virCommandAddArgBuffer(cmd, &buf); + } + break; + + case VIR_DOMAIN_LOADER_TYPE_LAST: + /* nada */ + break; + } + + ret = 0; + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + qemuBuildCommandLineCallbacks buildCommandLineCallbacks = { .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName, }; @@ -7437,10 +7525,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-enable-nesting"); } - if (def->os.loader) { - virCommandAddArg(cmd, "-bios"); - virCommandAddArg(cmd, def->os.loader->path); - } + if (qemuBuilDomainLoaderCommandLine(cmd, def, qemuCaps) < 0) + goto error; /* Set '-m MB' based on maxmem, because the lower 'memory' limit * is set post-startup using the balloon driver. If balloon driver diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e62828e..e398d2c 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -960,6 +960,10 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, rc = -1; } + if (def->os.loader && def->os.loader->nvram && + virSecurityDACRestoreSecurityFileLabel(def->os.loader->nvram) < 0) + rc = -1; + if (def->os.kernel && virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) rc = -1; @@ -1036,6 +1040,10 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group)) return -1; + if (def->os.loader && def->os.loader->nvram && + virSecurityDACSetOwnership(def->os.loader->nvram, user, group) < 0) + return -1; + if (def->os.kernel && virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) return -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5d18493..0b480c3 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1911,6 +1911,10 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) rc = -1; + if (def->os.loader && def->os.loader->nvram && + virSecuritySELinuxRestoreSecurityFileLabel(mgr, def->os.loader->nvram) < 0) + rc = -1; + if (def->os.kernel && virSecuritySELinuxRestoreSecurityFileLabel(mgr, def->os.kernel) < 0) rc = -1; @@ -2294,6 +2298,10 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, mgr) < 0) return -1; + if (def->os.loader && def->os.loader->nvram && + virSecuritySELinuxSetFilecon(def->os.loader->nvram, data->content_context) < 0) + return -1; + if (def->os.kernel && virSecuritySELinuxSetFilecon(def->os.kernel, data->content_context) < 0) return -1; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args new file mode 100644 index 0000000..b51e8f3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc \ +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ +-drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \ +-m 1024 -smp 1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -boot c -usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,format=raw \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-serial pty -device usb-tablet,id=input0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 65dc9c7..bce0655 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -637,6 +637,8 @@ mymain(void) DO_TEST_FAILURE("reboot-timeout-enabled", NONE); DO_TEST("bios", QEMU_CAPS_DEVICE, QEMU_CAPS_SGA); + DO_TEST("bios-nvram", QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, + QEMU_CAPS_DRIVE_FORMAT, QEMU_CAPS_DRIVE_READONLY); DO_TEST("clock-utc", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE); DO_TEST("clock-localtime", NONE); DO_TEST("clock-localtime-basis-localtime", QEMU_CAPS_RTC); -- 1.8.5.5
On 08/21/14 10:50, Michal Privoznik wrote:
QEMU now supports UEFI with the following command line:
-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \
where the first line reflects <loader> and the second one <nvram>. Moreover, these two lines obsoletes the -bios argument.
Note that UEFI is unusable without ACPI. This is handled properly now. Among with this extension, the variable file is expected to be writable and hence we need security drivers to label it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 94 +++++++++++++++++++++- src/security/security_dac.c | 8 ++ src/security/security_selinux.c | 8 ++ .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args | 10 +++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args
Acked-by: Laszlo Ersek <lersek@redhat.com> (Please append this line to the commit message in any further reposts.)
On 08/21/2014 02:50 AM, Michal Privoznik wrote:
QEMU now supports UEFI with the following command line:
-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \
where the first line reflects <loader> and the second one <nvram>. Moreover, these two lines obsoletes the -bios argument.
s/obsoletes/obsolete/
Note that UEFI is unusable without ACPI. This is handled properly now. Among with this extension, the variable file is expected to be writable and hence we need security drivers to label it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+ case VIR_DOMAIN_LOADER_TYPE_PFLASH: + /* UEFI is supported only for x86_64 currently */ + if (def->os.arch != VIR_ARCH_X86_64) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pflash is not supported for %s guest achitecture"),
s/achitecture/architecture/
+ + if (loader->readonly) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support passing " + "readonly attribute")); + goto cleanup; + } + + virBufferAsprintf(&buf, ",readonly=%s", + virTristateSwitchTypeToString(loader->readonly)); + } + + virCommandAddArg(cmd, "-drive"); + virCommandAddArgBuffer(cmd, &buf); + + if (loader->nvram) { + virBufferFreeAndReset(&buf); + virBufferAsprintf(&buf, + "file=%s,if=pflash,format=raw,unit=%d", + loader->nvram, unit);
Hmm. Here, it looks like readonly='on' is supported ONLY for type='pflash', and not for type='rom'. In that case, I'd write the .rng of patch 1 as (rough draft): <element name='loader'> <choice> <group> <!-- bios, default loader type --> <optional> <attribute name='type'> <value>rom</value> </attribute> </optional> </group> <group> <!-- pflash, for OVMF use --> <attribute name='type'> <value>pflash</value> </attribute> <optional> <attribute name='readonly'...> </optional> <optional> nvram stuff... </optional> </group> </choice> <ref name='absFileName'> </element> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 22.08.2014 18:48, Eric Blake wrote:
On 08/21/2014 02:50 AM, Michal Privoznik wrote:
QEMU now supports UEFI with the following command line:
-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ -drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \
where the first line reflects <loader> and the second one <nvram>. Moreover, these two lines obsoletes the -bios argument.
s/obsoletes/obsolete/
Note that UEFI is unusable without ACPI. This is handled properly now. Among with this extension, the variable file is expected to be writable and hence we need security drivers to label it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
+ case VIR_DOMAIN_LOADER_TYPE_PFLASH: + /* UEFI is supported only for x86_64 currently */ + if (def->os.arch != VIR_ARCH_X86_64) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pflash is not supported for %s guest achitecture"),
s/achitecture/architecture/
+ + if (loader->readonly) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support passing " + "readonly attribute")); + goto cleanup; + } + + virBufferAsprintf(&buf, ",readonly=%s", + virTristateSwitchTypeToString(loader->readonly)); + } + + virCommandAddArg(cmd, "-drive"); + virCommandAddArgBuffer(cmd, &buf); + + if (loader->nvram) { + virBufferFreeAndReset(&buf); + virBufferAsprintf(&buf, + "file=%s,if=pflash,format=raw,unit=%d", + loader->nvram, unit);
Hmm. Here, it looks like readonly='on' is supported ONLY for type='pflash', and not for type='rom'. In that case, I'd write the .rng of patch 1 as (rough draft):
<element name='loader'> <choice> <group> <!-- bios, default loader type --> <optional> <attribute name='type'> <value>rom</value> </attribute> </optional> </group> <group> <!-- pflash, for OVMF use --> <attribute name='type'> <value>pflash</value> </attribute> <optional> <attribute name='readonly'...> </optional> <optional> nvram stuff... </optional> </group> </choice> <ref name='absFileName'> </element>
While this can be correct I think having wider XML schema then the code is just okay. Keeping the schema readable wins over tightening all the narrow cases for me. Michal
When using split UEFI image, it may come handy if libvirt manages per domain _VARS file automatically. While the _CODE file is RO and can be shared among multiple domains, you certainly don't want to do that on the _VARS file. This latter one needs to be per domain. So at the domain startup process, if it's determined that domain needs _VARS file it's copied from this master _VARS file. The location of the master file is configurable in qemu.conf. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 + src/Makefile.am | 1 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 14 ++++ src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 ++ src/qemu/qemu_process.c | 132 +++++++++++++++++++++++++++++++++++++ src/qemu/test_libvirtd_qemu.aug.in | 3 + 8 files changed, 253 insertions(+) diff --git a/libvirt.spec.in b/libvirt.spec.in index f491de7..762b404 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1948,6 +1948,7 @@ exit 0 %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/ +%dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/ %{_datadir}/augeas/lenses/libvirtd_qemu.aug %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug @@ -2050,6 +2051,7 @@ exit 0 %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/channel/target/ +%dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/nvram/ %dir %attr(0750, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/ %{_datadir}/augeas/lenses/libvirtd_qemu.aug %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug diff --git a/src/Makefile.am b/src/Makefile.am index 538530e..dc34dd9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2644,6 +2644,7 @@ endif WITH_SANLOCK if WITH_QEMU $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu/channel/target" + $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu/nvram" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/log/libvirt/qemu" diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e7db7fe..62951da 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -88,6 +88,8 @@ module Libvirtd_qemu = let log_entry = bool_entry "log_timestamp" + let nvram_entry = str_array_entry "nvram" + (* Each entry in the config is one of the following ... *) let entry = vnc_entry | spice_entry @@ -100,6 +102,7 @@ module Libvirtd_qemu = | rpc_entry | network_entry | log_entry + | nvram_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] 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" ] diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 25e6d5e..6409d3a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -107,6 +107,9 @@ void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def) VIR_FREE(def); } +#define VIR_QEMU_LOADER_FILE_PATH "/usr/share/OVMF/OVMF_CODE.fd" +#define VIR_QEMU_NVRAM_FILE_PATH "/usr/share/OVMF/OVMF_VARS.fd" + virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) { virQEMUDriverConfigPtr cfg; @@ -255,6 +258,15 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->logTimestamp = true; + if (VIR_ALLOC_N(cfg->loader, 1) < 0 || + VIR_ALLOC_N(cfg->nvram, 1) < 0) + goto error; + cfg->nloader = 1; + + if (VIR_STRDUP(cfg->loader[0], VIR_QEMU_LOADER_FILE_PATH) < 0 || + VIR_STRDUP(cfg->nvram[0], VIR_QEMU_NVRAM_FILE_PATH) < 0) + goto error; + return cfg; error: @@ -305,6 +317,14 @@ static void virQEMUDriverConfigDispose(void *obj) virStringFreeList(cfg->securityDriverNames); VIR_FREE(cfg->lockManagerName); + + while (cfg->nloader) { + VIR_FREE(cfg->loader[cfg->nloader - 1]); + VIR_FREE(cfg->nvram[cfg->nloader - 1]); + cfg->nloader--; + } + VIR_FREE(cfg->loader); + VIR_FREE(cfg->nvram); } @@ -328,6 +348,43 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigNVRAMParse(const char *str, + char **loader, + char **nvram) +{ + int ret = -1; + char **token; + + if (!(token = virStringSplit(str, ":", 0))) + goto cleanup; + + if (token[0]) { + virSkipSpaces((const char **) &token[0]); + if (token[1]) + virSkipSpaces((const char **) &token[1]); + } + + /* Exactly two tokens are expected */ + if (!token[0] || !token[1] || token[2] || + STREQ(token[0], "") || STREQ(token[1], "")) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Invalid nvram format: '%s'"), + str); + goto cleanup; + } + + if (VIR_STRDUP(*loader, token[0]) < 0 || + VIR_STRDUP(*nvram, token[1]) < 0) + goto cleanup; + + ret = 0; + cleanup: + virStringFreeList(token); + return ret; +} + + int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename) { @@ -654,6 +711,42 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); + CHECK_TYPE("nvram", VIR_CONF_LIST); + if ((p = virConfGetValue(conf, "nvram"))) { + size_t len; + virConfValuePtr pp; + + while (cfg->nloader) { + VIR_FREE(cfg->loader[cfg->nloader - 1]); + VIR_FREE(cfg->nvram[cfg->nloader - 1]); + cfg->nloader--; + } + VIR_FREE(cfg->loader); + VIR_FREE(cfg->nvram); + + /* Calc length and check items */ + for (len = 0, pp = p->list; pp; len++, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("nvram must be a list of strings")); + goto cleanup; + } + } + + if (len && + (VIR_ALLOC_N(cfg->loader, len) < 0 || + VIR_ALLOC_N(cfg->nvram, len) < 0)) + goto cleanup; + cfg->nloader = len; + + for (i = 0, pp = p->list; pp; i++, pp = pp->next) { + if (virQEMUDriverConfigNVRAMParse(pp->str, + &cfg->loader[i], + &cfg->nvram[i]) < 0) + goto cleanup; + } + } + ret = 0; cleanup: diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ae7ac56..1f521e5 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -172,6 +172,11 @@ struct _virQEMUDriverConfig { int migrationPortMax; bool logTimestamp; + + /* Pairs of loader:nvram paths. The list is @nloader items long */ + char **loader; + char **nvram; + size_t nloader; }; /* Main driver state */ 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; + + 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; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3802,6 +3927,13 @@ int qemuProcessStart(virConnectPtr conn, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + /* Some things, paths, ... are generated here and we want them to persist. + * Fill them in prior to setting the domain def as transient. */ + VIR_DEBUG("Generating paths"); + + if (qemuPrepareNVRAM(cfg, vm->def, migrateFrom) < 0) + goto cleanup; + /* Do this upfront, so any part of the startup process can add * runtime state to vm->def that won't be persisted. This let's us * report implicit runtime defaults in the XML, like vnc listen/socket diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 7796acc..d2bc2c0 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -74,3 +74,6 @@ module Test_libvirtd_qemu = { "migration_port_min" = "49152" } { "migration_port_max" = "49215" } { "log_timestamp" = "0" } +{ "nvram" + { "1" = "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd" } +} -- 1.8.5.5
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 :|
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
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). 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 :|
On 08/22/14 10:54, Daniel P. Berrange wrote:
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.
If the domain XML spells out both <loader> and <nvram>, then both should be updated manually by the user (if the VM's old nvram is not compatible with the new loader). This would include the user either instantiating the new varstore for the VM, or removing the <nvram> element (so that the new default template can take effect). If the domain XML doesn't spell out <nvram> (either genuinely, or because the user removed that element, see above), then yes, you need to edit /etc/libvirt/qemu.conf. I don't see a problem with that. You won't keep installing OVMF_CODE.fd files in random locations in the host filesystem. You might be developing OVMF and install various ad-hoc builds, but those would go to the same location (same pathname), hence it would have to be added only once to the qemu.conf 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.
That information would only be useful if the <nvram> element had the @template argument, but no text contents. Meaning for the VM, "create my personal varstore instance from this template if I lose my personal varstore instance".
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.
Not if you edit the nvram=[] map in /etc/libvirt/qemu.conf.
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.
If you include the varstore template *only* in the VM-specific domain XML file, then one very important use case will be broken: creating brand new VMs with UEFI firmware. Since the domain XML doesn't exist yet for the virtual machine, there's simply no place to look up the location of the varstore template in, even if you are trying to use the system-wide OVMF installation. Users will have to provide the pathname of the default varstore.
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.
It's qemu.conf, not libvirtd.conf, but you're right about the need to restart libvirtd. I don't see any problem with that, given that you will not install custom OVMFs in random locations on the host; you'll do that only occasionally. If you *do* install custom OVMFs in random locations, then you can instantiate the varstores as well.
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).
I can't provide any input on such architectures or firmwares. We can only extract general traits if we know all the specifics. If we want to go general in advance, how do we know for example whether one <nvram> element will suffice at all? Maybe some architecture (or firmware) that has (or handles) non-volatile RAM expects several writeable flash chips, mapped at various guest-phys addresses. The current format doesn't accommodate that either (and it shouldn't, until we actually see those architectures / firmwares). Anyway I don't insist. Thanks, Laszlo
On Fri, Aug 22, 2014 at 11:38:06AM +0200, Laszlo Ersek wrote:
On 08/22/14 10:54, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 10:27:29AM +0200, Laszlo Ersek wrote:
On 08/21/14 11:05, Daniel P. Berrange wrote:
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.
If the domain XML spells out both <loader> and <nvram>, then both should be updated manually by the user (if the VM's old nvram is not compatible with the new loader). This would include the user either instantiating the new varstore for the VM, or removing the <nvram> element (so that the new default template can take effect).
If the domain XML doesn't spell out <nvram> (either genuinely, or because the user removed that element, see above), then yes, you need to edit /etc/libvirt/qemu.conf.
I don't see a problem with that. You won't keep installing OVMF_CODE.fd files in random locations in the host filesystem. You might be developing OVMF and install various ad-hoc builds, but those would go to the same location (same pathname), hence it would have to be added only once to the qemu.conf file.
Well I do see a problem with editing qemu.conf for this, particularly when there is a very straightforward way to avoid that need which I have outlined here. It is crazy to force these extra hoops onto people 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 :|
On 08/22/14 11:56, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 11:38:06AM +0200, Laszlo Ersek wrote:
On 08/22/14 10:54, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 10:27:29AM +0200, Laszlo Ersek wrote:
On 08/21/14 11:05, Daniel P. Berrange wrote:
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.
If the domain XML spells out both <loader> and <nvram>, then both should be updated manually by the user (if the VM's old nvram is not compatible with the new loader). This would include the user either instantiating the new varstore for the VM, or removing the <nvram> element (so that the new default template can take effect).
If the domain XML doesn't spell out <nvram> (either genuinely, or because the user removed that element, see above), then yes, you need to edit /etc/libvirt/qemu.conf.
I don't see a problem with that. You won't keep installing OVMF_CODE.fd files in random locations in the host filesystem. You might be developing OVMF and install various ad-hoc builds, but those would go to the same location (same pathname), hence it would have to be added only once to the qemu.conf file.
Well I do see a problem with editing qemu.conf for this, particularly when there is a very straightforward way to avoid that need which I have outlined here. It is crazy to force these extra hoops onto people
OK. But, if you don't provide a default map in some central config file, for at least the system-wide OVMF installation(s), how do you save people from the exact same burden (== manual varstore instantiation), when they try to create a brand new UEFI virtual machine that uses one of the system-wide OVMF filesets? Libvirt won't know where to copy the varstore from. What you outlined is not straightforward at all for this case. Since the VM is just being created, it has no nvram/@template attribute (because the domain XML is being composed, it doesn't exist yet). The v4 patchset addresses the common case and allows the special use case to remain hard. As far as I understand, your proposal breaks the common use case (== the main goal of this patchset), namely that users can just say "I have all the necessary distro packages installed, now give me a new OVMF VM". Thanks Laszlo
On Fri, Aug 22, 2014 at 12:12:52PM +0200, Laszlo Ersek wrote:
On 08/22/14 11:56, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 11:38:06AM +0200, Laszlo Ersek wrote:
On 08/22/14 10:54, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 10:27:29AM +0200, Laszlo Ersek wrote:
On 08/21/14 11:05, Daniel P. Berrange wrote:
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.
If the domain XML spells out both <loader> and <nvram>, then both should be updated manually by the user (if the VM's old nvram is not compatible with the new loader). This would include the user either instantiating the new varstore for the VM, or removing the <nvram> element (so that the new default template can take effect).
If the domain XML doesn't spell out <nvram> (either genuinely, or because the user removed that element, see above), then yes, you need to edit /etc/libvirt/qemu.conf.
I don't see a problem with that. You won't keep installing OVMF_CODE.fd files in random locations in the host filesystem. You might be developing OVMF and install various ad-hoc builds, but those would go to the same location (same pathname), hence it would have to be added only once to the qemu.conf file.
Well I do see a problem with editing qemu.conf for this, particularly when there is a very straightforward way to avoid that need which I have outlined here. It is crazy to force these extra hoops onto people
OK.
But, if you don't provide a default map in some central config file, for at least the system-wide OVMF installation(s), how do you save people from the exact same burden (== manual varstore instantiation), when they try to create a brand new UEFI virtual machine that uses one of the system-wide OVMF filesets? Libvirt won't know where to copy the varstore from.
Having a default map in libvirt and in qemu.conf is still acceptable for the common case. I just want to make sure that if the user wants to provide a non-default BIOS in <loader> they can still get the NVRAM automatically created from a template, without having to edit qemu.conf and restart libvirtd. 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 :|
On 08/22/14 12:24, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 12:12:52PM +0200, Laszlo Ersek wrote:
On 08/22/14 11:56, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 11:38:06AM +0200, Laszlo Ersek wrote:
On 08/22/14 10:54, Daniel P. Berrange wrote:
On Fri, Aug 22, 2014 at 10:27:29AM +0200, Laszlo Ersek wrote:
On 08/21/14 11:05, Daniel P. Berrange wrote: > 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.
If the domain XML spells out both <loader> and <nvram>, then both should be updated manually by the user (if the VM's old nvram is not compatible with the new loader). This would include the user either instantiating the new varstore for the VM, or removing the <nvram> element (so that the new default template can take effect).
If the domain XML doesn't spell out <nvram> (either genuinely, or because the user removed that element, see above), then yes, you need to edit /etc/libvirt/qemu.conf.
I don't see a problem with that. You won't keep installing OVMF_CODE.fd files in random locations in the host filesystem. You might be developing OVMF and install various ad-hoc builds, but those would go to the same location (same pathname), hence it would have to be added only once to the qemu.conf file.
Well I do see a problem with editing qemu.conf for this, particularly when there is a very straightforward way to avoid that need which I have outlined here. It is crazy to force these extra hoops onto people
OK.
But, if you don't provide a default map in some central config file, for at least the system-wide OVMF installation(s), how do you save people from the exact same burden (== manual varstore instantiation), when they try to create a brand new UEFI virtual machine that uses one of the system-wide OVMF filesets? Libvirt won't know where to copy the varstore from.
Having a default map in libvirt and in qemu.conf is still acceptable for the common case. I just want to make sure that if the user wants to provide a non-default BIOS in <loader> they can still get the NVRAM automatically created from a template, without having to edit qemu.conf and restart libvirtd.
Michal, can you please incorporate this? I guess the priorities would be: - if <nvram> has text contents, just use that. (same as now) - else, if <nvram> has @template, copy from there. (new) - otherwise, consult the map. (same as now) Thanks, Laszlo
comments below On 08/21/14 10:50, Michal Privoznik wrote:
When using split UEFI image, it may come handy if libvirt manages per domain _VARS file automatically. While the _CODE file is RO and can be shared among multiple domains, you certainly don't want to do that on the _VARS file. This latter one needs to be per domain. So at the domain startup process, if it's determined that domain needs _VARS file it's copied from this master _VARS file. The location of the master file is configurable in qemu.conf.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- libvirt.spec.in | 2 + src/Makefile.am | 1 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 14 ++++ src/qemu/qemu_conf.c | 93 ++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 ++ src/qemu/qemu_process.c | 132 +++++++++++++++++++++++++++++++++++++ src/qemu/test_libvirtd_qemu.aug.in | 3 + 8 files changed, 253 insertions(+)
I compared this patch with its v2 counterpart. I can see that all of my remarks have been addressed. Two notes:
@@ -654,6 +711,42 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
+ CHECK_TYPE("nvram", VIR_CONF_LIST); + if ((p = virConfGetValue(conf, "nvram"))) { + size_t len; + virConfValuePtr pp; + + while (cfg->nloader) { + VIR_FREE(cfg->loader[cfg->nloader - 1]); + VIR_FREE(cfg->nvram[cfg->nloader - 1]); + cfg->nloader--; + } + VIR_FREE(cfg->loader); + VIR_FREE(cfg->nvram); + + /* Calc length and check items */ + for (len = 0, pp = p->list; pp; len++, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("nvram must be a list of strings")); + goto cleanup; + } + } + + if (len && + (VIR_ALLOC_N(cfg->loader, len) < 0 || + VIR_ALLOC_N(cfg->nvram, len) < 0)) + goto cleanup; + cfg->nloader = len; + + for (i = 0, pp = p->list; pp; i++, pp = pp->next) { + if (virQEMUDriverConfigNVRAMParse(pp->str, + &cfg->loader[i], + &cfg->nvram[i]) < 0) + goto cleanup; + } + } + ret = 0;
cleanup:
(a) In general, this looks like a good solution. This prevents freeing garbage pointers, and also handles the nvram=[] (empty list) case: nvram=[] will work as if nvram were absent completely. Okay. (b) However, I think CHECK_TYPE() is used incorrectly: 'p = virConfGetValue(conf, "nvram")' must be done *before* CHECK_TYPE(). You didn't catch this in testing because the value of "p", before you reach CHECK_TYPE(), has been set by p = virConfGetValue(conf, "hugetlbfs_mount"); That is, "p" was most likely NULL in your testing. And p == NULL short-circuits CHECK_TYPE(), to success. You need: p = virConfGetValue(conf, "nvram"); CHECK_TYPE("nvram", VIR_CONF_LIST); if (p) { Refer to the "cgroup_controllers" block in virQEMUDriverConfigLoadFile(), a little bit higher up; that one uses this same pattern. The rest seems fine to me. Thanks! Laszlo
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laszlo Ersek -
Michal Privoznik