On 2013年04月18日 17:55, Osier Yang wrote:
On 18/04/13 13:40, Li Zhang wrote:
From: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
This patch is to add command line parser for NVRAM device,
s/parser/builder and parser/,
I didn't go through it carefully, but what I catched with a rough look...
Thanks for review.
and add test cases.
Signed-off-by: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
---
src/qemu/qemu_command.c | 72 ++++++++++++++++++++++
src/qemu/qemu_command.h | 2 +
tests/qemuargv2xmltest.c | 2 +
.../qemuxml2argv-pseries-nvram.args | 1 +
.../qemuxml2argv-pseries-nvram.xml | 22 +++++++
tests/qemuxml2argvtest.c | 1 +
6 files changed, 100 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
Don't we need a new capability flag to detect if the device is
supported by
qemu? or it's already supported?
No, there is no a capability for it. I can add one for it.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d899239..490881e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1172,6 +1172,15 @@ int
qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
goto cleanup;
}
+ if (def->nvram) {
+ 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)
What does "0x3000ul" means, should we have a macro for it?
This is the address of this VIO device.
We assign VIO devices from 0x1000UL~0x3000UL as the default address.
To make code clear, I will create macros for all these VIO devices.
+ goto cleanup;
+ }
+
/* No other devices are currently supported on spapr-vio */
ret = 0;
@@ -3960,6 +3969,32 @@ error:
return NULL;
}
+char *
Is it used externally? as far as I see, no, so it should be static.
No. I will change it as static.
Thanks.
+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);
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("NVRAM device address is not supported.\n"));
It should be XML error, but not "not supported".
OK.
+ goto error;
+ }
+
+ if (virBufferError(&buf)) {
+ virReportOOMError();
+ goto error;
+ }
+
+ return virBufferContentAndReset(&buf);
+
+error:
+ virBufferFreeAndReset(&buf);
+ return NULL;
+}
char *
qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
@@ -7662,6 +7697,23 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
+ if (def->nvram) {
+ if (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);
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("NVRAM device is not supported"));
How about make it more clear:
"nvram device is only supported for......", as this sounds like it's
not supported
anyway.
OK. I will modify it.
+ goto error;
+ }
+ }
if (snapshot)
virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
@@ -9770,6 +9822,26 @@ 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;
+
+ def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
+ def->nvram->info.addr.spaprvio.has_reg = true;
+
+ val += strlen("spapr-nvram.reg=");
+ if (virStrToLong_ull(val, NULL, 16,
+ &def->nvram->info.addr.spaprvio.reg) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
Not right error type.. Generally we report error like this:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot parse drive index '%s'"), val);
OK.
+ _("invalid value for spapr-nvram.reg :"
+ "'%s'"), val);
+ goto error;
Hope def->nvram is freed somwhere, such as virDomainDefFree.
Yes, it will be freed in virDomainDefFree.
+ }
+
Useless blank line.
} else if (STREQ(arg, "-S")) {
/* ignore, always added by libvirt */
} else {
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 1789c20..ecd4f02 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -118,6 +118,8 @@ char *
qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev,
char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev,
virQEMUCapsPtr qemuCaps);
+char * qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev);
And this should be removed. Per it's static helper.
+
char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
virQEMUCapsPtr qemuCaps);
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 9167d88..1ee7aa2 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -243,6 +243,8 @@ mymain(void)
DO_TEST("hyperv");
+ DO_TEST("pseries-nvram");
+
DO_TEST_FULL("restore-v1", 0, "stdio");
DO_TEST_FULL("restore-v2", 0, "stdio");
DO_TEST_FULL("restore-v2", 0, "exec:cat");
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args
new file mode 100644
index 0000000..ca5333e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args
@@ -0,0 +1 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
/usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic
-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb
-net none -serial none -parallel none -global spapr-nvram.reg=0x4000
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
Long line, see how other cases do.
Ah, I didn't realize that. Will modify it.
new file mode 100644
index 0000000..d7be30f
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml
@@ -0,0 +1,22 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
+ <memory unit='KiB'>524288</memory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='ppc64' machine='pseries'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-ppc64</emulator>
+ <controller type='usb' index='0'/>
+ <memballoon model='virtio'/>
+ <nvram>
+ <address type='spapr-vio' reg='0x4000'/>
+ </nvram>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index d6575e7..1a5d37f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -904,6 +904,7 @@ mymain(void)
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST_ERROR("pseries-vio-address-clash", QEMU_CAPS_DRIVE,
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+ DO_TEST("pseries-nvram", NONE);
DO_TEST("disk-ide-drive-split",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG,
QEMU_CAPS_IDE_CD);
I think you will want xml2xml test too. To make sure the XML formating
works as expected.
OK, I will add that.
Thanks.
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list