[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/1][RESEND] Add NVRAM device



On 2013年03月14日 19:02, Daniel P. Berrange wrote:
On Thu, Mar 14, 2013 at 02:54:19PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy 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 <zhlcindy linux vnet ibm com>
---
  * v2 -> v1:
     Add NVRAM parameters parsing in qemuParseCommandLine.

  src/conf/domain_conf.c  |   83 ++++++++++++++++++++++++++++++++++++++++++++++-
  src/conf/domain_conf.h  |   10 ++++++
  src/qemu/qemu_command.c |   48 +++++++++++++++++++++++++++
  src/qemu/qemu_command.h |    2 ++
  4 files changed, 142 insertions(+), 1 deletion(-)
When adding new XML, you also need to update docs/schemas/domaincommon.rng
and docs/formatdomain.html.in

In addition for anything which extends the QEMU command line generator
you should be adding test case(s) to tests/qemuxml2argvtest.c and also
tests/qemuargv2xmltest.c
Sure, I will do that in my next version.


@@ -10572,6 +10608,32 @@ virDomainDefParseXML(virCapsPtr caps,
          }
      }
+ 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) {
+        virDomainNVRAMDefPtr nvram =
+            virDomainNVRAMDefParseXML(nodes[0], flags);
+        if (!nvram)
+            goto error;
+        def->nvram = nvram;
+        VIR_FREE(nodes);
+    } else {
+        virDomainNVRAMDefPtr nvram;
+        if (VIR_ALLOC(nvram) < 0)
+            goto no_memory;
+        nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
+        def->nvram = nvram;
This adds a <nvram> device to every single guest whether it wants
one or not, which is wrong. Just delete this entire 'else' block.

In QEMU, NVRAM device is always enabled.
So I would like to add this device in libvirt with default.

NB if you had run 'make check' you would see this flaw.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index dee493f..30694d6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -941,6 +941,13 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
              goto cleanup;
      }
+ if (def->os.arch == VIR_ARCH_PPC64 &&
+        STREQ(def->os.machine, "pseries"))
+        def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
+     if (qemuAssignSpaprVIOAddress(def, &def->nvram->info,
+                                   0x3000ul) < 0)
+        goto cleanup;
Bad indentation level on the second 'if'
I will correct it.


+char *
+qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev)
+{
+     virBuffer buf = VIR_BUFFER_INITIALIZER;
+     if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO &&
+         dev->info.addr.spaprvio.has_reg)
+        virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx",
+                          dev->info.addr.spaprvio.reg);
+

You should have an 'else' clause here to report an error in that
scenario

You must also check  virBufferError() and raise an OOM error if
required.
OK. I will add that.

+     return virBufferContentAndReset(&buf);
+}
char *
  qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
@@ -7006,6 +7024,19 @@ qemuBuildCommandLine(virConnectPtr conn,
          }
      }
+ if (def->nvram &&
+        def->os.arch == VIR_ARCH_PPC64 &&
+        STREQ(def->os.machine, "pseries")) {
+        char *optstr;
+        virCommandAddArg(cmd, "-global");
+        optstr = qemuBuildNVRAMDevStr(def->nvram);
+        if (!optstr)
+            goto error;
+        if (optstr)
+           virCommandAddArg(cmd, optstr);
+        VIR_FREE(optstr);
+    }
You must have an else clause here and report VIR_ERR_CONFIG_UNSUPPORTED
OK, I will add that in next version.


+
      if (snapshot)
          virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
@@ -9139,6 +9170,23 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps,
                  goto error;
              }
+ } else if (STREQ(arg, "-global") &&
+                   STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) {
+
+           WANT_VALUE();
+
+           if (VIR_ALLOC(def->nvram) < 0)
+               goto no_memory;
+
+           val += strlen("spapr-nvram.reg=");
+           if (virStrToLong_ull(val, NULL, 16,
+                               &def->nvram->info.addr.spaprvio.reg) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                              _("invalid value for spapr-nvram.reg :"
+                                 "'%s'"), val);
+                goto error;
+          }
+
          } else if (STREQ(arg, "-S")) {
              /* ignore, always added by libvirt */
          } else {


Regards,
Daniel


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]