1. Patchew
  2. Libvirt
  3. View series

[PATCH] Introduce network-backed NVRAM

Rohit Kumar posted 1 patch 2 years, 1 month ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220321042813.437626-1-rohit.kumar3@nutanix.com
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 +
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
[PATCH] Introduce network-backed NVRAM
Posted by Rohit Kumar 2 years, 1 month ago 
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>
  <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@nutanix.com>
Signed-off-by: Florian Schmidt <flosch@nutanix.com>
Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
---
 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 +
 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>
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;
+        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"));
+                return;
+            }
+            virBufferAdjustIndent(buf, -2);
+            virBufferAddLit(buf, "</nvram>\n");
+        }
     }
 }
 
@@ -28122,7 +28180,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
         virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
 
     if (def->os.loader)
-        virDomainLoaderDefFormat(buf, def->os.loader);
+        virDomainLoaderDefFormat(buf, def->os.loader, xmlopt, flags);
     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 a4de46773c..f65558e82d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2231,7 +2231,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 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;
         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;
+        }
     }
 
     priv->pflash0 = g_steal_pointer(&pflash0);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b7e83c769a..4f4098975c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6605,8 +6605,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..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))))
             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 1ed60917ea..c8a25dbe91 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4475,7 +4475,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;
@@ -4507,7 +4508,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..5803f9ff8d 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 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);
     }
     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 \
+-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/qemuxml2argvdata/bios-nvram-network.xml b/tests/qemuxml2argvdata/bios-nvram-network.xml
new file mode 100644
index 0000000000..0e1e23fd31
--- /dev/null
+++ b/tests/qemuxml2argvdata/bios-nvram-network.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='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
+    <nvram>
+      <source protocol='iscsi' name='iqn.1992-01.com.example'>
+        <host name='example.org' port='6000'/>
+      </source>
+    </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-system-x86_64</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'/>
+    <input type='tablet' bus='usb'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
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");
     DO_TEST_CAPS_LATEST("bios-nvram-rw");
     DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit");
     DO_TEST("bios-nvram-secure",
-- 
2.25.1
Re: [PATCH] Introduce network-backed NVRAM
Posted by Peter Krempa 2 years, 1 month ago 
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@nutanix.com>
> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> ---
>  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
>
Re: [PATCH] Introduce network-backed NVRAM
Posted by Rohit Kumar 2 years, 1 month ago 
Thanks a lot for taking the time to review it !

On 28/03/22 6:35 pm, Peter Krempa wrote:
> 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.
Sure, I will add this attribute for network/file type in next patch.
>>    <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@nutanix.com>
>> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
>> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
>> ---
>>   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.
Thanks! Most of the changes are interdependent, so I found a bit hard to 
saperate it.
> You are completely missing documentation for the new syntax in
> docs/formatdomain.rst.
Sure, I will update it. Thanks for pointing this out.
>
>>   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.
Ack., thanks !
>
>> 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.
Right, If source tag is not present, we should report an error before 
returning. Will Update this as well.
>
>> +        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 ...
Yes, I will remove it.
>> +                return;
> ... also you must propagate it appropriately.
Ack.
>
>> +            }
>> +            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.
Sure, thanks !
>
>> +        }
>>       }
>>   }
>>   
> [...]
>
>> 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.
Ack. Thanks!
>
> Nobody is reallistically going to keep maintaining support for old qemu.
yes, true !
>
>>           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.
Ack.
>
>
>> +        }
>>       }
>>   
>>       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.
Yes, sure.
>>               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.
yes, Thanks.
>
>>       }
>>       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.
Ok, I will update the test case for blockdev cap. If we are introducing 
this feature when blockdev is supported, then this test case would not 
be needed anymore.
>
>> +-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.
Ack. Thanks !
>
>>       DO_TEST_CAPS_LATEST("bios-nvram-rw");
>>       DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit");
>>       DO_TEST("bios-nvram-secure",
>> -- 
>> 2.25.1
>>
Re: [PATCH] Introduce network-backed NVRAM
Posted by Rohit Kumar 2 years, 1 month ago 
Ping.
Hi, please review this patch.
Thanks !

On 21/03/22 9:58 am, 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>
>    <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@nutanix.com>
> Signed-off-by: Florian Schmidt <flosch@nutanix.com>
> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com>
> ---
>   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 +
>   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>
> 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;
> +        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"));
> +                return;
> +            }
> +            virBufferAdjustIndent(buf, -2);
> +            virBufferAddLit(buf, "</nvram>\n");
> +        }
>       }
>   }
>   
> @@ -28122,7 +28180,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>           virBufferAsprintf(buf, "<initgroup>%s</initgroup>\n", def->os.initgroup);
>   
>       if (def->os.loader)
> -        virDomainLoaderDefFormat(buf, def->os.loader);
> +        virDomainLoaderDefFormat(buf, def->os.loader, xmlopt, flags);
>       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 a4de46773c..f65558e82d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2231,7 +2231,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 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;
>           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;
> +        }
>       }
>   
>       priv->pflash0 = g_steal_pointer(&pflash0);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b7e83c769a..4f4098975c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6605,8 +6605,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..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))))
>               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 1ed60917ea..c8a25dbe91 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4475,7 +4475,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;
> @@ -4507,7 +4508,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..5803f9ff8d 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 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);
>       }
>       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 \
> +-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/qemuxml2argvdata/bios-nvram-network.xml b/tests/qemuxml2argvdata/bios-nvram-network.xml
> new file mode 100644
> index 0000000000..0e1e23fd31
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/bios-nvram-network.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='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
> +    <nvram>
> +      <source protocol='iscsi' name='iqn.1992-01.com.example'>
> +        <host name='example.org' port='6000'/>
> +      </source>
> +    </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-system-x86_64</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'/>
> +    <input type='tablet' bus='usb'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> 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");
>       DO_TEST_CAPS_LATEST("bios-nvram-rw");
>       DO_TEST_CAPS_LATEST("bios-nvram-rw-implicit");
>       DO_TEST("bios-nvram-secure",

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.