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

Re: [libvirt] [PATCH v4 2/2 RESEND] qemu: Add command line parser for NVRAM.



On 2013年04月18日 17:55, Osier Yang wrote:
On 18/04/13 13:40, Li Zhang wrote:
From: Li Zhang <zhlcindy linux vnet ibm com>

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 linux vnet ibm com>
---
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


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