[libvirt] [PATCH] Added QEMU support for IVSHMEM devices
*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/'/> --- 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> + </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); + if (id == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ERROR: ivshmem, id not defined' %s'"), id); + goto error; + } + + def->id = id; + size = virXMLPropString(node, "size"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ivshmem size=' %s'"), size); + + 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); + VIR_FREE(size); + goto error; + } + } else { + def->size = 16834; + } + + 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); + goto error; + } + def->path = path; + + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + goto error; + +cleanup: + VIR_FREE(size); + return def; + +error: + virDomainIvshmemDefFree(def); + def = NULL; + goto cleanup; +} + 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) { + virDomainIvshmemDefPtr ivshmem = + virDomainIvshmemDefParseXML(nodes[0], flags); + if (!ivshmem) + goto error; + + def->ivshmem = ivshmem; + VIR_FREE(nodes); + } else if (def->virtType != VIR_DOMAIN_VIRT_QEMU) { + /* 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) +{ + 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; + } + + virBufferAsprintf(buf, " <ivshmem id='%s'", id); + virBufferAsprintf(buf, " size='%d'", def->size); + virBufferAsprintf(buf, " path='%s'", path); + + 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); 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 +char * +qemuBuildIvshmemDevStr(virDomainIvshmemDefPtr dev, + qemuCapsPtr caps) +{ +virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "ivshmem"); + virBufferAsprintf(&buf, ",chardev=%s", dev->id); + virBufferAsprintf(&buf, ",size=%dm", dev->size/1024); + virBufferAsprintf(&buf, ",ioeventfd=on"); + virBufferAsprintf(&buf, ",vectors=8"); + //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)) { + char *optstr; + virCommandAddArg(cmd, "-chardev"); + optstr = qemuBuildIvshmemCharDevStr(def->ivshmem, caps); + if (!optstr) + goto error; + virCommandAddArg(cmd, optstr); + + optstr = NULL; + + virCommandAddArg(cmd, "-device"); + optstr = qemuBuildIvshmemDevStr(def->ivshmem, caps); + if (!optstr) + goto error; + 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); -- 1.7.0.4
On 09/24/2012 09:44 AM, Shawn Furrow wrote: [sorry for my delay in replying]
*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:
Thanks for starting to tackle this.
<ivshmem id='nahanni' size='16834' path='/tmp/'/>
Elsewhere, we have represented memory sized in KiB (1024 byte units); what scale are you using here? A default unit of byte might be nicer, on the other hand, does shared memory have to be page aligned, at which point it will always be a multiple of 4k (and thus listing in KiB still makes sense)? At any rate, I think that this argues you need to support units on output to show the preferred scale, as well as parse it on input to allow users to specify units='M' size='1' to reserve 2**20 bytes with ease.
--- docs/schemas/domaincommon.rng | 17 +++++
Incomplete without also patching docs/formatdomain.html.in to describe the new element. I'll go ahead and review the code, but cannot apply this without documentation.
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>
For ease of parsing (that is, for purposes of code reuse), I would suggest listing size as a sub-element rather than an attribute: <ivshmem name='...' path='...'> <size units='KiB'>nnn</size> </ivshmem>
+ <optional> + <attribute name="path"> + <ref name="filePath"> + </attribute> + </optional> + </element> + </define> <define name="parallel"> <element name="parallel"> <ref name="qemucdev"/>
+void virDomainIvshmemDefFree(virDomainIvshmemDefPtr def)
Style - newline after the return type so that the function name begins in column 1. Also, can this be static?
+{ + if (!def) + return; + + virDomainDeviceInfoClear(&def->info); + + VIR_FREE(def);
Memory leak of def->id and def->path.
+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);
Alignment is off.
+ if (id == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ERROR: ivshmem, id not defined' %s'"), id); + goto error; + } + + def->id = id; + size = virXMLPropString(node, "size"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("ivshmem size=' %s'"), size);
And again.
+ + 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); + VIR_FREE(size); + goto error; + }
What happens if size is not a multiple of page size? Is that an error, or does it get rounded up?
+ } else { + def->size = 16834;
Why 16k? Where is this default documented?
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) { + virDomainIvshmemDefPtr ivshmem = + virDomainIvshmemDefParseXML(nodes[0], flags); + if (!ivshmem) + goto error; + + def->ivshmem = ivshmem; + VIR_FREE(nodes); + } else if (def->virtType != VIR_DOMAIN_VIRT_QEMU) { + /* TODO: currently ivshmem only on QEMU */
Don't do this. The code in domain_conf.c is supposed to be driver-agnostic. If anything, the right way to reject <ivshmem> would be to add a new capability in virCapsPtr, and set that capability only on qemu, and reject the device if the capability is not present. Then, when other drivers add support for ivshmem (I imagine LXC might be a good candidate for this, by using a POSIX shared memory region), then that driver sets the capability bit, and you don't have to revisit this code.
+ +static int +virDomainIvshmemDefFormat(virBufferPtr buf, + virDomainIvshmemDefPtr def, + unsigned int flags)
Alignment.
+{ + 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; + }
In our formatters, we can generally assume that the struct is properly populated, and therefore these paranoia checks do not need to be preformed.
+ virBufferAsprintf(buf, " <ivshmem id='%s'", id); + virBufferAsprintf(buf, " size='%d'", def->size);
If you don't take my advice of making size a subelement, then these two lines can be merged.
+ virBufferAsprintf(buf, " path='%s'", path);
path is arbitrary user text, and therefore needs xml escaping; you must use virBufferEscapeString here.
+ + if (virDomainDeviceInfoIsSet(&def->info, flags)) { + virBufferAddLit(buf, ">\n"); + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1;
How is the shared memory presented to the guest (that is, what will virDomainDeviceInfoFormat output here, if it is set)? Does a qemu guest see it as a PCI device, or what?
+struct _virDomainIvshmemDef { + char *id; + int size;
int is probably the wrong type, since it is feasible to have a shared memory region larger than 4GB. You also ought to document what units this is stored in (I recommend bytes internally, even if you decide to default the XML to KiB for consistency with other memory descriptions already present).
@@ -2194,6 +2207,7 @@ VIR_ENUM_DECL(virDomainSoundCodec) VIR_ENUM_DECL(virDomainSoundModel) VIR_ENUM_DECL(virDomainMemDump) VIR_ENUM_DECL(virDomainMemballoonModel) +VIR_ENUM_DECL(virDomainIvshmemModel)
Where do you define this enum? I don't think you need this line.
+++ b/src/qemu/qemu_command.c
+//adds the options for the "-device" portion of QEMU command line for ivshmem
C89 comments, please (/* */, not //)
+char * +qemuBuildIvshmemDevStr(virDomainIvshmemDefPtr dev, + qemuCapsPtr caps) +{ +virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "ivshmem"); + virBufferAsprintf(&buf, ",chardev=%s", dev->id); + virBufferAsprintf(&buf, ",size=%dm", dev->size/1024);
Yuck, this rounds. I'd rather pass this information in bytes than in MiB, to ensure that we are as faithful to the user's request as possible.
+ virBufferAsprintf(&buf, ",ioeventfd=on"); + virBufferAsprintf(&buf, ",vectors=8");
You can cram these into a single virBufferAsprintf call: virBufferAsprintf(&buf, "ivshmem,chardev=%s,size=%dm,ioeventfd=on,vectors=8", dev->id, dev->size/1024); Also, does ioeventfd or vectors need to be configurable in the XML?
+ //virBufferAsprintf(&buf, ",shm=%s", dev->id);
Leftover debugging?
+ +//adds the options for the "-chardev" portion of QEMU command line for ivshmem
Again, C89 comment.
+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);
Again, these can be combined.
@@ -6582,6 +6638,25 @@ qemuBuildCommandLine(virConnectPtr conn, } }
+ // adds ivshmem QEMU command line entries
C89 comment. Also, it would help to add tests to tests/qemuxml2argvdata to prove that you map the new XML to the proper command line (and that the new XML is valid per the RNG).
+ if ((def->ivshmem) && (def->ivshmem->id != NULL)) { + char *optstr; + virCommandAddArg(cmd, "-chardev");
-chardev was not always available in older qemu. You probably also need a patch to qemu_capabilities.[hc] to detect whether qemu is new enough to support a shared memory device, so that you can give a graceful error message here when it is not supported (rather than a cryptic message from qemu saying that the command line could not be parsed). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Oct 04, 2012 at 09:53:49AM -0600, Eric Blake wrote:
On 09/24/2012 09:44 AM, Shawn Furrow wrote:
[sorry for my delay in replying]
*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:
Thanks for starting to tackle this.
<ivshmem id='nahanni' size='16834' path='/tmp/'/>
Elsewhere, we have represented memory sized in KiB (1024 byte units); what scale are you using here? A default unit of byte might be nicer, on the other hand, does shared memory have to be page aligned, at which point it will always be a multiple of 4k (and thus listing in KiB still makes sense)? At any rate, I think that this argues you need to support units on output to show the preferred scale, as well as parse it on input to allow users to specify units='M' size='1' to reserve 2**20 bytes with ease.
I think it'd be worthwhile to push some of the attributes down to a subelement, eg <ivshmem id='nahanni'> <source size="12345" units="M" path='/tmp'/> </ivshmem> 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 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);
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang -
Shawn Furrow