Currently, libvirt allows only local filepaths to specify a NVRAM disk. Since, VMs can migrate across hosts, so making it to virStorageSource type would help in uninturrupted access NVRAM disks over network. Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- src/conf/domain_conf.c | 13 ++++++++++--- src/conf/domain_conf.h | 2 +- src/qemu/qemu_cgroup.c | 3 ++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain.c | 14 ++++++++------ src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_firmware.c | 17 ++++++++++++----- src/qemu/qemu_namespace.c | 5 +++-- src/qemu/qemu_process.c | 5 +++-- src/security/security_dac.c | 6 ++++-- src/security/security_selinux.c | 6 ++++-- src/security/virt-aa-helper.c | 5 +++-- src/vbox/vbox_common.c | 2 +- 13 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5dd269b283..b83c2f0e6a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3540,7 +3540,7 @@ virDomainLoaderDefFree(virDomainLoaderDef *loader) return; g_free(loader->path); - g_free(loader->nvram); + virObjectUnref(loader->nvram); g_free(loader->nvramTemplate); g_free(loader); } @@ -18266,7 +18266,11 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, fwAutoSelect) < 0) return -1; - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); + if (virXPathNode("./os/nvram[1]", ctxt)) { + def->os.loader->nvram = g_new0(virStorageSource, 1); + def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt); + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + } if (!fwAutoSelect) def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt); @@ -26971,7 +26975,10 @@ virDomainLoaderDefFormat(virBuffer *buf, virXMLFormatElementInternal(buf, "loader", &loaderAttrBuf, &loaderChildBuf, false, false); virBufferEscapeString(&nvramAttrBuf, " template='%s'", loader->nvramTemplate); - virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram); + if (loader->nvram) { + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) + virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path); + } virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 694491cd63..eecee1075c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2234,7 +2234,7 @@ struct _virDomainLoaderDef { virTristateBool readonly; virDomainLoader type; virTristateBool secure; - char *nvram; /* path to non-volatile RAM */ + virStorageSource *nvram; char *nvramTemplate; /* user override of path to master nvram */ }; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index aa0c927578..d46c9ff36a 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -581,7 +581,8 @@ qemuSetupFirmwareCgroup(virDomainObj *vm) return -1; if (vm->def->os.loader->nvram && - qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0) + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && + qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0) return -1; return 0; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bb45954108..1393d4fc22 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9648,7 +9648,7 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd, if (loader->nvram) { virBufferAddLit(&buf, "file="); - virQEMUBuildBufferEscapeComma(&buf, loader->nvram); + virQEMUBuildBufferEscapeComma(&buf, loader->nvram->path); virBufferAsprintf(&buf, ",if=pflash,format=raw,unit=%d", unit); virCommandAddArg(cmd, "-drive"); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a4334de158..70e96cc5a5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4663,8 +4663,12 @@ qemuDomainDefPostParse(virDomainDef *def, } if (virDomainDefHasOldStyleROUEFI(def) && - !def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + (!def->os.loader->nvram || !def->os.loader->nvram->path)) { + if (!def->os.loader->nvram) + def->os.loader->nvram = g_new0(virStorageSource, 1); + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + } if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0) return -1; @@ -11349,11 +11353,9 @@ qemuDomainInitializePflashStorageSource(virDomainObj *vm) if (def->os.loader->nvram) { - pflash1 = virStorageSourceNew(); - pflash1->type = VIR_STORAGE_TYPE_FILE; + if (!(pflash1 = virStorageSourceCopy(def->os.loader->nvram, false))) + return -1; pflash1->format = VIR_STORAGE_FILE_RAW; - pflash1->path = g_strdup(def->os.loader->nvram); - pflash1->readonly = false; pflash1->nodeformat = g_strdup("libvirt-pflash1-format"); pflash1->nodestorage = g_strdup("libvirt-pflash1-storage"); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c0e36e9b2..2b2bd8d20c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6615,8 +6615,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, } } - if (vm->def->os.loader && vm->def->os.loader->nvram) { - nvram_path = g_strdup(vm->def->os.loader->nvram); + if (vm->def->os.loader && vm->def->os.loader->nvram && + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) { + nvram_path = g_strdup(vm->def->os.loader->nvram->path); } else if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) { qemuDomainNVRAMPathFormat(cfg, vm->def, &nvram_path); } diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 51223faadf..6556a613a8 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1192,13 +1192,16 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, VIR_FREE(def->os.loader->nvramTemplate); def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename); - if (!def->os.loader->nvram) - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); + if (!def->os.loader->nvram) { + def->os.loader->nvram = g_new0(virStorageSource, 1); + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); + } VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", def->os.loader->path, def->os.loader->nvramTemplate, - def->os.loader->nvram); + def->os.loader->nvram->path); break; case QEMU_FIRMWARE_DEVICE_KERNEL: @@ -1364,8 +1367,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, * its path in domain XML) but no template for NVRAM was * specified and the varstore doesn't exist ... */ if (!virDomainDefHasOldStyleROUEFI(def) || - def->os.loader->nvramTemplate || - (!reset_nvram && virFileExists(def->os.loader->nvram))) + def->os.loader->nvramTemplate) + return 0; + + if (!reset_nvram && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE + && virFileExists(def->os.loader->nvram->path)) return 0; /* ... then we want to consult JSON FW descriptors first, diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 23681b14a4..18a24635ad 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -572,8 +572,9 @@ qemuDomainSetupLoader(virDomainObj *vm, case VIR_DOMAIN_LOADER_TYPE_PFLASH: *paths = g_slist_prepend(*paths, g_strdup(loader->path)); - if (loader->nvram) - *paths = g_slist_prepend(*paths, g_strdup(loader->nvram)); + if (loader->nvram && + loader->nvram->type == VIR_STORAGE_TYPE_FILE) + *paths = g_slist_prepend(*paths, g_strdup(loader->nvram->path)); break; case VIR_DOMAIN_LOADER_TYPE_NONE: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2e11d24be2..e53a26c6fd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4472,7 +4472,8 @@ qemuPrepareNVRAM(virQEMUDriver *driver, struct qemuPrepareNVRAMHelperData data; if (!loader || !loader->nvram || - (virFileExists(loader->nvram) && !reset_nvram)) + loader->nvram->type != VIR_STORAGE_TYPE_FILE || + (virFileExists(loader->nvram->path) && !reset_nvram)) return 0; master_nvram_path = loader->nvramTemplate; @@ -4504,7 +4505,7 @@ qemuPrepareNVRAM(virQEMUDriver *driver, data.srcFD = srcFD; data.srcPath = master_nvram_path; - if (virFileRewrite(loader->nvram, + if (virFileRewrite(loader->nvram->path, S_IRUSR | S_IWUSR, cfg->user, cfg->group, qemuPrepareNVRAMHelper, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e9e316551e..66c36c57a3 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1971,7 +1971,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && + virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0) rc = -1; if (def->os.kernel && @@ -2182,8 +2183,9 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && virSecurityDACSetOwnership(mgr, NULL, - def->os.loader->nvram, + def->os.loader->nvram->path, user, group, true) < 0) return -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6f02baf2ce..1b6b67e8c7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2806,7 +2806,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && + virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path, true) < 0) rc = -1; if (def->os.kernel && @@ -3212,8 +3213,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, /* This is different than kernel or initrd. The nvram store * is really a disk, qemu can read and write to it. */ if (def->os.loader && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && secdef && secdef->imagelabel && - virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, + virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path, secdef->imagelabel, true) < 0) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1f1cce8b3d..d4eb8b08c9 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1006,8 +1006,9 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) goto cleanup; - if (ctl->def->os.loader && ctl->def->os.loader->nvram) - if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->nvram && + ctl->def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE) + if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 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 34e555644c..214d5e84b8 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -992,7 +992,7 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data, 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.loader->nvram->path %s", def->os.loader->nvram->path); } VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); -- 2.25.1

Thanks for the review, Peter! Can you please look at patch 2nd of this patch series too ? It will be easier if I make all the necessary changes, if any, at once. On 20/04/22 6:18 pm, Peter Krempa wrote: Thanks, I will update it. Ack. Ack. Thanks! Ack. Ack. Ack. Ack. Ack. Sure, Ack. Ack. Ack. Ack. Ack. Ack. Ack. Ack. Right, I will add the check for def->os.loader->nvram existance, before accessing nvram->path. Thanks!

This patch introduces the logic to support remote NVRAM and adds 'type' attribute to nvram element. Sample XML with new annotation: <nvram type='network'> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> <host name='example.com' port='6000'/> </source> </nvram> or <nvram type='file'> <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/> </nvram> Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_firmware.c | 6 +++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b83c2f0e6a..bc8c7e0d6f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17866,6 +17866,42 @@ virDomainLoaderDefParseXML(xmlNodePtr node, } +static int +virDomainNvramDefParseXML(virStorageSource *nvram, + xmlXPathContextPtr ctxt, + virDomainXMLOption *opt, + unsigned int flags) +{ + g_autofree char *nvramType = NULL; + xmlNodePtr source; + + nvramType = virXPathString("string(./os/nvram/@type)", ctxt); + + /* if nvramType==NULL read old-style, else + * if there's a type, read new style */ + if (!nvramType) { + nvram->type = VIR_STORAGE_TYPE_FILE; + nvram->path = virXPathString("string(./os/nvram[1])", ctxt); + return 0; + } else { + if ((nvram->type = virStorageTypeFromString(nvramType)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown disk type '%s'"), nvramType); + return -1; + } + source = virXPathNode("./os/nvram/source[1]", ctxt); + if (!source) { + virReportError(VIR_ERR_XML_ERROR, + _("Missing source element with nvram type '%s'"), + nvramType); + return -1; + } + return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt); + } + return 0; +} + + static int virDomainSchedulerParseCommonAttrs(xmlNodePtr node, virProcessSchedPolicy *policy, @@ -18251,7 +18287,9 @@ virDomainDefParseBootFirmwareOptions(virDomainDef *def, static int virDomainDefParseBootLoaderOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + virDomainXMLOption *xmlopt, + unsigned int flags) { xmlNodePtr loader_node = virXPathNode("./os/loader[1]", ctxt); const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE; @@ -18268,8 +18306,10 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, if (virXPathNode("./os/nvram[1]", ctxt)) { def->os.loader->nvram = g_new0(virStorageSource, 1); - def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt); - def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; + + if (virDomainNvramDefParseXML(def->os.loader->nvram, + ctxt, xmlopt, flags) < 0) + return -1; } if (!fwAutoSelect) def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt); @@ -18324,7 +18364,9 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def, static int virDomainDefParseBootOptions(virDomainDef *def, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + virDomainXMLOption *xmlopt, + unsigned int flags) { /* * Booting options for different OS types.... @@ -18342,7 +18384,7 @@ virDomainDefParseBootOptions(virDomainDef *def, if (virDomainDefParseBootFirmwareOptions(def, ctxt) < 0) return -1; - if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0) return -1; if (virDomainDefParseBootAcpiOptions(def, ctxt) < 0) @@ -18358,7 +18400,7 @@ virDomainDefParseBootOptions(virDomainDef *def, case VIR_DOMAIN_OSTYPE_UML: virDomainDefParseBootKernelOptions(def, ctxt); - if (virDomainDefParseBootLoaderOptions(def, ctxt) < 0) + if (virDomainDefParseBootLoaderOptions(def, ctxt, xmlopt, flags) < 0) return -1; break; @@ -19649,7 +19691,7 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, if (virDomainDefClockParse(def, ctxt) < 0) return NULL; - if (virDomainDefParseBootOptions(def, ctxt) < 0) + if (virDomainDefParseBootOptions(def, ctxt, xmlopt, flags) < 0) return NULL; /* analysis of the disk devices */ @@ -26949,9 +26991,11 @@ virDomainHugepagesFormat(virBuffer *buf, virBufferAddLit(buf, "</hugepages>\n"); } -static void +static int virDomainLoaderDefFormat(virBuffer *buf, - virDomainLoaderDef *loader) + virDomainLoaderDef *loader, + virDomainXMLOption *xmlopt, + unsigned int flags) { g_auto(virBuffer) loaderAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) loaderChildBuf = VIR_BUFFER_INITIALIZER; @@ -26976,10 +27020,20 @@ virDomainLoaderDefFormat(virBuffer *buf, virBufferEscapeString(&nvramAttrBuf, " template='%s'", loader->nvramTemplate); if (loader->nvram) { - if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) { virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path); + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false); + } else { + virBufferEscapeString(&nvramAttrBuf, " type='%s'", virStorageTypeToString(loader->nvram->type)); + if (virDomainDiskSourceFormat(&nvramChildBuf, loader->nvram, "source", 0, + 0, false, flags, true, xmlopt) < 0) { + return -1; + } + virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, true); + } } - virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false); + + return 0; } static void @@ -28180,8 +28234,9 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, if (def->os.initgroup) virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup); - if (def->os.loader) - virDomainLoaderDefFormat(buf, def->os.loader); + if (def->os.loader && + virDomainLoaderDefFormat(buf, def->os.loader, xmlopt, flags) < 0) + return -1; virBufferEscapeString(buf, "<kernel>%s</kernel>\n", def->os.kernel); virBufferEscapeString(buf, "<initrd>%s</initrd>\n", diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 6556a613a8..22ad7d42a1 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1375,6 +1375,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, && virFileExists(def->os.loader->nvram->path)) return 0; + + if (!reset_nvram && def->os.loader->nvram && + def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK && + def->os.loader->nvram->path) + return 0; + /* ... then we want to consult JSON FW descriptors first, * but we don't want to fail if we haven't found a match. */ needResult = false; -- 2.25.1

On 21/04/22 8:20 pm, Peter Krempa wrote: Right. Ack. Yes, sure. I will add  the validation in next patch. Right, should I add a boolean variable in virDomainLoaderDef to remember the new style and format in the new style ? Or formatting always in the old style in case of VIR_STORAGE_TYPE_FILE is okay ? Ack. Ack. Ack. This is ignoring firmware autoselection feature in case if NVRAM image is there. Shouldn't this be here in this patch ?

On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; wrote: Please add negative unit tests to check the validation along with this patch. Prefix the patch with "qemu:".

On Fri, Apr 08, 2022 at 10:48:46 -0700, Rohit Kumar wrote: As noted in 2/8, this will need to be moved earlier. Return early if there's nothing to validate to decrease indentation level. "Remote NVRAM is not supported by this qemu". Also you need to reject all the other unsupported configs here.

On 21/04/22 8:36 pm, Peter Krempa wrote: Ack. Ack. Ack. Thanks! Sure. I will update it in the next patch. Thanks! Yes, I will add it. Thanks!

Moving diskSourceNetwork and diskSourceFile's Source definition under 'define' element, so that it will be easier to reuse it at multiple places. Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- src/conf/schemas/domaincommon.rng | 60 +++++++++++++++++-------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index c0c14fe558..58eb9670d4 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1724,6 +1724,31 @@ </choice> </define> + <define name="diskSourceFileElement"> + <element name="source"> + <interleave> + <optional> + <attribute name="file"> + <choice> + <ref name="absFilePath"/> + <ref name="vmwarePath"/> + </choice> + </attribute> + </optional> + <ref name="diskSourceCommon"/> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <optional> + <ref name="encryption"/> + </optional> + <zeroOrMore> + <ref name="devSeclabel"/> + </zeroOrMore> + </interleave> + </element> + </define> + <define name="diskSourceFile"> <optional> <attribute name="type"> @@ -1731,28 +1756,7 @@ </attribute> </optional> <optional> - <element name="source"> - <interleave> - <optional> - <attribute name="file"> - <choice> - <ref name="absFilePath"/> - <ref name="vmwarePath"/> - </choice> - </attribute> - </optional> - <ref name="diskSourceCommon"/> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <optional> - <ref name="encryption"/> - </optional> - <zeroOrMore> - <ref name="devSeclabel"/> - </zeroOrMore> - </interleave> - </element> + <ref name="diskSourceFileElement"/> </optional> </define> @@ -2147,10 +2151,7 @@ </element> </define> - <define name="diskSourceNetwork"> - <attribute name="type"> - <value>network</value> - </attribute> + <define name="diskSourceNetworkElement"> <choice> <ref name="diskSourceNetworkProtocolNBD"/> <ref name="diskSourceNetworkProtocolGluster"/> @@ -2165,6 +2166,13 @@ </choice> </define> + <define name="diskSourceNetwork"> + <attribute name="type"> + <value>network</value> + </attribute> + <ref name="diskSourceNetworkElement"/> + </define> + <define name="diskSourceVolume"> <attribute name="type"> <value>volume</value> -- 2.25.1

On 21/04/22 8:29 pm, Peter Krempa wrote: Agreed. Ack. I will remove it. Should this be removed after parsing as well, If we are not mentioning these in schema defination ? Should we report error if user provides startup policy, encryption or security labels  ? Right. I did not understand what does rejecting it meaning here. Is it reporting error like encryption is not supported ? or is it just deallocating 'Auth' field from virStorageSource after parsing ? Right. I will remove it from schema defination. Thanks!

This patch updates NVRAM element schema to support network backed NVRAM. It introduces 'type' attribute to NVRAM element. Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- src/conf/schemas/domaincommon.rng | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 58eb9670d4..cd61d00b33 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -333,7 +333,25 @@ </attribute> </optional> <optional> - <ref name="absFilePath"/> + <attribute name="type"> + <choice> + <value>file</value> + <value>network</value> + </choice> + </attribute> + </optional> + <optional> + <choice> + <group> + <ref name="absFilePath"/> + </group> + <group> + <ref name="diskSourceFileElement"/> + </group> + <group> + <ref name="diskSourceNetworkElement"/> + </group> + </choice> </optional> </element> </optional> -- 2.25.1

On 21/04/22 8:31 pm, Peter Krempa wrote: Thanks for pointing this. I will re-group these. Also, having this grouping in schema is enough ? or do we need to validation for this as well ?

Updating the doc as now NVRAM can be remote as well. Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- docs/formatdomain.rst | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 24fbfd8670..53361c7996 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -141,6 +141,31 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. </os> ... + <!-- QEMU with UEFI manual firmware, secure boot and with NVRAM type 'file'--> + ... + <os> + <type>hvm</type> + <loader readonly='yes' secure='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram type='file' template='/usr/share/OVMF/OVMF_VARS.fd'> + <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/> + <boot dev='hd'/> + </os> + ... + + <!-- QEMU with UEFI manual firmware, secure boot and with network backed NVRAM'--> + ... + <os> + <type>hvm</type> + <loader readonly='yes' secure='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram type='network'> + <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> + <host name='example.com' port='6000'/> + </source> + </nvram> + <boot dev='hd'/> + </os> + ... + <!-- QEMU with automatic UEFI firmware and secure boot --> ... <os firmware='efi'> @@ -222,14 +247,16 @@ harddisk, cdrom, network) determining where to obtain/find the boot image. :since:`Since 2.1.0` ``nvram`` 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 absolute path - to the file is stored in this element. Moreover, when the domain is started - up libvirt copies so called master NVRAM store file defined in ``qemu.conf``. - If needed, the ``template`` attribute can be used to per domain override map - of master NVRAM stores from the config file. Note, that for transient domains - if the NVRAM file has been created by libvirt it is left behind and it is - management application's responsibility to save and remove file (if needed to - be persistent). :since:`Since 1.2.8` + variables. They can be present on host or on some network storage. + In the host, this is represented as a file and the absolute path + to the file is stored in this element. On network storage, + this can be accessed by providing network path. Moreover, when the domain + is started up libvirt copies so called master NVRAM store file defined + in ``qemu.conf``. If needed, the ``template`` attribute can be used to per + domain override map of master NVRAM stores from the config file. Note, that + for transient domains if the NVRAM file has been created by libvirt + it is left behind and it is management application's responsibility to + save and remove file (if needed to be persistent). :since:`Since 1.2.8` ``boot`` The ``dev`` attribute takes one of the values "fd", "hd", "cdrom" or "network" and is used to specify the next boot device to consider. The -- 2.25.1

On 21/04/22 8:35 pm, Peter Krempa wrote: Ack. Oh! Right. I did not understand what do you mean by referring to disk configuration section here.  Does it mean I need to add a link that points to disk configuration section ? Is this the disk configuration section, https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms are you referring ? Sure, thanks. I will merge it with schema update patch.

This patch adds unit test for network backed NVRAM Example: <nvram type='network'> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> <host name='example.com' port='6000'/> </source> </nvram> Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- .../bios-nvram-network.x86_64-latest.args | 37 +++++++++++++++++++ tests/qemuxml2argvdata/bios-nvram-network.xml | 25 +++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 63 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml diff --git a/tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args b/tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args new file mode 100644 index 0000000000..f8a4ff812c --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-test-bios \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=test-bios,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-test-bios/master-key.aes"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \ +-blockdev '{"driver":"iscsi","portal":"example.com:6000","target":"iqn.2013-07.com.example:iscsi-nopool","lun":0,"transport":"tcp","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 1024 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot menu=on,strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/bios-nvram-network.xml b/tests/qemuxml2argvdata/bios-nvram-network.xml new file mode 100644 index 0000000000..ffc590ddfa --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-network.xml @@ -0,0 +1,25 @@ +<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='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram type='network'> + <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool'> + <host name='example.com' port='6000'/> + </source> + </nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ed41b7a7a2..7700ed6cb1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1198,6 +1198,7 @@ mymain(void) QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_VIRTIO_SCSI); DO_TEST_CAPS_LATEST("bios-nvram-template"); + DO_TEST_CAPS_LATEST("bios-nvram-network"); /* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST_NOCAPS("q35-acpi-uefi"); -- 2.25.1

On 21/04/22 8:39 pm, Peter Krempa wrote: Ack. Thanks. I will add qemuxml2xmltest as well. I meant this as just an example. Having iSCSI support is enough for us, but I think we can have support for all the other protocols as well with this patchset.

This patch adds an unit test to test xml when NVRAM type 'file' is provided. Example: <nvram type='file'> <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/> </nvram> Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; --- .../bios-nvram-file.x86_64-latest.args | 37 +++++++++++++++++++ tests/qemuxml2argvdata/bios-nvram-file.xml | 23 ++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 61 insertions(+) create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.xml diff --git a/tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args b/tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args new file mode 100644 index 0000000000..4b0aec7539 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args @@ -0,0 +1,37 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-test-bios \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-test-bios/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-test-bios/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-test-bios/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=test-bios,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-test-bios/master-key.aes"}' \ +-blockdev '{"driver":"file","filename":"/usr/share/OVMF/OVMF_CODE.fd","node-name":"libvirt-pflash0-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash0-format","read-only":true,"driver":"raw","file":"libvirt-pflash0-storage"}' \ +-blockdev '{"driver":"file","filename":"/var/lib/libvirt/nvram/guest_VARS.fd","node-name":"libvirt-pflash1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-pflash1-format","read-only":false,"driver":"raw","file":"libvirt-pflash1-storage"}' \ +-machine pc,usb=off,dump-guest-core=off,pflash0=libvirt-pflash0-format,pflash1=libvirt-pflash1-format,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 1024 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 362d1fc1-df7d-193e-5c18-49a71bd1da66 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot menu=on,strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/bios-nvram-file.xml b/tests/qemuxml2argvdata/bios-nvram-file.xml new file mode 100644 index 0000000000..8df9412112 --- /dev/null +++ b/tests/qemuxml2argvdata/bios-nvram-file.xml @@ -0,0 +1,23 @@ +<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='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader> + <nvram type='file'> + <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/> + </nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7700ed6cb1..0c9881e483 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1199,6 +1199,7 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI); DO_TEST_CAPS_LATEST("bios-nvram-template"); DO_TEST_CAPS_LATEST("bios-nvram-network"); + DO_TEST_CAPS_LATEST("bios-nvram-file"); /* Make sure all combinations of ACPI and UEFI behave as expected */ DO_TEST_NOCAPS("q35-acpi-uefi"); -- 2.25.1