Re: [PATCH] Introduce network-backed NVRAM

On Sun, Mar 20, 2022 at 21:28:13 -0700, Rohit Kumar wrote: > Libvirt domain XML allows only local filepaths to specify a NVRAM > element. Since VMs can move across different hosts, it should be > possibe to allocate NVRAM disks on network storage for uninturrupted > access. > > This Patch extends the NVRAM disk elements to be described as > virStorageSource* elements. > > Sample XML with new annotation: > > <nvram> This should have a 'type' attribute rather than blindly checking the fields.

> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'> > <host name='example.com' port='6000'/> > </source> > </nvram> > > or > > <nvram> > <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; > --- > docs/schemas/domaincommon.rng | 72 +++++++++------ > src/conf/domain_conf.c | 90 +++++++++++++++---- > src/conf/domain_conf.h | 2 +- > src/qemu/qemu_cgroup.c | 3 +- > src/qemu/qemu_command.c | 5 +- > src/qemu/qemu_domain.c | 23 +++-- > 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 | 8 +- > .../qemuxml2argvdata/bios-nvram-network.args | 35 ++++++++ > tests/qemuxml2argvdata/bios-nvram-network.xml | 40 +++++++++ > tests/qemuxml2argvtest.c | 1 + So this patch is doing too much. You'll have to split it. I'll note how below.

You are completely missing documentation for the new syntax in docs/formatdomain.rst.

> 17 files changed, 256 insertions(+), 72 deletions(-) > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 9c1b64a644..a25c84a0b7 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -330,7 +330,17 @@ > </attribute> > </optional> > <optional> > - <ref name="absFilePath"/> > + <choice> > + <group> > + <ref name="absFilePath"/> > + </group> > + <group> > + <ref name="diskSourceFileElement"/> > + </group> > + <group> > + <ref name="diskSourceNetworkElement"/> > + </group> > + </choice> > </optional> > </element> > </optional> > @@ -1714,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"> > @@ -1721,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> > > @@ -2137,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"/> > @@ -2155,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> Please separate the cleanups (moving of definitions under the 'define' element into a separate patch.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index e0dfc9e45f..5fcc09db05 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -3535,7 +3535,7 @@ virDomainLoaderDefFree(virDomainLoaderDef *loader) > return; > > g_free(loader->path); > - g_free(loader->nvram); > + virObjectUnref(loader->nvram); > g_free(loader->nvramTemplate); > g_free(loader); > } > @@ -17834,6 +17834,37 @@ virDomainLoaderDefParseXML(xmlNodePtr node, > } > > > +static int > +virDomainNvramDefParseXML(virStorageSource *nvram, > + xmlXPathContextPtr ctxt, > + virDomainXMLOption *opt, > + unsigned int flags) > +{ > + g_autofree char *srcTypeFile = NULL; > + g_autofree char *srcTypeNetwork = NULL; > + xmlNodePtr source; > + > + srcTypeFile = virXPathString("string(./os/nvram/source/@file)", ctxt); > + srcTypeNetwork = virXPathString("string(./os/nvram/source/@protocol)", ctxt); > + > + if (!srcTypeFile && !srcTypeNetwork) { > + nvram->type = VIR_STORAGE_TYPE_FILE; > + nvram->path = virXPathString("string(./os/nvram[1])", ctxt); > + return 0; > + } else { > + if (srcTypeFile) { > + nvram->type = VIR_STORAGE_TYPE_FILE; > + } else { > + nvram->type = VIR_STORAGE_TYPE_NETWORK; > + } > + source = virXPathNode("./os/nvram/source[1]", ctxt); > + if (!source) > + return -1; virXPathNode does not report an error but you return it, thus the code would not report anything.

> + return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt); > + } > + > +} > + > static int > virDomainSchedulerParseCommonAttrs(xmlNodePtr node, > virProcessSchedPolicy *policy, > @@ -18219,7 +18250,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; > @@ -18234,7 +18267,14 @@ 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); > + > + 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); > > @@ -18288,7 +18328,9 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def, > > static int > virDomainDefParseBootOptions(virDomainDef *def, > - xmlXPathContextPtr ctxt) > + xmlXPathContextPtr ctxt, > + virDomainXMLOption *xmlopt, > + unsigned int flags) > { > /* > * Booting options for different OS types.... > @@ -18306,7 +18348,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) > @@ -18322,7 +18364,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; > @@ -19606,7 +19648,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 */ > @@ -26899,7 +26941,9 @@ virDomainHugepagesFormat(virBuffer *buf, > > static void > virDomainLoaderDefFormat(virBuffer *buf, > - virDomainLoaderDef *loader) > + virDomainLoaderDef *loader, > + virDomainXMLOption *xmlopt, > + unsigned int flags) > { > const char *readonly = virTristateBoolTypeToString(loader->readonly); > const char *secure = virTristateBoolTypeToString(loader->secure); > @@ -26921,13 +26965,27 @@ virDomainLoaderDefFormat(virBuffer *buf, > else > virBufferAddLit(buf, "/>\n"); > > - if (loader->nvram || loader->nvramTemplate) { > - virBufferAddLit(buf, "<nvram"); > - virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate); > - if (loader->nvram) > - virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram); > - else > - virBufferAddLit(buf, "/>\n"); > + if (loader->nvram) { > + if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) { > + virBufferAddLit(buf, "<nvram"); > + virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate); > + if (loader->nvram->path) > + virBufferEscapeString(buf, ">%s</nvram>\n", loader->nvram->path); > + else > + virBufferAddLit(buf, "/>\n"); > + } else { > + virBufferAddLit(buf, "<nvram"); > + virBufferEscapeString(buf, " template='%s'", loader->nvramTemplate); > + virBufferAdjustIndent(buf, 2); > + if (virDomainDiskSourceFormat(buf, loader->nvram, "source", 0, > + 0, false, flags, true, xmlopt) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot format NVRAM source")); 'virDomainDiskSourceFormat' is already reporting an error so you must not overwrite it ...

> + return; ... also you must propagate it appropriately.

> + } > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</nvram>\n"); I'll post a patch refactoring this function to use virXMLFormatElement which will greatly clean up your addition. Make sure to rebase it on top of it.

> + } > } > } > [...] > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index c836799888..649a6f5b55 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9583,6 +9583,7 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd, > virQEMUCaps *qemuCaps) > { > g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; > + g_autofree char *nvramPath = NULL; > int unit = 0; > > if (loader->secure == VIR_TRISTATE_BOOL_YES) { > @@ -9610,8 +9611,10 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd, > virCommandAddArgBuffer(cmd, &buf); > > if (loader->nvram) { > + if (qemuGetDriveSourceString(loader->nvram, NULL, &nvramPath) < 0) > + return; NACK to this bit. We should only allow the new feature when qemu supports VIR_QEMU_CAPS_BLOCKDEV, thus no change to this function should be needed. Add the check into qemu_validate.c into the appropriate place.

Nobody is reallistically going to keep maintaining support for old qemu.

> virBufferAddLit(&buf, "file="); > - virQEMUBuildBufferEscapeComma(&buf, loader->nvram); > + virQEMUBuildBufferEscapeComma(&buf, nvramPath); > 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 7180ae616b..303d9661a4 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -4452,8 +4452,13 @@ 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; > @@ -11133,15 +11138,23 @@ qemuDomainInitializePflashStorageSource(virDomainObj *vm) > pflash0->nodeformat = g_strdup("libvirt-pflash0-format"); > pflash0->nodestorage = g_strdup("libvirt-pflash0-storage"); > > - > if (def->os.loader->nvram) { > pflash1 = virStorageSourceNew(); > - pflash1->type = VIR_STORAGE_TYPE_FILE; > pflash1->format = VIR_STORAGE_FILE_RAW; > - pflash1->path = g_strdup(def->os.loader->nvram); > + pflash1->path = g_strdup(def->os.loader->nvram->path); > pflash1->readonly = false; > pflash1->nodeformat = g_strdup("libvirt-pflash1-format"); > pflash1->nodestorage = g_strdup("libvirt-pflash1-storage"); > + > + if (def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) { > + pflash1->type = VIR_STORAGE_TYPE_FILE; > + } else if (def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK) { > + pflash1->protocol = def->os.loader->nvram->protocol; > + pflash1->hosts = g_new0(virStorageNetHostDef, 1); > + pflash1->nhosts = 1; > + pflash1->hosts[0].name = def->os.loader->nvram->hosts[0].name; > + pflash1->hosts[0].port = def->os.loader->nvram->hosts[0].port; since both 'def->os.loader->nvram' and pflash1 are 'virStorageSource' you should use virStorageSourceCopy instead of this.

> + } > } > > priv->pflash0 = g_steal_pointer(&pflash0); [...] > diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c > index 51223faadf..fa99c7d217 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 || > + (def->os.loader->nvram && > + !reset_nvram && ((def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE > + && virFileExists(def->os.loader->nvram->path)) || > + (def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK && > + def->os.loader->nvram->path)))) This condition got a bit too crazy ... please factor out the inner clause you've added into a sub-condition.

> return 0; > > /* ... then we want to consult JSON FW descriptors first, [...] > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c > index acd18494d3..d6a482d28b 100644 > --- a/src/vbox/vbox_common.c > +++ b/src/vbox/vbox_common.c > @@ -994,10 +994,10 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data, > VIR_DEBUG("def->os.cmdline %s", def->os.cmdline); > VIR_DEBUG("def->os.root %s", def->os.root); > 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.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->path %s", def->os.loader->nvram->path); These seem to be aligning with the block above as well, but IMO we should not do any alignment here. I'll post a patch removing the whitespace, so please rebase to it.

> } > VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); > VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); > diff --git a/tests/qemuxml2argvdata/bios-nvram-network.args b/tests/qemuxml2argvdata/bios-nvram-network.args > new file mode 100644 > index 0000000000..05075b45de > --- /dev/null > +++ b/tests/qemuxml2argvdata/bios-nvram-network.args > @@ -0,0 +1,35 @@ > +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 \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu-system-x86_64 \ > +-name guest=test-bios,debug-threads=on \ > +-S \ > +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-test-bios/master-key.aes \ > +-machine pc,usb=off,dump-guest-core=off \ > +-accel tcg \ > +-drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \ > +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example,if=pflash,format=raw,unit=1 \ As noted below we need at least one test case which uses -blockdev for this which should be used by modern qemu.

> +-m 1024 \ > +-realtime mlock=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,path=/tmp/lib/domain--1-test-bios/monitor.sock,server=on,wait=off \ > +-mon chardev=charmonitor,id=monitor,mode=control \ > +-rtc base=utc \ > +-no-shutdown \ > +-boot menu=on,strict=on \ > +-usb \ > +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ > +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ > +-device usb-tablet,id=input0,bus=usb.0,port=1 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ > +-msg timestamp=on [...] > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index e7fecb24d3..7ec45ec74b 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1189,6 +1189,7 @@ mymain(void) > QEMU_CAPS_DEVICE_ISA_SERIAL); > DO_TEST_NOCAPS("bios-nvram"); > DO_TEST_PARSE_ERROR_NOCAPS("bios-nvram-no-path"); > + DO_TEST_NOCAPS("bios-nvram-network"); For new tests, please always use 'DO_TEST_CAPS_LATEST' or DO_TEST_CAPS_VER for older versions. New use of DO_TEST_NOCAPS is not desirable.

> DO_TEST_CAPS_LATEST("bios-nvram-rw"); > DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit"); > DO_TEST("bios-nvram-secure", > -- > 2.25.1 >