On 24/04/13 19:58, Osier Yang wrote:
On 19/04/13 16:37, Li Zhang wrote:
From: Li Zhang <zhlci...@linux.vnet.ibm.com>

For pSeries guest in QEMU, NVRAM is one kind of spapr-vio device.
Users are allowed to specify spapr-vio devices'address.
But NVRAM is not supported in libvirt. So this patch is to
add NVRAM device to allow users to specify its address.

In QEMU, NVRAM device's address is specified by
  "-global spapr-nvram.reg=xxxxx".

In libvirt, XML file is defined as the following:

   <nvram>
     <address type='spapr-vio' reg='0x3000'/>
   </nvram>

Signed-off-by: Li Zhang <zhlci...@linux.vnet.ibm.com>
---
  v5 -> v4:
   * Add NVRAM capability suggested by Osier Yang.
   * Define macros for VIO device address.
   * Define qemuBuildNVRAMDevStr as static func.
   * Correct several error messages.
   * Add xml2xml test case

  v4 -> v3:
   * Seperate NVRAM definition from qemu command line parser.

  v3 -> v2:
* Add NVRAM in domaincommon.rng and formatdomain.xml.in suggested by Daniel P.Berrange
   * Add NVRAM test cases suggested by Daniel P.Berrange
* Remove nvram allocation when it is not specified suggested by Daniel P.Berrange
   * Add several error reports suggested by Daniel P.Berrange

  v2 -> v1:
   * Add NVRAM parameters parsing in qemuParseCommandLine

  docs/formatdomain.html.in     | 35 +++++++++++++++++++
  docs/schemas/domaincommon.rng | 10 ++++++
src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++++++++++++
  src/conf/domain_conf.h        | 10 ++++++
  4 files changed, 136 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0cc56d9..4a7a66f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4502,6 +4502,41 @@ qemu-kvm -net nic,model=? /dev/null
        </dd>
      </dl>
  +    <h4><a name="elementsNVRAM">NVRAM device</a></h4>
+    <p>
+      One NVRAM device is always added to pSeries guests on PPC64.
+      And on PPC64, NVRAM devices' address type are VIO which

s/are/is/,  s/VIO which/VIO, which/,

+      allows users to change.<code>nvram</code> element in XML file

s/change\./change\. /,

+      is provided to specify its address.
+ Currently, libvirt only considers configuration for pSeries guests.
+    </p>

How about rewords the documents like:

nvram device is always added to pSeries guest on PPC64, and its address
is allowed to be changed.  Element <code>nvram</code> is provided to
enable the address setting.

+    <p>
+      Example: usage of NVRAM configuration
+    </p>
+<pre>
+  ...
+  &lt;devices&gt;
+    &lt;nvram&gt;
+      &lt;address type='spapr-vio' reg='0x3000'/&gt;
+    &lt;/nvram&gt;
+  &lt;/devices&gt;
+  ...
+</pre>
+  <dl>
+    <dt><code>spapr-vio</code></dt>
+    <dd>
+      <p>
+        VIO device address type, this is only for PPC64.

s/this is only for/only valid for/,

+      </p>
+    </dd>
+    <dt><code>reg</code></dt>
+    <dd>
+      <p>
+        Devices' address

s/Devices'/Device's/

+      </p>
+    </dd>
+  </dl>
+
      <h3><a name="seclabel">Security label</a></h3>
        <p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 3976b82..86ef540 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2793,6 +2793,13 @@
        </optional>
      </element>
    </define>
+  <define name="nvram">
+    <element name="nvram">
+      <optional>
+        <ref name="address"/>
+      </optional>
+    </element>
+  </define>
    <define name="memballoon">
      <element name="memballoon">
        <attribute name="model">
@@ -3280,6 +3287,9 @@
          <optional>
            <ref name="memballoon"/>
          </optional>
+        <optional>
+          <ref name="nvram"/>
+        </optional>
        </interleave>
      </element>
    </define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1643f30..acaec20 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -194,6 +194,7 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
                "smartcard",
                "chr",
                "memballoon",
+              "nvram",
                "rng")
VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, @@ -1560,6 +1561,16 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def)
      VIR_FREE(def);
  }
  +void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def)
+{
+    if (!def)
+        return;
+
+    virDomainDeviceInfoClear(&def->info);
+
+    VIR_FREE(def);
+}
+
  void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def)
  {
      if (!def)
@@ -1743,6 +1754,7 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def)
      case VIR_DOMAIN_DEVICE_SMARTCARD:
      case VIR_DOMAIN_DEVICE_CHR:
      case VIR_DOMAIN_DEVICE_MEMBALLOON:
+    case VIR_DOMAIN_DEVICE_NVRAM:

Any special reason to not use virDomainNVRAMDefFree here? and same question
for other device types.

      case VIR_DOMAIN_DEVICE_LAST:
          break;
      }
@@ -1951,6 +1963,7 @@ void virDomainDefFree(virDomainDefPtr def)
      virDomainWatchdogDefFree(def->watchdog);
        virDomainMemballoonDefFree(def->memballoon);
+    virDomainNVRAMDefFree(def->nvram);
        for (i = 0; i < def->nseclabels; i++)
          virSecurityLabelDefFree(def->seclabels[i]);
@@ -2519,6 +2532,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
          if (cb(def, &device, &def->rng->info, opaque) < 0)
              return -1;
      }
+    if (def->nvram) {
+        device.type = VIR_DOMAIN_DEVICE_NVRAM;
+        device.data.nvram = def->nvram;
+        if (cb(def, &device, &def->nvram->info, opaque) < 0)
+            return -1;
+    }
      device.type = VIR_DOMAIN_DEVICE_HUB;
      for (i = 0; i < def->nhubs ; i++) {
          device.data.hub = def->hubs[i];
@@ -2550,6 +2569,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
      case VIR_DOMAIN_DEVICE_SMARTCARD:
      case VIR_DOMAIN_DEVICE_CHR:
      case VIR_DOMAIN_DEVICE_MEMBALLOON:
+    case VIR_DOMAIN_DEVICE_NVRAM:
      case VIR_DOMAIN_DEVICE_LAST:
      case VIR_DOMAIN_DEVICE_RNG:
          break;
@@ -8130,6 +8150,28 @@ error:
      goto cleanup;
  }
  +static virDomainNVRAMDefPtr
+virDomainNVRAMDefParseXML(const xmlNodePtr node,
+                          unsigned int flags)
+{
+   virDomainNVRAMDefPtr def;
+
+    if (VIR_ALLOC(def) < 0) {
+        virReportOOMError();
+        return NULL;
+    }
+
+    if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
+        goto error;
+
+    return def;
+
+error:
+    virDomainNVRAMDefFree(def);
+    def = NULL;
+    return def;
          "return NULL;" is enough.
+}
+
  static virSysinfoDefPtr
  virSysinfoParseXML(const xmlNodePtr node,
                    xmlXPathContextPtr ctxt)
@@ -11304,6 +11346,26 @@ virDomainDefParseXML(xmlDocPtr xml,
      }
      VIR_FREE(nodes);
  +    def->nvram = NULL;
+    if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) {
+        goto error;
+    }
+
+    if (n > 1) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("only a single nvram device is supported"));
+        goto error;
+    }
+
+    if (n > 0) {
           } else if (n == 1) {
+        virDomainNVRAMDefPtr nvram =
+            virDomainNVRAMDefParseXML(nodes[0], flags);
+        if (!nvram)
+            goto error;
+        def->nvram = nvram;
+        VIR_FREE(nodes);
+    }
+
      /* analysis of the hub devices */
      if ((n = virXPathNodeSet("./devices/hub", ctxt, &nodes)) < 0) {
          goto error;
@@ -14391,6 +14453,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
  }
    static int
+virDomainNVRAMDefFormat(virBufferPtr buf,
+                        virDomainNVRAMDefPtr def,
+                        unsigned int flags)
+{
+    virBufferAddLit(buf, "    <nvram>\n");
+    if (virDomainDeviceInfoIsSet(&def->info, flags) &&
+        virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+        return -1;
+
+    virBufferAddLit(buf, "    </nvram>\n");
+
+    return 0;
+}
+
+static int
  virDomainSysinfoDefFormat(virBufferPtr buf,
                            virSysinfoDefPtr def)
  {
@@ -15720,6 +15797,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
      if (def->rng)
          virDomainRNGDefFormat(buf, def->rng, flags);
  +    if (def->nvram)
+        virDomainNVRAMDefFormat(buf, def->nvram, flags);
+
      virBufferAddLit(buf, "  </devices>\n");
        virBufferAdjustIndent(buf, 2);
@@ -17002,6 +17082,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
      case VIR_DOMAIN_DEVICE_SMARTCARD:
      case VIR_DOMAIN_DEVICE_CHR:
      case VIR_DOMAIN_DEVICE_MEMBALLOON:
+    case VIR_DOMAIN_DEVICE_NVRAM:
      case VIR_DOMAIN_DEVICE_LAST:
          virReportError(VIR_ERR_INTERNAL_ERROR,
                         _("Copying definition of '%d' type "
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f1f01fa..502629a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -109,6 +109,9 @@ typedef virDomainChrDef *virDomainChrDefPtr;
  typedef struct _virDomainMemballoonDef virDomainMemballoonDef;
  typedef virDomainMemballoonDef *virDomainMemballoonDefPtr;
  +typedef struct _virDomainNVRAMDef virDomainNVRAMDef;
+typedef virDomainNVRAMDef *virDomainNVRAMDefPtr;
+
  typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
  typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
  @@ -137,6 +140,7 @@ typedef enum {
      VIR_DOMAIN_DEVICE_SMARTCARD,
      VIR_DOMAIN_DEVICE_CHR,
      VIR_DOMAIN_DEVICE_MEMBALLOON,
+    VIR_DOMAIN_DEVICE_NVRAM,
      VIR_DOMAIN_DEVICE_RNG,
        VIR_DOMAIN_DEVICE_LAST
@@ -163,6 +167,7 @@ struct _virDomainDeviceDef {
          virDomainSmartcardDefPtr smartcard;
          virDomainChrDefPtr chr;
          virDomainMemballoonDefPtr memballoon;
+        virDomainNVRAMDefPtr nvram;
          virDomainRNGDefPtr rng;
      } data;
  };
@@ -1469,6 +1474,9 @@ struct _virDomainMemballoonDef {
      virDomainDeviceInfo info;
  };
  +struct _virDomainNVRAMDef {
+    virDomainDeviceInfo info;
+};
    enum virDomainSmbiosMode {
      VIR_DOMAIN_SMBIOS_NONE = 0,
@@ -1916,6 +1924,7 @@ struct _virDomainDef {
      /* Only 1 */
      virDomainWatchdogDefPtr watchdog;
      virDomainMemballoonDefPtr memballoon;
+    virDomainNVRAMDefPtr nvram;
      virDomainTPMDefPtr tpm;
      virCPUDefPtr cpu;
      virSysinfoDefPtr sysinfo;
@@ -2076,6 +2085,7 @@ int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src,
  void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def);
  void virDomainSoundDefFree(virDomainSoundDefPtr def);
  void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def);
+void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def);
  void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def);
  void virDomainVideoDefFree(virDomainVideoDefPtr def);
  virDomainHostdevDefPtr virDomainHostdevDefAlloc(void);

ACK with the nits fixed, and if a good answer for the question.



I'm going to push the patch with the attached diff squashed in. For the "question",
we can have a later patch.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 086a3da..835e73b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4504,11 +4504,10 @@ qemu-kvm -net nic,model=? /dev/null
 
     <h4><a name="elementsNVRAM">NVRAM device</a></h4>
     <p>
-      One NVRAM device is always added to pSeries guests on PPC64.
-      And on PPC64, NVRAM devices' address type are VIO which
-      allows users to change.<code>nvram</code> element in XML file
-      is provided to specify its address.
-      Currently, libvirt only considers configuration for pSeries guests.
+      nvram device is always added to pSeries guest on PPC64, and its address
+      is allowed to be changed.  Element <code>nvram</code> (only valid for
+      pSeries guest, <span class="since">since 1.0.5</span>) is provided to
+      enable the address setting.
     </p>
     <p>
       Example: usage of NVRAM configuration
@@ -4526,13 +4525,13 @@ qemu-kvm -net nic,model=? /dev/null
     <dt><code>spapr-vio</code></dt>
     <dd>
       <p>
-        VIO device address type, this is only for PPC64.
+        VIO device address type, only valid for PPC64.
       </p>
     </dd>
     <dt><code>reg</code></dt>
     <dd>
       <p>
-        Devices' address
+        Device address
       </p>
     </dd>
   </dl>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a7be035..892931a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8168,8 +8168,7 @@ virDomainNVRAMDefParseXML(const xmlNodePtr node,
 
 error:
     virDomainNVRAMDefFree(def);
-    def = NULL;
-    return def;
+    return NULL;
 }
 
 static virSysinfoDefPtr
@@ -11340,7 +11339,6 @@ virDomainDefParseXML(xmlDocPtr xml,
     }
     VIR_FREE(nodes);
 
-    def->nvram = NULL;
     if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) {
         goto error;
     }
@@ -11349,9 +11347,7 @@ virDomainDefParseXML(xmlDocPtr xml,
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("only a single nvram device is supported"));
         goto error;
-    }
-
-    if (n > 0) {
+    } else if (n == 1) {
         virDomainNVRAMDefPtr nvram =
             virDomainNVRAMDefParseXML(nodes[0], flags);
         if (!nvram)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to