[libvirt] [PATCH] Added QEMU support for IVSHMEM devices
Osier Yang
jyang at redhat.com
Tue Oct 30 07:38:45 UTC 2012
On 2012年09月24日 23:44, Shawn Furrow wrote:
> *Created at Virginia Tech's Systems Software Research Group
>
> This patch adds the XML schema and implementation for IVSHMEM device driver
> support. Currently it defaults to using interrupts. A sample IVSHMEM entry
> in the VM's XML file is:
>
> <ivshmem id='nahanni' size='16834' path='/tmp/'/>
I'm obviously late, but better than no news. :-)
>
> ---
> docs/schemas/domaincommon.rng | 17 +++++
> src/conf/domain_conf.c | 152 ++++++++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 16 ++++-
> src/qemu/qemu_command.c | 75 ++++++++++++++++++++
> src/qemu/qemu_command.h | 6 ++
> 5 files changed, 264 insertions(+), 2 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f47fdad..ddf8eb1 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2576,6 +2576,23 @@
> </optional>
> </element>
> </define>
> +<define name="ivshmem">
> +<element name="ivshmem">
> +<attribute name="id">
> +<ref name="deviceName">
> +</attribute>
> +<optional>
> +<attribute name="size">
> +<ref name="memoryKB">
> +</attribute>
> +</optional>
> +<optional>
> +<attribute name="path">
> +<ref name="filePath">
> +</attribute>
> +</optional>
> +</element>
I think it makes sense to introduce a "memory" device, (assuming
there might be other memory related device in future). And
"ivshmem" or "shared" as a model of it. How about something
like below:
<device>
<memory mode='shared' name='ivshmemtest'>
<size unit='KiB'>10240</size>
<vectors>3</vectors>
<role mode='master'/>
<msi/>
<ioeventfd/>
<server name='shmem' path='/tmp/shmem'/>
</memory>
</device>
> +</define>
> <define name="parallel">
> <element name="parallel">
> <ref name="qemucdev"/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4aa08d0..0cd2f98 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -156,7 +156,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
> "redirdev",
> "smartcard",
> "chr",
> - "memballoon")
> + "memballoon",
> + "ivshmem")
>
> VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST,
> "none",
> @@ -1374,6 +1375,16 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def)
> VIR_FREE(def);
> }
>
> +void virDomainIvshmemDefFree(virDomainIvshmemDefPtr def)
> +{
> + if (!def)
> + return;
> +
> + virDomainDeviceInfoClear(&def->info);
> +
> + VIR_FREE(def);
> +}
> +
> void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def)
> {
> if (!def)
> @@ -1707,6 +1718,8 @@ void virDomainDefFree(virDomainDefPtr def)
>
> virDomainMemballoonDefFree(def->memballoon);
>
> + virDomainIvshmemDefFree(dev->ivshmem);
> +
> for (i = 0; i< def->nseclabels; i++)
> virSecurityLabelDefFree(def->seclabels[i]);
> VIR_FREE(def->seclabels);
> @@ -2136,6 +2149,12 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def,
> if (cb(def,&device,&def->memballoon->info, opaque)< 0)
> return -1;
> }
> + if (def->ivshmem) {
> + device.type = VIR_DOMAIN_DEVICE_IVSHMEM;
> + device.data.ivshmem = def->ivshmem;
> + if (cb(def,&device,&def->ivshmem->info, opaque)< 0)
> + return -1;
> + }
> device.type = VIR_DOMAIN_DEVICE_HUB;
> for (i = 0; i< def->nhubs ; i++) {
> device.data.hub = def->hubs[i];
> @@ -6935,6 +6954,69 @@ error:
> goto cleanup;
> }
>
> +static virDomainIvshmemDefPtr
> +virDomainIvshmemDefParseXML(const xmlNodePtr node,
> + unsigned int flags)
> +{
> + char *id = NULL;
> + char *size = NULL;
> + char *path = NULL;
> + virDomainIvshmemDefPtr def;
> +
> + if (VIR_ALLOC(def)< 0) {
> + virReportOOMError();
> + return NULL;
> + }
> +
> + id = virXMLPropString(node, "id");
> + VIR_DEBUG("ivshmem: id = '%s'", id);
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("ivshmem id=' %s'"), id);
Why to report an error here?
> + if (id == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("ERROR: ivshmem, id not defined' %s'"), id);
As 'id' is NULL, including it as part of the error doesn't help.
And the error type should be VIR_ERR_XML_ERROR.
> + goto error;
> + }
> +
> + def->id = id;
> + size = virXMLPropString(node, "size");
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("ivshmem size=' %s'"), size);
Again, why to report an error here?
> +
> + VIR_DEBUG("ivshmem: size = '%s'", size);
> + if (size) {
> + if (virStrToLong_i(size, NULL, 10,&def->size)< 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse ivshmem size %s"), size);
s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_XML_ERROR/,
> + VIR_FREE(size);
> + goto error;
> + }
> + } else {
> + def->size = 16834;
Better to use a MACRO for the default value. And there should be
doc for the default value.
> + }
> +
> + path = virXMLPropString(node, "path");
> + VIR_DEBUG("ivshmem: path = '%s'", path);
> + if (path == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("ERROR: ivshmem, path not defined' %s'"), path);
s/ERROR//,
And likewise, 'path' is no need in the output.
> + goto error;
> + }
> + def->path = path;
I'd write the codes like:
if (!(def->path = virXMLPropString(node, "path"))) {
virReportError(VIR_ERR_XML_ERROR,
_("ivshmem path must be specified"));
goto cleanup;
}
This applies for the previous "id" parsing too. And Variables
"id" and "path" can be removed then.
Another problem is, from the rng, 'path' is optional. But
here it's required as a force.
> +
> + if (virDomainDeviceInfoParseXML(node, NULL,&def->info, flags)< 0)
> + goto error;
> +
> +cleanup:
> + VIR_FREE(size);
> + return def;
> +
> +error:
> + virDomainIvshmemDefFree(def);
> + def = NULL;
> + goto cleanup;
I think it's compact if rewriting above 2 lables like:
VIR_FREE(size);
return def;
cleanup:
virDomainIvshmemDefFree(def);
VIR_FREE(size);
return NULL;
And thus you can use 'goto cleanup' across the function.
> +}
> +
> static virSysinfoDefPtr
> virSysinfoParseXML(const xmlNodePtr node,
> xmlXPathContextPtr ctxt)
> @@ -9742,6 +9824,28 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> }
> }
>
> + /* analysis of the ivshmem devices */
> + def->ivshmem = NULL;
> + if ((n = virXPathNodeSet("./devices/ivshmem", ctxt,&nodes))< 0) {
> + goto error;
> + }
> +
> + if (n> 0) {
Should we error out if more than 1 <ivshmem> is specified in the XML?
> + virDomainIvshmemDefPtr ivshmem =
> + virDomainIvshmemDefParseXML(nodes[0], flags);
> + if (!ivshmem)
> + goto error;
> +
> + def->ivshmem = ivshmem;
Again I'd like write the codes like:
if (!(def->ivshmem = virDomainIvshmemDefParseXML(nodes[0], flags)))
goto error;
> + VIR_FREE(nodes);
> + } else if (def->virtType != VIR_DOMAIN_VIRT_QEMU) {
We should just error out if the domain type is not 'qemu' then.
> + /* TODO: currently ivshmem only on QEMU */
> + virDomainIvshmemDefPtr ivshmem;
> + if (VIR_ALLOC(ivshmem)< 0)
> + goto no_memory;
> + def->ivshmem = 0;
> + }
> +
> /* analysis of the hub devices */
> if ((n = virXPathNodeSet("./devices/hub", ctxt,&nodes))< 0) {
> goto error;
> @@ -12673,6 +12777,49 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
> return 0;
> }
>
> +
> +static int
> +virDomainIvshmemDefFormat(virBufferPtr buf,
> + virDomainIvshmemDefPtr def,
> + unsigned int flags)
Indention problems.
> +{
> + const char *id = def->id;
> + const char *path = def->path;
> +
> + if (!id) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected ivshmem id = %s"), def->id);
> + return -1;
> + }
> + if (!def->size) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected ivshmem size = %d"), def->size);
> + return -1;
> + }
> +
> + if (!path) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected ivshmem id = %s"), def->path);
> + return -1;
> + }
No need to do these checkings, as the parsing already ensured there
are good.
> +
> + virBufferAsprintf(buf, "<ivshmem id='%s'", id);
> + virBufferAsprintf(buf, " size='%d'", def->size);
> + virBufferAsprintf(buf, " path='%s'", path);
If the rng is right, then it's wrong to always output 'path' here.
> +
> + if (virDomainDeviceInfoIsSet(&def->info, flags)) {
> + virBufferAddLit(buf, ">\n");
> + if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0)
> + return -1;
> + virBufferAddLit(buf, "</ivshmem>\n");
> + } else {
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> virDomainSysinfoDefFormat(virBufferPtr buf,
> virSysinfoDefPtr def)
> @@ -13828,6 +13975,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> if (def->memballoon)
> virDomainMemballoonDefFormat(buf, def->memballoon, flags);
>
> + if (def->ivshmem)
> + virDomainIvshmemDefFormat(buf, def->ivshmem, flags);
> +
> virBufferAddLit(buf, "</devices>\n");
>
> virBufferAdjustIndent(buf, 2);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1a61318..78bdf84 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -114,6 +114,9 @@ typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
> typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList;
> typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr;
>
> +typedef struct _virDomainIvshmemDef virDomainIvshmemDef;
> +typedef virDomainIvshmemDef *virDomainIvshmemDefPtr;
> +
> /* Flags for the 'type' field in virDomainDeviceDef */
> typedef enum {
> VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -133,6 +136,7 @@ typedef enum {
> VIR_DOMAIN_DEVICE_SMARTCARD,
> VIR_DOMAIN_DEVICE_CHR,
> VIR_DOMAIN_DEVICE_MEMBALLOON,
> + VIR_DOMAIN_DEVICE_IVSHMEM,
>
> VIR_DOMAIN_DEVICE_LAST
> } virDomainDeviceType;
> @@ -157,7 +161,8 @@ struct _virDomainDeviceDef {
> virDomainRedirdevDefPtr redirdev;
> virDomainSmartcardDefPtr smartcard;
> virDomainChrDefPtr chr;
> - virDomainMemballoonDefPtr memballoon;
> + virDomainMemballoonDefPtr memballoon,
> + virDomainIvshmemDefPtr ivshmem;
> } data;
> };
>
> @@ -1344,6 +1349,12 @@ struct _virDomainMemballoonDef {
> virDomainDeviceInfo info;
> };
>
> +struct _virDomainIvshmemDef {
> + char *id;
> + int size;
> + char *path;
> + virDomainDeviceInfo info;
> +};
>
> enum virDomainSmbiosMode {
> VIR_DOMAIN_SMBIOS_NONE,
> @@ -1752,6 +1763,7 @@ struct _virDomainDef {
> /* Only 1 */
> virDomainWatchdogDefPtr watchdog;
> virDomainMemballoonDefPtr memballoon;
> + virDomainIvshmemDefPtr ivshmem;
> virCPUDefPtr cpu;
> virSysinfoDefPtr sysinfo;
> virDomainRedirFilterDefPtr redirfilter;
> @@ -1859,6 +1871,7 @@ int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src,
> void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def);
> void virDomainSoundDefFree(virDomainSoundDefPtr def);
> void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def);
> +void virDomainIvshmemDefFree(virDomainIvshmemDefPtr def);
Is virDomainIvshmemDefFree used by external fucntions? if not,
it should be static. And no need to declare it here.
If it's used by some external functions, you should expost it
to the private symbols (libvirt_private.syms)
> void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def);
> void virDomainVideoDefFree(virDomainVideoDefPtr def);
> virDomainHostdevDefPtr virDomainHostdevDefAlloc(void);
> @@ -2194,6 +2207,7 @@ VIR_ENUM_DECL(virDomainSoundCodec)
> VIR_ENUM_DECL(virDomainSoundModel)
> VIR_ENUM_DECL(virDomainMemDump)
> VIR_ENUM_DECL(virDomainMemballoonModel)
> +VIR_ENUM_DECL(virDomainIvshmemModel)
> VIR_ENUM_DECL(virDomainSmbiosMode)
> VIR_ENUM_DECL(virDomainWatchdogModel)
> VIR_ENUM_DECL(virDomainWatchdogAction)
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e7bb88e..6c66075 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -770,6 +770,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, qemuCapsPtr caps)
> if (virAsprintf(&def->memballoon->info.alias, "balloon%d", 0)< 0)
> goto no_memory;
> }
> + if (def->ivshmem) {
> + if (virAsprintf(&def->ivshmem->info.alias, "ivshmem%d", 0)< 0)
> + goto no_memory;
> + }
>
> return 0;
>
> @@ -3245,6 +3249,58 @@ error:
> return NULL;
> }
>
> +//adds the options for the "-device" portion of QEMU command line for ivshmem
It's not recommend to comment using "//" in libvirt.
> +char *
> +qemuBuildIvshmemDevStr(virDomainIvshmemDefPtr dev,
> + qemuCapsPtr caps)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
Indention problem.
> +
> + virBufferAddLit(&buf, "ivshmem");
> + virBufferAsprintf(&buf, ",chardev=%s", dev->id);
> + virBufferAsprintf(&buf, ",size=%dm", dev->size/1024);
There is macro for division: VIR_DIV_UP
> + virBufferAsprintf(&buf, ",ioeventfd=on");
> + virBufferAsprintf(&buf, ",vectors=8");
Why not to expot 'vectors' as another attribute? It's not
nice to hardcode here.
> + //virBufferAsprintf(&buf, ",shm=%s", dev->id);
> + if (qemuBuildDeviceAddressStr(&buf,&dev->info, caps)< 0)
> + goto error;
> +
> + if (virBufferError(&buf)) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + return virBufferContentAndReset(&buf);
> +
> +error:
> + virBufferFreeAndReset(&buf);
> + return NULL;
> +}
> +
> +//adds the options for the "-chardev" portion of QEMU command line for ivshmem
> +char *
> +qemuBuildIvshmemCharDevStr(virDomainIvshmemDefPtr dev,
> + qemuCapsPtr caps)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferAddLit(&buf, "socket");
> + virBufferAsprintf(&buf, ",id=%s", dev->id);
> + virBufferAsprintf(&buf, ",path=%s%s", dev->path,dev->id);
> + if (qemuBuildDeviceAddressStr(&buf,&dev->info, caps)< 0)
> + goto error;
> +
> + if (virBufferError(&buf)) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + return virBufferContentAndReset(&buf);
> +
> +error:
> + virBufferFreeAndReset(&buf);
> + return NULL;
> +}
>
> char *
> qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
> @@ -6582,6 +6638,25 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> }
>
> + // adds ivshmem QEMU command line entries
> + if ((def->ivshmem)&& (def->ivshmem->id != NULL)) {
ivshmem->id can't be NULL. It's redundant here.
> + char *optstr;
> + virCommandAddArg(cmd, "-chardev");
> + optstr = qemuBuildIvshmemCharDevStr(def->ivshmem, caps);
> + if (!optstr)
> + goto error;
Again, it's desired to write the codes like:
if (!(optstr = qemuBuildIvshmemCharDevStr(def->ivshmem, caps))))
goto error;
> + virCommandAddArg(cmd, optstr);
> +
> + optstr = NULL;
> +
> + virCommandAddArg(cmd, "-device");
> + optstr = qemuBuildIvshmemDevStr(def->ivshmem, caps);
> + if (!optstr)
> + goto error;
Likewise.
> + virCommandAddArg(cmd, optstr);
> + VIR_FREE(optstr);
> + }
> +
> if (snapshot)
> virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
>
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 939833d..80e7565 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -118,6 +118,12 @@ char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev,
> char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev,
> qemuCapsPtr caps);
>
> +char * qemuBuildIvshmemDevStr(virDomainIvshmemDefPtr dev,
> + qemuCapsPtr caps);
> +
> +char * qemuBuildIvshmemCharDevStr(virDomainIvshmemDefPtr dev,
> + qemuCapsPtr caps);
> +
> char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev,
> qemuCapsPtr caps);
>
More information about the libvir-list
mailing list