[libvirt] [PATCH v4 1/2 RESEND] Add NVRAM device
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> --- v4 -> v3: * Sperate 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 | 76 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++ 4 files changed, 131 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d400e35..f590304 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4415,6 +4415,41 @@ qemu-kvm -net nic,model=? /dev/null </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 + 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. + </p> + <p> + Example: usage of NVRAM configuration + </p> +<pre> + ... + <devices> + <nvram> + <address type='spapr-vio' reg='0x3000'/> + </nvram> + </devices> + ... +</pre> + <dl> + <dt><code>spapr-vio</code></dt> + <dd> + <p> + VIO device address type, this is only for PPC64. + </p> + </dd> + <dt><code>reg</code></dt> + <dd> + <p> + Devices' address + </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 2c31f76..2b58d95 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2764,6 +2764,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"> @@ -3208,6 +3215,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 36a46da..3a5e59a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -193,6 +193,7 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "smartcard", "chr", "memballoon", + "nvram", "rng") VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, @@ -1552,6 +1553,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) @@ -1718,6 +1729,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: case VIR_DOMAIN_DEVICE_LAST: break; } @@ -1910,6 +1922,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]); @@ -2478,6 +2491,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]; @@ -2509,6 +2528,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; @@ -7994,6 +8014,23 @@ 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 ) + return NULL; + + return def; +} + static virSysinfoDefPtr virSysinfoParseXML(const xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -11105,6 +11142,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) { + 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; @@ -14159,6 +14216,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, } static int +virDomainNVRAMDefFormat(virBufferPtr buf, + virDomainNVRAMDefPtr def, + unsigned int flags) +{ + virBufferAsprintf(buf, " <nvram>\n"); + if (virDomainDeviceInfoIsSet(&def->info, flags)) + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAddLit(buf, " </nvram>\n"); + + return 0; +} + +static int virDomainSysinfoDefFormat(virBufferPtr buf, virSysinfoDefPtr def) { @@ -15469,6 +15541,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); @@ -16749,6 +16824,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 3ab0f15..d98bec1 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; }; @@ -1442,6 +1447,9 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; }; +struct _virDomainNVRAMDef { + virDomainDeviceInfo info; +}; enum virDomainSmbiosMode { VIR_DOMAIN_SMBIOS_NONE = 0, @@ -1883,6 +1891,7 @@ struct _virDomainDef { /* Only 1 */ virDomainWatchdogDefPtr watchdog; virDomainMemballoonDefPtr memballoon; + virDomainNVRAMDefPtr nvram; virCPUDefPtr cpu; virSysinfoDefPtr sysinfo; virDomainRedirFilterDefPtr redirfilter; @@ -2041,6 +2050,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); -- 1.8.1.4
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> This patch is to add command line parser for NVRAM device, 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 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) + goto cleanup; + } + /* No other devices are currently supported on spapr-vio */ ret = 0; @@ -3960,6 +3969,32 @@ error: return NULL; } +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); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("NVRAM device address is not supported.\n")); + 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")); + 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, + _("invalid value for spapr-nvram.reg :" + "'%s'"), val); + goto error; + } + } 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); + 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 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); -- 1.8.1.4
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...
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?
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?
+ 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.
+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".
+ 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.
+ 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);
+ _("invalid value for spapr-nvram.reg :" + "'%s'"), val); + goto error;
Hope def->nvram is freed somwhere, such as 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.
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. Osier
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
On Thu, Apr 18, 2013 at 01:40:03PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
This patch is to add command line parser for NVRAM device, 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
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Apr 18, 2013 at 01:40:02PM +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> --- v4 -> v3: * Sperate 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 | 76 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 10 ++++++ 4 files changed, 131 insertions(+)
ACK Danie -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/17/2013 11:40 PM, 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".
+static virDomainNVRAMDefPtr +virDomainNVRAMDefParseXML(const xmlNodePtr node, + unsigned int flags)
Alignment. The 'c' of const and 'u' of unsigned should be in the same column: 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 )
Style: no spaces inside () and the expression it contains. This should be: if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
+ return NULL;
Memory leak. If the parse fails, you leaked def.
@@ -14159,6 +14216,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, }
static int +virDomainNVRAMDefFormat(virBufferPtr buf, + virDomainNVRAMDefPtr def, + unsigned int flags)
Another case of incorrect indentation.
+{ + virBufferAsprintf(buf, " <nvram>\n");
Use virBufferAddLit when there is nothing to be formatted (reserve virBufferAsprintf for use of "%" sequences).
+ if (virDomainDeviceInfoIsSet(&def->info, flags)) + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
You could combine these two into one: if (virDomainDeviceInfoIsSet(&def->info, flags) && virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) I know Dan acked this, but since you have a memory leak, and since Osier suggested improvements for 2/2, please send a v5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2013年04月19日 11:19, Eric Blake wrote:
On 04/17/2013 11:40 PM, 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".
+static virDomainNVRAMDefPtr +virDomainNVRAMDefParseXML(const xmlNodePtr node, + unsigned int flags) Alignment. The 'c' of const and 'u' of unsigned should be in the same
column:
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 ) Style: no spaces inside () and the expression it contains. This should be:
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
+ return NULL; Memory leak. If the parse fails, you leaked def.
@@ -14159,6 +14216,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, }
static int +virDomainNVRAMDefFormat(virBufferPtr buf, + virDomainNVRAMDefPtr def, + unsigned int flags) Another case of incorrect indentation.
+{ + virBufferAsprintf(buf, " <nvram>\n"); Use virBufferAddLit when there is nothing to be formatted (reserve virBufferAsprintf for use of "%" sequences).
+ if (virDomainDeviceInfoIsSet(&def->info, flags)) + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) You could combine these two into one:
if (virDomainDeviceInfoIsSet(&def->info, flags) && virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
I know Dan acked this, but since you have a memory leak, and since Osier suggested improvements for 2/2, please send a v5.
Sure.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Li Zhang -
Osier Yang