[libvirt] [PATCH v1 0/6] ivshmem support
The following patches are an implementation of a new shmem device to support ivshmem in libvirt. Any feedback is welcome. Changes since RFC: - replace <ivshmem> by a more generic term <shmem> - remove role attribute - libvirt is able to start and stop an ivshmem server - update xml format from: <ivshmem use_server='yes' role='master'> <source file='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </ivshmem> to: <shmem name='shmem0' model='ivshmem'> <server path='/tmp/sockect-ivshem0' start='yes'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem> Maxime Leroy (6): doc: schema: Add documentation for the shmem support conf: Parse and format shmem device XML qemu: Add cap flag QEMU_CAPS_IVSHMEM qemu: Build command line for ivshmem device tests: Add tests for ivshmem device handling ivshmem: add start param to server attribute configure.ac | 4 + docs/formatdomain.html.in | 71 ++++++ docs/schemas/domaincommon.rng | 52 +++++ po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 264 ++++++++++++++++++++++- src/conf/domain_conf.h | 42 ++++ src/libvirt_private.syms | 7 + src/qemu/qemu_capabilities.c | 3 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 112 +++++++++- src/qemu/qemu_command.h | 4 + src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_process.c | 10 + src/util/virivshmemserver.c | 141 ++++++++++++ src/util/virivshmemserver.h | 28 +++ tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemuhelptest.c | 17 +- tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args | 10 + tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml | 35 +++ tests/qemuxml2argvtest.c | 3 + tests/qemuxml2xmltest.c | 2 + 27 files changed, 808 insertions(+), 7 deletions(-) create mode 100644 src/util/virivshmemserver.c create mode 100644 src/util/virivshmemserver.h create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml -- 1.9.3
This patch documents XML elements used for support of shmem devices. Considering there could be other shared memory related devices, this patch introduces a general device model, called shmem device. Currenly "ivshmem" is the only shared memory model supported. About ivshmem, please see the following documentation: http://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/ivshmem_device_spec.txt (Ivshmem documentation for qemu will be updated soon: http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01244.html) In the devices section in the domain XML users may specify: - For ivshmem device using an ivshmem server: <shmem name='shmem0' model='ivshmem'> <source file='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem> - For ivshmem device not using an ivshmem server: <shmem name='ivshmem1' model='ivshmem'> <size unit='M'>32</size> </shmem> Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- docs/formatdomain.html.in | 68 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 49 +++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ed17389..c7644bc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5422,6 +5422,74 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> + <h4><a name="elementsShmem">Shmem device</a></h4> + + <p> + A shmem (i.e. shared memory) device allows to share a memory + region between different virtual machines and the host. + </p> + +<pre> + ... + <devices> + <shmem name='shmem0' model='ivshmem'/> + <shmem name='shmem1' model='ivshmem'/> + </devices> + ...</pre> + + <dl> + <dt><code>shmem</code></dt> + <dd> + The <code>shmem</code> element has one mandatory attribute, + <code>name</code> to identify the shared memory. The optional element + <code>model</code> specify the type of shared memory. Only one mode of + shared-memory is currently supported: <code>ivshmem</code>. + </dd> + </dl> + + <h5><a name="elementsIvshmem">Ivshmem model</a></h5> + + <p> + An ivshmem (i.e. Inter-VM shared memory) device allows to share a memory + region (created on the host via the POSIX shared memory API) between + multiple QEMU processes running different guests. + For more information, please see the + <a href="http://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/ivshmem_device_spec.txt"> + ivshmem documentation of qemu</a>. + </p> + +<pre> + ... + <devices> + <shmem name='shmem0' model='ivshmem'> + <server path='/tmp/socket-shmem0'/> + <size unit='M'>32</size> + <msi vectors='32' ioeventfd='on'/> + </shmem> + <shmem name='shmem1' model='ivshmem'> + <size unit='M'>32</size> + </shmem> + </devices> + ...</pre> + + <dl> + <dt><code>server</code></dt> + <dd>The optional <code>server</code> element can be used to configure an + ivshmem device connected to the ivshmem server via a unix socket. The + <code>path</code> attribute specifies the path to the unix socket. + </dd> + <dt><code>size</code></dt> + <dd>The optional <code>size</code> element specifies the size of the shared memory. + </dd> + <dt><code>msi</code></dt> + <dd>The optional <code>msi</code> element allows to enable MSI interrupts. + This option can only be used with the <code>server</code> attribute. + The <code>vectors</code> attribute can be used to specify the number of + interrupt vectors. The <code>ioeventd</code> attribute allows to enable + ioeventfd. + </dd> + </dl> + <h3><a name="seclabel">Security label</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 033f2f6..64abf2b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3221,6 +3221,54 @@ </optional> </element> </define> + + <define name="shmem"> + <element name="shmem"> + <attribute name="name"/> + <optional> + <ref name="ivshmem"/> + </optional> + <optional> + <ref name="alias"/> + </optional> + <optional> + <ref name="address"/> + </optional> + </element> + </define> + + <define name="ivshmem"> + <group> + <attribute name="model"> + <value>ivshmem</value> + </attribute> + <interleave> + <optional> + <element name="server"> + <attribute name="path"/> + </element> + </optional> + <optional> + <element name="msi"> + <optional> + <ref name="ioeventfd"/> + </optional> + <optional> + <attribute name="vectors"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional> + <optional> + <element name="size"> + <ref name="scaledInteger"/> + </element> + </optional> + </interleave> + </group> + </define> + <define name="memballoon"> <element name="memballoon"> <attribute name="model"> @@ -3807,6 +3855,7 @@ <ref name="redirfilter"/> <ref name="rng"/> <ref name="tpm"/> + <ref name="shmem"/> </choice> </zeroOrMore> <optional> -- 1.9.3
On Fri, Aug 22, 2014 at 12:47:00PM +0200, Maxime Leroy wrote:
This patch documents XML elements used for support of shmem devices.
Considering there could be other shared memory related devices, this patch introduces a general device model, called shmem device. Currenly "ivshmem" is the only shared memory model supported.
Automatically adding model='ivshmem' takes away the generality gained by renaming the device. Especially if it is the only model supported we don't have to put "model" in there. If we expand this to another device/hypervisor, we then might separate the setting by a model and automatically add model='ivshmem', but we might keep it the same if the options will compatible. Until then the model specification just adds unnecessary code.
About ivshmem, please see the following documentation: http://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/ivshmem_device_spec.txt (Ivshmem documentation for qemu will be updated soon: http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01244.html)
In the devices section in the domain XML users may specify:
- For ivshmem device using an ivshmem server:
<shmem name='shmem0' model='ivshmem'> <source file='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem>
From the documentation you've linked I understand it that either the ivshmem has a name *or* it uses a server; and then it doesn't need a name because the server sends an FD to the shared memory using SCM_RIGHTS. If I'm right, documentation and parsing should reflect that. Martin
On Tue, Aug 26, 2014 at 8:22 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:00PM +0200, Maxime Leroy wrote:
[..]
Considering there could be other shared memory related devices, this patch introduces a general device model, called shmem device. Currenly "ivshmem" is the only shared memory model supported.
Automatically adding model='ivshmem' takes away the generality gained by renaming the device. Especially if it is the only model supported we don't have to put "model" in there.
Model is an optional attribute. You can still use this xml: <shmem name="asdf"/>
If we expand this to another device/hypervisor, we then might separate the setting by a model and automatically add model='ivshmem', but we might keep it the same if the options will compatible. Until then the model specification just adds unnecessary code.
It's the same concept from this first implementation of ivshmem: https://www.redhat.com/archives/libvir-list/2012-November/msg00341.html Having a model attribute allow to split properly the code specific to a generic shared memory concept and the code specific to ivshmem. There are pros and cons to have or not a model attribute. Anyway, let keep the code simple by removing the model attribute.
About ivshmem, please see the following documentation:
http://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/ivshmem_device_spec.txt (Ivshmem documentation for qemu will be updated soon: http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01244.html)
In the devices section in the domain XML users may specify:
- For ivshmem device using an ivshmem server:
<shmem name='shmem0' model='ivshmem'> <source file='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem>
From the documentation you've linked I understand it that either the ivshmem has a name *or* it uses a server; and then it doesn't need a name because the server sends an FD to the shared memory using SCM_RIGHTS.
The name is not needed if libvirt doesn't start the ivshmem server. In this case, it could be optional. Maxime
This patch adds configuration support for the shmem device as described in the schema in the previous patch. Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- src/conf/domain_conf.c | 249 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 41 ++++++++ src/libvirt_private.syms | 2 + 3 files changed, 291 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9557020..08d653a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -234,7 +234,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "chr", "memballoon", "nvram", - "rng") + "rng", + "shmem") VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "none", @@ -759,6 +760,9 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST, "abort", "pivot") +VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, + "ivshmem") + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob) @@ -1692,6 +1696,26 @@ void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def) VIR_FREE(def); } +void virDomainShmemDefFree(virDomainShmemDefPtr def) +{ + if (!def) + return; + + switch (def->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + VIR_FREE(def->data.ivshmem.server.path); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected shmem model %d"), def->model); + } + + virDomainDeviceInfoClear(&def->info); + + VIR_FREE(def->name); + VIR_FREE(def); +} + void virDomainVideoDefFree(virDomainVideoDefPtr def) { if (!def) @@ -1893,6 +1917,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_NVRAM: virDomainNVRAMDefFree(def->data.nvram); break; + case VIR_DOMAIN_DEVICE_SHMEM: + virDomainShmemDefFree(def->data.shmem); + break; case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_NONE: break; @@ -2134,6 +2161,10 @@ void virDomainDefFree(virDomainDefPtr def) virDomainRedirFilterDefFree(def->redirfilter); + for (i = 0; i < def->nshmems; i++) + virDomainShmemDefFree(def->shmems[i]); + VIR_FREE(def->shmems); + if (def->namespaceData && def->ns.free) (def->ns.free)(def->namespaceData); @@ -2568,6 +2599,8 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) return &device->data.memballoon->info; case VIR_DOMAIN_DEVICE_NVRAM: return &device->data.nvram->info; + case VIR_DOMAIN_DEVICE_SHMEM: + return &device->data.shmem->info; case VIR_DOMAIN_DEVICE_RNG: return &device->data.rng->info; @@ -2783,6 +2816,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, if (cb(def, &device, &def->hubs[i]->info, opaque) < 0) return -1; } + device.type = VIR_DOMAIN_DEVICE_SHMEM; + for (i = 0; i < def->nshmems; i++) { + device.data.shmem = def->shmems[i]; + if (cb(def, &device, &def->shmems[i]->info, opaque) < 0) + return -1; + } /* This switch statement is here to trigger compiler warning when adding * a new device type. When you are adding a new field to the switch you @@ -2809,6 +2848,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_RNG: break; @@ -9462,6 +9502,135 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, return NULL; } +static int +virDomainIvshmemDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainIvshmemDefPtr def) +{ + char *ioeventfd = NULL; + char *vectors = NULL; + xmlNodePtr cur; + xmlNodePtr save = ctxt->node; + int ret; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "server")) { + def->server.enabled = true; + if (!(def->server.path = virXMLPropString(cur, "path"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse <server> 'path' attribute")); + goto error; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "size")) { + if (virDomainParseScaledValue("./size[1]", ctxt, + &def->size, 1, + ULLONG_MAX, true) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse <size> attribute")); + goto error; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "msi")) { + def->msi.enabled = true; + vectors = virXMLPropString(cur, "vectors"); + ioeventfd = virXMLPropString(cur, "ioeventfd"); + + if (vectors && + virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse <msi> 'vectors' attribute '%s'"), + vectors); + goto error; + } + if (ioeventfd && + (def->msi.ioeventfd = + virTristateSwitchTypeFromString(ioeventfd)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse <msi> 'ioeventfd' mode '%s'"), + ioeventfd); + goto error; + } + } + } + cur = cur->next; + } + + /* msi option is only relevant with a server */ + if (def->msi.enabled && !def->server.enabled) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("msi option is only supported with an ivshmem server")); + goto error; + } + + /* size should be a power of two */ + if (def->size & (def->size-1)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem size should be a power of two for ivshmem model")); + goto error; + } + + ret = 0; + cleanup: + ctxt->node = save; + VIR_FREE(ioeventfd); + VIR_FREE(vectors); + return ret; + + error: + ret = -1; + goto cleanup; +} + +static virDomainShmemDefPtr +virDomainShmemDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + char *model = virXMLPropString(node, "model"); + virDomainShmemDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (model) { + if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown <shmem> model '%s'"), model); + goto error; + } + } else + def->model = VIR_DOMAIN_SHMEM_MODEL_IVSHMEM; + + if (!(def->name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<shmem> must contain 'name' attribute")); + goto error; + } + + switch (def->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + if (virDomainIvshmemDefParseXML(node, ctxt, &def->data.ivshmem) < 0) + goto error; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected <shmem> model %d"), def->model); + goto error; + } + + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + goto error; + + cleanup: + VIR_FREE(model); + return def; + error: + virDomainShmemDefFree(def); + def = NULL; + goto cleanup; +} + static virSysinfoDefPtr virSysinfoParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -10318,6 +10487,10 @@ virDomainDeviceDefParse(const char *xmlStr, if (!(dev->data.nvram = virDomainNVRAMDefParseXML(node, flags))) goto error; break; + case VIR_DOMAIN_DEVICE_SHMEM: + if (!(dev->data.shmem = virDomainShmemDefParseXML(node, ctxt, flags))) + goto error; + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LAST: break; @@ -13200,6 +13373,25 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(nodes); } + /* analysis of the shmem devices */ + if ((n = virXPathNodeSet("./devices/shmem", ctxt, &nodes)) < 0) { + goto error; + } + if (n && VIR_ALLOC_N(def->shmems, n) < 0) + goto error; + + node = ctxt->node; + for (i = 0; i < n; i++) { + virDomainShmemDefPtr shmem; + ctxt->node = nodes[i]; + shmem = virDomainShmemDefParseXML(nodes[i], ctxt, flags); + if (!shmem) + goto error; + + def->shmems[def->nshmems++] = shmem; + } + ctxt->node = node; + VIR_FREE(nodes); /* analysis of the user namespace mapping */ if ((n = virXPathNodeSet("./idmap/uid", ctxt, &nodes)) < 0) @@ -16828,6 +17020,56 @@ static int virDomainPanicDefFormat(virBufferPtr buf, return 0; } +static int virDomainIvshmemDefFormat(virBufferPtr buf, + virDomainIvshmemDefPtr def) +{ + if (def->server.enabled) + virBufferAsprintf(buf, "<server path='%s'/>\n", + def->server.path); + if (def->size) + virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", + def->size / (1024 * 1024)); + + if (def->server.enabled && def->msi.enabled) { + virBufferAddLit(buf, "<msi"); + if (def->msi.vectors) + virBufferAsprintf(buf, " vectors='%u'", def->msi.vectors); + if (def->msi.ioeventfd) + virBufferAsprintf(buf, " ioeventfd='%s'", + virTristateSwitchTypeToString(def->msi.ioeventfd)); + virBufferAddLit(buf, "/>\n"); + } + + return 0; +} + +static int virDomainShmemDefFormat(virBufferPtr buf, + virDomainShmemDefPtr def, + unsigned int flags) +{ + virBufferAsprintf(buf, "<shmem name='%s' model='%s'>\n", + def->name, virDomainShmemModelTypeToString(def->model)); + + virBufferAdjustIndent(buf, 2); + switch (def->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virDomainIvshmemDefFormat(buf, &def->data.ivshmem); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected shem model %d"), def->model); + } + + if (virDomainDeviceInfoIsSet(&def->info, flags) && + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</shmem>\n"); + + return 0; +} + static int virDomainRNGDefFormat(virBufferPtr buf, virDomainRNGDefPtr def, @@ -18377,6 +18619,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainPanicDefFormat(buf, def->panic) < 0) goto error; + for (n = 0; n < def->nshmems; n++) + if (virDomainShmemDefFormat(buf, def->shmems[n], flags) < 0) + goto error; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n"); @@ -19742,6 +19988,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_SHMEM: 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 f2df4eb..0c6aa21 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -136,6 +136,12 @@ typedef virDomainPanicDef *virDomainPanicDefPtr; typedef struct _virDomainChrSourceDef virDomainChrSourceDef; typedef virDomainChrSourceDef *virDomainChrSourceDefPtr; +typedef struct _virDomainIvshmemDef virDomainIvshmemDef; +typedef virDomainIvshmemDef *virDomainIvshmemDefPtr; + +typedef struct _virDomainShmemDef virDomainShmemDef; +typedef virDomainShmemDef *virDomainShmemDefPtr; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -157,6 +163,7 @@ typedef enum { VIR_DOMAIN_DEVICE_MEMBALLOON, VIR_DOMAIN_DEVICE_NVRAM, VIR_DOMAIN_DEVICE_RNG, + VIR_DOMAIN_DEVICE_SHMEM, VIR_DOMAIN_DEVICE_LAST } virDomainDeviceType; @@ -184,6 +191,7 @@ struct _virDomainDeviceDef { virDomainMemballoonDefPtr memballoon; virDomainNVRAMDefPtr nvram; virDomainRNGDefPtr rng; + virDomainShmemDefPtr shmem; } data; }; @@ -1370,6 +1378,12 @@ typedef enum { VIR_DOMAIN_HUB_TYPE_LAST } virDomainHubType; +typedef enum { + VIR_DOMAIN_SHMEM_MODEL_IVSHMEM, + + VIR_DOMAIN_SHMEM_MODEL_LAST +} virDomainShmemModel; + typedef struct _virDomainGraphicsListenDef virDomainGraphicsListenDef; typedef virDomainGraphicsListenDef *virDomainGraphicsListenDefPtr; struct _virDomainGraphicsListenDef { @@ -1486,6 +1500,28 @@ struct _virDomainNVRAMDef { virDomainDeviceInfo info; }; +struct _virDomainIvshmemDef { + unsigned long long size; + struct { + bool enabled; + char *path; + } server; + struct { + bool enabled; + unsigned vectors; + virTristateSwitch ioeventfd; + } msi; +}; + +struct _virDomainShmemDef { + int model; /* enum virDomainShmemModel */ + char *name; + union { + virDomainIvshmemDef ivshmem; + } data; + virDomainDeviceInfo info; +}; + typedef enum { VIR_DOMAIN_SMBIOS_NONE = 0, VIR_DOMAIN_SMBIOS_EMULATE, @@ -2007,6 +2043,9 @@ struct _virDomainDef { size_t nrngs; virDomainRNGDefPtr *rngs; + size_t nshmems; + virDomainShmemDefPtr *shmems; + /* Only 1 */ virDomainWatchdogDefPtr watchdog; virDomainMemballoonDefPtr memballoon; @@ -2204,6 +2243,7 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def); +void virDomainShmemDefFree(virDomainShmemDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, const virDomainDef *def, @@ -2628,6 +2668,7 @@ VIR_ENUM_DECL(virDomainGraphicsVNCSharePolicy) VIR_ENUM_DECL(virDomainHyperv) VIR_ENUM_DECL(virDomainRNGModel) VIR_ENUM_DECL(virDomainRNGBackend) +VIR_ENUM_DECL(virDomainShmemModel) VIR_ENUM_DECL(virDomainTPMModel) VIR_ENUM_DECL(virDomainTPMBackend) /* from libvirt.h */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e09ddd5..f86926e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -373,6 +373,8 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemModelTypeFromString; +virDomainShmemModelTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; -- 1.9.3
On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:
This patch adds configuration support for the shmem device as described in the schema in the previous patch.
Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- src/conf/domain_conf.c | 249 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 41 ++++++++ src/libvirt_private.syms | 2 + 3 files changed, 291 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9557020..08d653a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -9462,6 +9502,135 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, return NULL; }
+static int +virDomainIvshmemDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virDomainIvshmemDefPtr def) +{ + char *ioeventfd = NULL; + char *vectors = NULL; + xmlNodePtr cur; + xmlNodePtr save = ctxt->node; + int ret; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "server")) { + def->server.enabled = true; + if (!(def->server.path = virXMLPropString(cur, "path"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse <server> 'path' attribute")); + goto error; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "size")) { + if (virDomainParseScaledValue("./size[1]", ctxt, + &def->size, 1, + ULLONG_MAX, true) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot parse <size> attribute")); + goto error; + }
This parsing using loop over children does not readably (and in this case neither reliably) check whether required options are present. Currently it parses <shmem name="asdf"/> as valid, but not specifying the size is probably not what you want on qemu command-line ... [1]
+ } else if (xmlStrEqual(cur->name, BAD_CAST "msi")) { + def->msi.enabled = true; + vectors = virXMLPropString(cur, "vectors"); + ioeventfd = virXMLPropString(cur, "ioeventfd"); + + if (vectors && + virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors) < 0) {
Use *_uip() if you don't want to accept negative values.
+ virReportError(VIR_ERR_XML_ERROR, + _("cannot parse <msi> 'vectors' attribute '%s'"), + vectors); + goto error; + } + if (ioeventfd && + (def->msi.ioeventfd = + virTristateSwitchTypeFromString(ioeventfd)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse <msi> 'ioeventfd' mode '%s'"), + ioeventfd); + goto error; + } + } + } + cur = cur->next; + } + + /* msi option is only relevant with a server */ + if (def->msi.enabled && !def->server.enabled) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("msi option is only supported with an ivshmem server")); + goto error; + } + + /* size should be a power of two */ + if (def->size & (def->size-1)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem size should be a power of two for ivshmem model")); + goto error; + } + + ret = 0; + cleanup: + ctxt->node = save; + VIR_FREE(ioeventfd); + VIR_FREE(vectors); + return ret; + + error: + ret = -1; + goto cleanup; +} + +static virDomainShmemDefPtr +virDomainShmemDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + char *model = virXMLPropString(node, "model"); + virDomainShmemDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (model) { + if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown <shmem> model '%s'"), model); + goto error; + } + } else + def->model = VIR_DOMAIN_SHMEM_MODEL_IVSHMEM; +
As I said in 1/6 (with which this can be merged, so we have support in schema and parsing code together), the model can be completely dropped. [...]
@@ -16828,6 +17020,56 @@ static int virDomainPanicDefFormat(virBufferPtr buf, return 0; }
+static int virDomainIvshmemDefFormat(virBufferPtr buf, + virDomainIvshmemDefPtr def) +{ + if (def->server.enabled) + virBufferAsprintf(buf, "<server path='%s'/>\n", + def->server.path);
One general idea while looking through the code; could we make the path optional or even better, leave the "server" out somehow and generalize it even more. The path could be for example "/var/run/libvirt/ivshmem-<name>-sock" since we are starting it anyway, and permissions on that would be easier to set then (whole path is prepared already). The name would then be enough to get either the shmem name or the server socket path. Question is whether we can get the information whether server needs to be started from somewhere else. E.g. does server make sense with no msi vectors and without ioeventfd?
+ if (def->size) + virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", + def->size / (1024 * 1024)); +
If anyone specifies size < 1M, this won't be properly formatted (another idea for a test case). Martin
On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:
This patch adds configuration support for the shmem device as described in the schema in the previous patch.
[...]
This parsing using loop over children does not readably (and in this case neither reliably) check whether required options are present.
For example, I could use: servernode = virXPathNode("./server", ctxt)) instead of having a loop ?
Currently it parses <shmem name="asdf"/> as valid, but not specifying the size is probably not what you want on qemu command-line ... [1]
Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB. Thus <shmem name="asdf"/> is valid. [...]
+ if (vectors && + virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors) < 0) {
Use *_uip() if you don't want to accept negative values.
Ok. Thanks. [...]
+static int virDomainIvshmemDefFormat(virBufferPtr buf, + virDomainIvshmemDefPtr def) +{ + if (def->server.enabled) + virBufferAsprintf(buf, "<server path='%s'/>\n", + def->server.path);
One general idea while looking through the code; could we make the path optional or even better, leave the "server" out somehow and generalize it even more. The path could be for example "/var/run/libvirt/ivshmem-<name>-sock" since we are starting it anyway, and permissions on that would be easier to set then (whole path is prepared already). The name would then be enough to get either the shmem name or the server socket path.
If libvirt starts the ivshmem server, then we can use a default path. If libvirt did not start the server, then we should still let the path as an optional field for cases where the user wants to start the server himself with a custom path. We can have an optional path field to handle both cases.
Question is whether we can get the information whether server needs to be started from somewhere else. E.g. does server make sense with no msi vectors and without ioeventfd?
The server handles eventfds distribution across ivshmem clients. These eventfds are used to trigger interrupts (with or without msi) in the "guests" clients. So you can imagine starting a server without msi to only trigger interrupts in the guests. I would say that the server can be seen as a "support interrupt" property of the shmem object but calling it server is more explicit.
+ if (def->size) + virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", + def->size / (1024 * 1024)); +
If anyone specifies size < 1M, this won't be properly formatted (another idea for a test case).
Ivshmem in qemu only accepts size in MB or GB. To prevent this error, we should not accept size under 1MB, in virDomainIvshmemDefParseXML: if (virDomainParseScaledValue("./size[1]", ctxt, - &def->size, 1, + &def->size, 1024x1024, ULLONG_MAX, true) < 0) Maxime
On Wed, Aug 27, 2014 at 10:52:02PM +0200, Maxime Leroy wrote:
On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:
This patch adds configuration support for the shmem device as described in the schema in the previous patch.
[...]
This parsing using loop over children does not readably (and in this case neither reliably) check whether required options are present.
For example, I could use: servernode = virXPathNode("./server", ctxt)) instead of having a loop ?
Yes, sure. I think that the parsing parts using the loops are some very old code leftovers. Maybe from some SAX parsing, but I don't remember that, just guessing.
Currently it parses <shmem name="asdf"/> as valid, but not specifying the size is probably not what you want on qemu command-line ... [1]
Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB. Thus <shmem name="asdf"/> is valid.
Oh, I didn't know that. That is OK, but it should be mentioned in the documentation, and tests for that should be included as well.
[...]
+ if (vectors && + virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors) < 0) {
Use *_uip() if you don't want to accept negative values.
Ok. Thanks.
[...]
+static int virDomainIvshmemDefFormat(virBufferPtr buf, + virDomainIvshmemDefPtr def) +{ + if (def->server.enabled) + virBufferAsprintf(buf, "<server path='%s'/>\n", + def->server.path);
One general idea while looking through the code; could we make the path optional or even better, leave the "server" out somehow and generalize it even more. The path could be for example "/var/run/libvirt/ivshmem-<name>-sock" since we are starting it anyway, and permissions on that would be easier to set then (whole path is prepared already). The name would then be enough to get either the shmem name or the server socket path.
If libvirt starts the ivshmem server, then we can use a default path.
If libvirt did not start the server, then we should still let the path as an optional field for cases where the user wants to start the server himself with a custom path.
We can have an optional path field to handle both cases.
I agree. ... [coming back after few minutes] ... Maybe you were right the first time. Since we're not starting the server, the path should be specified all the time. After we add the support for the server, we can relax the checks for the XML parsing. That's always possible, but impossible the other way around (i.e. we cannot make the checks more strict in the future). So I'd say keep the path mandatory and relax it in thelast patch that adds the support for start='yes'. One more suggestion that would help us get the patches earlier (before the ivshmem-server is packaged in qemu) would be to check whether the start='' attribute is in the XML and forbid such configuration. And also relax that check after the support for ivshmem-server is added. Would you be OK with that? What's you opinion?
Question is whether we can get the information whether server needs to be started from somewhere else. E.g. does server make sense with no msi vectors and without ioeventfd?
The server handles eventfds distribution across ivshmem clients.
These eventfds are used to trigger interrupts (with or without msi) in the "guests" clients.
So you can imagine starting a server without msi to only trigger interrupts in the guests.
I would say that the server can be seen as a "support interrupt" property of the shmem object but calling it server is more explicit.
That explicit naming is what I wanted to get rid of, originally (why I asked the question above). I'm still thinking about the generalization of "shared memory" instead of exactly matching one particular technology+hypervisor (qemu+ivshmem).
+ if (def->size) + virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", + def->size / (1024 * 1024)); +
If anyone specifies size < 1M, this won't be properly formatted (another idea for a test case).
Ivshmem in qemu only accepts size in MB or GB.
Well, that should be somehow written in the documentation, then. But if I'm reading David's patch properly (not that I'm qemu-experienced or anything), it might be smaller after his patches: https://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01243.html We should somehow be able to parse any size in src/conf/domain_conf.c (global parsing code), but PostParse() functions for QEMU (src/qemu/qemu_domain.c) should allow only what's possible in QEMU. Or even better, if we can introspect how qemu parses the size (whether size=1 means 1 B or 1 MB; or whether it supports size=1K for example), we can forbid invalid values in qemuBuildCommandLine() based on the qemu that we'll be running. Martin
To prevent this error, we should not accept size under 1MB, in virDomainIvshmemDefParseXML:
if (virDomainParseScaledValue("./size[1]", ctxt, - &def->size, 1, + &def->size, 1024x1024, ULLONG_MAX, true) < 0)
Maxime
On 2014/8/22 18:47, Maxime Leroy wrote:
This patch adds configuration support for the shmem device as described in the schema in the previous patch.
Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> ---
+static virDomainShmemDefPtr +virDomainShmemDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + char *model = virXMLPropString(node, "model"); + virDomainShmemDefPtr def; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (model) { + if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) {
I guess you intend 'def->modle = ...' not 'def->model == ...' .
+ virReportError(VIR_ERR_XML_ERROR, + _("Unknown <shmem> model '%s'"), model); + goto error; + } + } else + def->model = VIR_DOMAIN_SHMEM_MODEL_IVSHMEM; + + if (!(def->name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("<shmem> must contain 'name' attribute")); + goto error; + }
As Martin mentioned, 'name' is not necessary.
+static int virDomainShmemDefFormat(virBufferPtr buf, + virDomainShmemDefPtr def, + unsigned int flags) +{ + virBufferAsprintf(buf, "<shmem name='%s' model='%s'>\n", + def->name, virDomainShmemModelTypeToString(def->model));
Here too, 'name' is not necessary
On Tue, Aug 26, 2014 at 11:02 AM, Wang Rui <moon.wangrui@huawei.com> wrote:
On 2014/8/22 18:47, Maxime Leroy wrote:
This patch adds configuration support for the shmem device as described in the schema in the previous patch. [..] + if (model) { + if ((def->model == virDomainShmemModelTypeFromString(model)) < 0) {
I guess you intend 'def->modle = ...' not 'def->model == ...' .
Thanks. Anyway model attribute will be removed. Maxime
Ivshmem is supported by QEMU since 0.13 release. Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_1.2.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.3.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.4.2-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.5.3-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_1.6.50-1.caps | 1 + tests/qemuhelptest.c | 17 ++++++++++++----- 9 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b758b5a..49e0f0d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -266,6 +266,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "memory-backend-file", "usb-audio", "rtc-reset-reinjection", + + "ivshmem", /* 175 */ ); @@ -1487,6 +1489,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "memory-backend-ram", QEMU_CAPS_OBJECT_MEMORY_RAM }, { "memory-backend-file", QEMU_CAPS_OBJECT_MEMORY_FILE }, { "usb-audio", QEMU_CAPS_OBJECT_USB_AUDIO }, + { "ivshmem", QEMU_CAPS_DEVICE_IVSHMEM }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index cbd3646..1005a4e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -214,6 +214,7 @@ typedef enum { QEMU_CAPS_OBJECT_MEMORY_FILE = 172, /* -object memory-backend-file */ QEMU_CAPS_OBJECT_USB_AUDIO = 173, /* usb-audio device support */ QEMU_CAPS_RTC_RESET_REINJECTION = 174, /* rtc-reset-reinjection monitor command */ + QEMU_CAPS_DEVICE_IVSHMEM = 175, /* -device ivshmem */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps index c8a379a..f32bc27 100644 --- a/tests/qemucapabilitiesdata/caps_1.2.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.caps @@ -116,4 +116,5 @@ <flag name='usb-kbd'/> <flag name='host-pci-multidomain'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps index 4b7651e..f4f0397 100644 --- a/tests/qemucapabilitiesdata/caps_1.3.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.3.1-1.caps @@ -129,4 +129,5 @@ <flag name='usb-kbd'/> <flag name='host-pci-multidomain'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps index d146bf9..e6659e4 100644 --- a/tests/qemucapabilitiesdata/caps_1.4.2-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.4.2-1.caps @@ -130,4 +130,5 @@ <flag name='usb-kbd'/> <flag name='host-pci-multidomain'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps index 5fa30aa..591cde6 100644 --- a/tests/qemucapabilitiesdata/caps_1.5.3-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.5.3-1.caps @@ -138,4 +138,5 @@ <flag name='usb-kbd'/> <flag name='host-pci-multidomain'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps index f364bbf..538a16e 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.0-1.caps @@ -145,4 +145,5 @@ <flag name='host-pci-multidomain'/> <flag name='msg-timestamp'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps index 10ce1b5..b814e8a 100644 --- a/tests/qemucapabilitiesdata/caps_1.6.50-1.caps +++ b/tests/qemucapabilitiesdata/caps_1.6.50-1.caps @@ -144,4 +144,5 @@ <flag name='msg-timestamp'/> <flag name='numa'/> <flag name='usb-audio'/> + <flag name='ivshmem'/> </qemuCaps> diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 366e36d..ba12cdb 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -518,7 +518,8 @@ mymain(void) QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_DEVICE_USB_KBD, QEMU_CAPS_DEVICE_USB_STORAGE, - QEMU_CAPS_HOST_PCI_MULTIDOMAIN); + QEMU_CAPS_HOST_PCI_MULTIDOMAIN, + QEMU_CAPS_DEVICE_IVSHMEM); DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -745,7 +746,8 @@ mymain(void) QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX, QEMU_CAPS_DEVICE_USB_KBD, - QEMU_CAPS_DEVICE_USB_STORAGE); + QEMU_CAPS_DEVICE_USB_STORAGE, + QEMU_CAPS_DEVICE_IVSHMEM); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -843,7 +845,9 @@ mymain(void) QEMU_CAPS_VNC_SHARE_POLICY, QEMU_CAPS_DEVICE_USB_KBD, QEMU_CAPS_DEVICE_USB_STORAGE, - QEMU_CAPS_OBJECT_USB_AUDIO); + QEMU_CAPS_OBJECT_USB_AUDIO, + QEMU_CAPS_DEVICE_IVSHMEM); + DO_TEST("qemu-1.2.0", 1002000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -953,7 +957,9 @@ mymain(void) QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_DEVICE_USB_KBD, QEMU_CAPS_USB_STORAGE_REMOVABLE, - QEMU_CAPS_OBJECT_USB_AUDIO); + QEMU_CAPS_OBJECT_USB_AUDIO, + QEMU_CAPS_DEVICE_IVSHMEM); + DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -1068,7 +1074,8 @@ mymain(void) QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_DEVICE_USB_KBD, QEMU_CAPS_USB_STORAGE_REMOVABLE, - QEMU_CAPS_OBJECT_USB_AUDIO); + QEMU_CAPS_OBJECT_USB_AUDIO, + QEMU_CAPS_DEVICE_IVSHMEM); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.3
This patch implements support for the ivshmem device in QEMU. Example from this xml: <shmem name='ivshmem0' model='ivshmem'> <server path='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem> The following QEMU line is built: -device ivshmem,size=32m,vectors=32,chardev=charshmem0,msi=on, ioeventfd=on,role=master -chardev socket,path=/tmp/socket-ivshmem0,id=charshmem0 Note: PCI hotpluging is not implemented. Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- src/qemu/qemu_command.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 1 + 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0d7b12d..9fcceae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1019,6 +1019,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) if (virAsprintf(&def->hubs[i]->info.alias, "hub%zu", i) < 0) return -1; } + for (i = 0; i < def->nshmems; i++) { + if (virAsprintf(&def->shmems[i]->info.alias, "shmem%zu", i) < 0) + return -1; + } for (i = 0; i < def->nsmartcards; i++) { if (virAsprintf(&def->smartcards[i]->info.alias, "smartcard%zu", i) < 0) return -1; @@ -5043,6 +5047,100 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def, return NULL; } +static char * +qemuBuildIvshmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virDomainIvshmemDefPtr ivshmem = &dev->data.ivshmem; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem device is not supported by QEMU")); + goto error; + } + + virBufferAddLit(&buf, "ivshmem"); + if (ivshmem->size) + virBufferAsprintf(&buf, ",size=%llum", ivshmem->size / (1024 * 1024)); + + if (!ivshmem->server.enabled) + virBufferAsprintf(&buf, ",shm=%s", dev->name); + else { + virBufferAsprintf(&buf, ",chardev=char%s", dev->info.alias); + if (ivshmem->msi.enabled) { + virBufferAddLit(&buf, ",msi=on"); + if (ivshmem->msi.vectors) + virBufferAsprintf(&buf, ",vectors=%u", ivshmem->msi.vectors); + if (ivshmem->msi.ioeventfd) + virBufferAsprintf(&buf, ",ioeventfd=%s", + virTristateSwitchTypeToString(ivshmem->msi.ioeventfd)); + } + } + + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) + goto error; + + if (virBufferCheckError(&buf) < 0) + goto error; + + return virBufferContentAndReset(&buf); + + error: + virBufferFreeAndReset(&buf); + return NULL; +} + +static int +qemuBuildIvshmemCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virDomainShmemDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + char *devstr; + virDomainIvshmemDefPtr ivshmem = &dev->data.ivshmem; + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildIvshmemDevStr(def, dev, qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + + if (ivshmem->server.enabled) { + virDomainChrSourceDef source; + + source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + source.data.nix.path = ivshmem->server.path; + source.data.nix.listen = false; + + virCommandAddArg(cmd, "-chardev"); + if (!(devstr = qemuBuildChrChardevStr(&source, dev->info.alias, + qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } + + return 0; +} + +static int +qemuBuildShmemCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virDomainShmemDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + switch (dev->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + return qemuBuildIvshmemCommandLine(cmd, def, dev, qemuCaps); + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected shem model %d"), dev->model); + } + return -1; +} + char * qemuBuildUSBHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, @@ -5299,7 +5397,7 @@ qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, /* This function outputs a -chardev command line option which describes only the * host side of the character device */ -static char * +char * qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias, virQEMUCapsPtr qemuCaps) { @@ -9332,6 +9430,11 @@ qemuBuildCommandLine(virConnectPtr conn, } } + for (i = 0; i < def->nshmems; i++) { + if (qemuBuildShmemCommandLine(cmd, def, def->shmems[i], qemuCaps)) + goto error; + } + if (mlock) { unsigned long long memKB; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 633ff71..3373a59 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -130,6 +130,10 @@ char *qemuBuildDriveDevStr(virDomainDefPtr def, char *qemuBuildFSDevStr(virDomainDefPtr domainDef, virDomainFSDefPtr fs, virQEMUCapsPtr qemuCaps); + +char * qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, + const char *alias, + virQEMUCapsPtr qemuCaps); /* Current, best practice */ char *qemuBuildControllerDevStr(virDomainDefPtr domainDef, virDomainControllerDefPtr def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a364c52..def442b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2858,6 +2858,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("don't know how to remove a %s device"), -- 1.9.3
On Fri, Aug 22, 2014 at 12:47:03PM +0200, Maxime Leroy wrote:
This patch implements support for the ivshmem device in QEMU.
Example from this xml:
<shmem name='ivshmem0' model='ivshmem'> <server path='/tmp/socket-ivshmem0'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem>
The following QEMU line is built:
-device ivshmem,size=32m,vectors=32,chardev=charshmem0,msi=on, ioeventfd=on,role=master -chardev socket,path=/tmp/socket-ivshmem0,id=charshmem0
Note: PCI hotpluging is not implemented.
Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- src/qemu/qemu_command.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_command.h | 4 ++ src/qemu/qemu_hotplug.c | 1 + 3 files changed, 109 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0d7b12d..9fcceae 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1019,6 +1019,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) if (virAsprintf(&def->hubs[i]->info.alias, "hub%zu", i) < 0) return -1; } + for (i = 0; i < def->nshmems; i++) { + if (virAsprintf(&def->shmems[i]->info.alias, "shmem%zu", i) < 0) + return -1; + } for (i = 0; i < def->nsmartcards; i++) { if (virAsprintf(&def->smartcards[i]->info.alias, "smartcard%zu", i) < 0) return -1; @@ -5043,6 +5047,100 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def, return NULL; }
+static char * +qemuBuildIvshmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virDomainIvshmemDefPtr ivshmem = &dev->data.ivshmem; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem device is not supported by QEMU")); + goto error; + } + + virBufferAddLit(&buf, "ivshmem"); + if (ivshmem->size) + virBufferAsprintf(&buf, ",size=%llum", ivshmem->size / (1024 * 1024)); +
It will sometimes format the command-line parameter to size=0m, which will terribly fail. We should add it to the command-line with the smallest scale possible, so it can be configured to the tiniest bit (and it will also get rid of this error). Other than that, the patch looks fine. Martin
On Tue, Aug 26, 2014 at 10:58 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:03PM +0200, Maxime Leroy wrote:
[..]
It will sometimes format the command-line parameter to size=0m, which will terribly fail. We should add it to the command-line with the smallest scale possible, so it can be configured to the tiniest bit (and it will also get rid of this error).
As explain in my previous email, Ivshmem in qemu only accepts size in MB or GB. To prevent this error, we should not accept size under 1MB, in virDomainIvshmemDefParseXML: if (virDomainParseScaledValue("./size[1]", ctxt, - &def->size, 1, + &def->size, 1024x1024, ULLONG_MAX, true) < 0) Maxime
On 2014/8/22 18:47, Maxime Leroy wrote:
+static int +qemuBuildIvshmemCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virDomainShmemDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + char *devstr; + virDomainIvshmemDefPtr ivshmem = &dev->data.ivshmem; + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildIvshmemDevStr(def, dev, qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + + if (ivshmem->server.enabled) { + virDomainChrSourceDef source; + + source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + source.data.nix.path = ivshmem->server.path; + source.data.nix.listen = false; + + virCommandAddArg(cmd, "-chardev"); + if (!(devstr = qemuBuildChrChardevStr(&source, dev->info.alias, + qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr);
indentation
On Tue, Aug 26, 2014 at 11:23 AM, Wang Rui <moon.wangrui@huawei.com> wrote:
On 2014/8/22 18:47, Maxime Leroy wrote: [...]
+ if (!(devstr = qemuBuildIvshmemDevStr(def, dev, qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + + if (ivshmem->server.enabled) { + virDomainChrSourceDef source; + + source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + source.data.nix.path = ivshmem->server.path; + source.data.nix.listen = false; + + virCommandAddArg(cmd, "-chardev"); + if (!(devstr = qemuBuildChrChardevStr(&source, dev->info.alias, + qemuCaps))) + return -1; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr);
indentation
Ok. Thanks. Maxime
Add XML parsing and qemu command line tests for the ivshmem device support. Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args | 10 +++++++ tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml | 35 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ tests/qemuxml2xmltest.c | 2 ++ 4 files changed, 50 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args new file mode 100644 index 0000000..8a5cc0f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S \ +-M pc -m 214 -smp 1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-device ivshmem,chardev=charshmem0,msi=on,vectors=32,ioeventfd=on \ +-chardev socket,id=charshmem0,path=/tmp/socket-shmem0 \ +-device ivshmem,size=32m,chardev=charshmem1,msi=on,vectors=32 \ +-chardev socket,id=charshmem1,path=/tmp/socket-shmem1 \ +-device ivshmem,size=32m,shm=shmem2,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml new file mode 100644 index 0000000..7177612 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>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</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + <shmem name='shmem0' model='ivshmem'> + <server path='/tmp/socket-shmem0'/> + <msi vectors='32' ioeventfd='on'/> + </shmem> + <shmem name='shmem1' model='ivshmem'> + <server path='/tmp/socket-shmem1'/> + <size unit='M'>32</size> + <msi vectors='32'/> + </shmem> + <shmem name='shmem2' model='ivshmem'> + <size unit='M'>32</size> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </shmem> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 65dc9c7..f91ac36 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1431,6 +1431,9 @@ mymain(void) DO_TEST("panic", QEMU_CAPS_DEVICE_PANIC, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("ivshmem", QEMU_CAPS_PCIDEVICE, + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_IVSHMEM); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 5941323..4e62fe9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -388,6 +388,8 @@ mymain(void) DO_TEST_DIFFERENT("numatune-memnode"); DO_TEST("numatune-memnode-no-memory"); + DO_TEST("ivshmem"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 1.9.3
On Fri, Aug 22, 2014 at 12:47:04PM +0200, Maxime Leroy wrote:
Add XML parsing and qemu command line tests for the ivshmem device support.
Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args | 10 +++++++ tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml | 35 ++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ tests/qemuxml2xmltest.c | 2 ++ 4 files changed, 50 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args new file mode 100644 index 0000000..8a5cc0f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S \ +-M pc -m 214 -smp 1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ +-device ivshmem,chardev=charshmem0,msi=on,vectors=32,ioeventfd=on \ +-chardev socket,id=charshmem0,path=/tmp/socket-shmem0 \ +-device ivshmem,size=32m,chardev=charshmem1,msi=on,vectors=32 \ +-chardev socket,id=charshmem1,path=/tmp/socket-shmem1 \ +-device ivshmem,size=32m,shm=shmem2,bus=pci.0,addr=0x8 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml new file mode 100644 index 0000000..7177612 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>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</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + <shmem name='shmem0' model='ivshmem'> + <server path='/tmp/socket-shmem0'/> + <msi vectors='32' ioeventfd='on'/> + </shmem> + <shmem name='shmem1' model='ivshmem'> + <server path='/tmp/socket-shmem1'/> + <size unit='M'>32</size> + <msi vectors='32'/>
The qemuxml2xmltest didn't faile with this xml, which means we are not allocating addresses for shmem devices. We have to do that and put it in the XML so (a) the address won't change and (b) that we know which address is occupied by that in case we'll want to attach something to the VM. And xml2xml (and PARSE_ERROR xml2argv) tests can be squashed into the patch with documentation and parsing. Other xml2argv tests can be squashed into the patch where qemu formats the command-line. Martin
+ </shmem> + <shmem name='shmem2' model='ivshmem'> + <size unit='M'>32</size> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </shmem> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 65dc9c7..f91ac36 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1431,6 +1431,9 @@ mymain(void) DO_TEST("panic", QEMU_CAPS_DEVICE_PANIC, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+ DO_TEST("ivshmem", QEMU_CAPS_PCIDEVICE, + QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_IVSHMEM); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 5941323..4e62fe9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -388,6 +388,8 @@ mymain(void) DO_TEST_DIFFERENT("numatune-memnode"); DO_TEST("numatune-memnode-no-memory");
+ DO_TEST("ivshmem"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);
-- 1.9.3
On Tue, Aug 26, 2014 at 11:02 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:04PM +0200, Maxime Leroy wrote:
[...]
+ <shmem name='shmem0' model='ivshmem'> + <server path='/tmp/socket-shmem0'/> + <msi vectors='32' ioeventfd='on'/> + </shmem> + <shmem name='shmem1' model='ivshmem'> + <server path='/tmp/socket-shmem1'/> + <size unit='M'>32</size> + <msi vectors='32'/>
The qemuxml2xmltest didn't faile with this xml, which means we are not allocating addresses for shmem devices. We have to do that and put it in the XML so (a) the address won't change and (b) that we know which address is occupied by that in case we'll want to attach something to the VM.
Ok. Thanks.
And xml2xml (and PARSE_ERROR xml2argv) tests can be squashed into the patch with documentation and parsing. Other xml2argv tests can be squashed into the patch where qemu formats the command-line.
Ok. I also should merge the documentation and parsing commit ? Maxime
On Wed, Aug 27, 2014 at 11:02:36PM +0200, Maxime Leroy wrote:
On Tue, Aug 26, 2014 at 11:02 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Fri, Aug 22, 2014 at 12:47:04PM +0200, Maxime Leroy wrote:
[...]
+ <shmem name='shmem0' model='ivshmem'> + <server path='/tmp/socket-shmem0'/> + <msi vectors='32' ioeventfd='on'/> + </shmem> + <shmem name='shmem1' model='ivshmem'> + <server path='/tmp/socket-shmem1'/> + <size unit='M'>32</size> + <msi vectors='32'/>
The qemuxml2xmltest didn't faile with this xml, which means we are not allocating addresses for shmem devices. We have to do that and put it in the XML so (a) the address won't change and (b) that we know which address is occupied by that in case we'll want to attach something to the VM.
Ok. Thanks.
And xml2xml (and PARSE_ERROR xml2argv) tests can be squashed into the patch with documentation and parsing. Other xml2argv tests can be squashed into the patch where qemu formats the command-line.
Ok.
I also should merge the documentation and parsing commit ?
That's a common practice, it ensures that if anyone back-ports any commits anywhere, the documentation will match the parsing and tests will be included as well. It's not super-strict, but it's nice to have and it also helps review sometimes. Martin
With the new start param, we are able to start the ivshmem-server. With this xml: <shmem name='ivshmem0' model='ivshmem'> <server path='/tmp/ivshmem0.sock' start='yes'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem> Libvirt will execute an ivshmem-server: /usr/bin/ivshmem-server -m ivshmem0 -S /tmp/ivshmem0.sock \ -p /var/run/libvirt/ivshmem-server/ivshd-ivshmem0.pid -n 32 Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- configure.ac | 4 + docs/formatdomain.html.in | 5 +- docs/schemas/domaincommon.rng | 3 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 19 +++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 5 + src/qemu/qemu_command.c | 7 ++ src/qemu/qemu_process.c | 10 ++ src/util/virivshmemserver.c | 141 ++++++++++++++++++++++++ src/util/virivshmemserver.h | 28 +++++ tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml | 2 +- 13 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 src/util/virivshmemserver.c create mode 100644 src/util/virivshmemserver.h diff --git a/configure.ac b/configure.ac index f93c6c2..6b525b9 100644 --- a/configure.ac +++ b/configure.ac @@ -431,6 +431,8 @@ AC_PATH_PROG([SCRUB], [scrub], [scrub], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line], [/sbin:/usr/bin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([IVSHMEMSERVER], [ivshmem-server], [ivshmem-server], + [/usr/bin:/usr/local/bin:$PATH]) AC_DEFINE_UNQUOTED([DMIDECODE],["$DMIDECODE"], [Location or name of the dmidecode program]) @@ -463,6 +465,8 @@ AC_DEFINE_UNQUOTED([SCRUB],["$SCRUB"], [Location or name of the scrub program (for wiping algorithms)]) AC_DEFINE_UNQUOTED([ADDR2LINE],["$ADDR2LINE"], [Location of addr2line program]) +AC_DEFINE_UNQUOTED([IVSHMEMSERVER], ["$IVSHMEMSERVER"], + [Location or name of ivshmem-server program]) dnl Specific dir for HTML output ? AC_ARG_WITH([html-dir], [AS_HELP_STRING([--with-html-dir=path], diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c7644bc..df231d8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5462,7 +5462,7 @@ qemu-kvm -net nic,model=? /dev/null ... <devices> <shmem name='shmem0' model='ivshmem'> - <server path='/tmp/socket-shmem0'/> + <server path='/tmp/socket-shmem0' start='yes'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem> @@ -5477,6 +5477,9 @@ qemu-kvm -net nic,model=? /dev/null <dd>The optional <code>server</code> element can be used to configure an ivshmem device connected to the ivshmem server via a unix socket. The <code>path</code> attribute specifies the path to the unix socket. + The <code>start</code> attribute specifies if the libvirt must start + the ivshmem-server. By default, libvirt expects that the ivshmem-server is + already running on the host. </dd> <dt><code>size</code></dt> <dd>The optional <code>size</code> element specifies the size of the shared memory. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 64abf2b..a601747 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3246,6 +3246,9 @@ <optional> <element name="server"> <attribute name="path"/> + <optional> + <attribute name="start"/> + </optional> </element> </optional> <optional> diff --git a/po/POTFILES.in b/po/POTFILES.in index f17b35f..7d90517 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -177,6 +177,7 @@ src/util/viridentity.c src/util/virinitctl.c src/util/viriptables.c src/util/viriscsi.c +src/util/virivshmemserver.c src/util/virjson.c src/util/virkeyfile.c src/util/virlockspace.c diff --git a/src/Makefile.am b/src/Makefile.am index 538530e..00e1ccf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -118,6 +118,7 @@ UTIL_SOURCES = \ util/virinitctl.c util/virinitctl.h \ util/viriptables.c util/viriptables.h \ util/viriscsi.c util/viriscsi.h \ + util/virivshmemserver.c util/virivshmemserver.h \ util/virjson.c util/virjson.h \ util/virkeycode.c util/virkeycode.h \ util/virkeyfile.c util/virkeyfile.h \ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 08d653a..224b367 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9509,6 +9509,7 @@ virDomainIvshmemDefParseXML(xmlNodePtr node, { char *ioeventfd = NULL; char *vectors = NULL; + char *start = NULL; xmlNodePtr cur; xmlNodePtr save = ctxt->node; int ret; @@ -9523,6 +9524,13 @@ virDomainIvshmemDefParseXML(xmlNodePtr node, _("cannot parse <server> 'path' attribute")); goto error; } + start = virXMLPropString(cur, "start"); + if (start && + (def->server.start = virTristateBoolTypeFromString(start)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown <start> value '%s'"), + start); + } } else if (xmlStrEqual(cur->name, BAD_CAST "size")) { if (virDomainParseScaledValue("./size[1]", ctxt, &def->size, 1, @@ -9575,6 +9583,7 @@ virDomainIvshmemDefParseXML(xmlNodePtr node, ctxt->node = save; VIR_FREE(ioeventfd); VIR_FREE(vectors); + VIR_FREE(start); return ret; error: @@ -17023,9 +17032,15 @@ static int virDomainPanicDefFormat(virBufferPtr buf, static int virDomainIvshmemDefFormat(virBufferPtr buf, virDomainIvshmemDefPtr def) { - if (def->server.enabled) - virBufferAsprintf(buf, "<server path='%s'/>\n", + if (def->server.enabled) { + virBufferAsprintf(buf, "<server path='%s'", def->server.path); + if (def->server.start) + virBufferAsprintf(buf, " start='%s'", + virTristateBoolTypeToString(def->server.start)); + virBufferAddLit(buf, "/>\n"); + } + if (def->size) virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", def->size / (1024 * 1024)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0c6aa21..7c01c22 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1504,6 +1504,7 @@ struct _virDomainIvshmemDef { unsigned long long size; struct { bool enabled; + int start; /* enum virTristateBool */ char *path; } server; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f86926e..ee77995 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1440,6 +1440,11 @@ virISCSIRescanLUNs; virISCSIScanTargets; +# util/virivshmemserver.h +virStartIvshmemServer; +virStopIvshmemServer; + + # util/virjson.h virJSONValueArrayAppend; virJSONValueArrayGet; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9fcceae..fe2fc1a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -34,6 +34,7 @@ #include "virarch.h" #include "virerror.h" #include "virfile.h" +#include "virivshmemserver.h" #include "virnetdev.h" #include "virstring.h" #include "virtime.h" @@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } } return 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index baa866a..aaf03a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -67,6 +67,7 @@ #include "virstring.h" #include "virhostdev.h" #include "storage/storage_driver.h" +#include "virivshmemserver.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -4684,6 +4685,15 @@ void qemuProcessStop(virQEMUDriverPtr driver, } } + /* stop runnning ivshmem server */ + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM && + shmem->data.ivshmem.server.start == VIR_TRISTATE_BOOL_YES) { + virStopIvshmemServer(shmem->name); + } + } + vm->taint = 0; vm->pid = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); diff --git a/src/util/virivshmemserver.c b/src/util/virivshmemserver.c new file mode 100644 index 0000000..89130b1 --- /dev/null +++ b/src/util/virivshmemserver.c @@ -0,0 +1,141 @@ +/* + * Copyright 2014 6WIND S.A. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> +#include <string.h> +#include <signal.h> + +#include "virivshmemserver.h" +#include "vircommand.h" +#include "viralloc.h" +#include "virerror.h" +#include "virlog.h" +#include "virstring.h" +#include "virfile.h" +#include "virpidfile.h" +#include "virprocess.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.ivshmemserver") + +#define IVSHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/ivshmem-server" + +static char *virIvshmemServerPidFileBasename(const char *shm_name) { + char *pidfilebase; + + ignore_value(virAsprintf(&pidfilebase, "ivshd-%s", shm_name)); + return pidfilebase; +} + +/** + * virStartIvshmemServer: + * @shm_name: the name of the shared memory + * @unix_socket_path: the path to the socket unix file + * @size: the size of the shared memory (optionnal) + * @vectors: the number of vectors (optionnal) + * + * Start an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStartIvshmemServer(const char *shm_name, const char *unix_socket_path, + size_t size, unsigned vectors) +{ + char *ivshmserver_pidbase = NULL; + char *pidfile = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + + if (!virFileIsExecutable(IVSHMEMSERVER)) { + virReportSystemError(errno, _(" %s is not available"), + IVSHMEMSERVER); + goto cleanup; + } + + if (virFileMakePath(IVSHMEM_STATE_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + IVSHMEM_STATE_DIR); + goto cleanup; + } + + /* construct pidfile name */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (!(pidfile = virPidFileBuildPath(IVSHMEM_STATE_DIR, ivshmserver_pidbase))) + goto cleanup; + + cmd = virCommandNewArgList(IVSHMEMSERVER, "-m", shm_name, + "-S", unix_socket_path, "-p", pidfile, NULL); + + if (size) + virCommandAddArgFormat(cmd, "-l %zu", size); + + if (vectors) + virCommandAddArgFormat(cmd, "-n %u", vectors); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start %s for %s shm"), + IVSHMEMSERVER, shm_name); + goto cleanup; + } + ret = 0; + cleanup: + virCommandFree(cmd); + VIR_FREE(ivshmserver_pidbase); + VIR_FREE(pidfile); + return ret; +} + +/** + * virStopIvshmemServer: + * @shm_name: the name of the shared memory + * + * Stop an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStopIvshmemServer(const char *shm_name) +{ + char *ivshmserver_pidbase = NULL; + pid_t ivshmserver_pid; + int ret = -1; + + /* get pid of the ivshmem server */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase, + &ivshmserver_pid)) + goto cleanup; + + virProcessKill(ivshmserver_pid, SIGTERM); + virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase); + ret = 0; + cleanup: + VIR_FREE(ivshmserver_pidbase); + return ret; +} diff --git a/src/util/virivshmemserver.h b/src/util/virivshmemserver.h new file mode 100644 index 0000000..1fae8d9 --- /dev/null +++ b/src/util/virivshmemserver.h @@ -0,0 +1,28 @@ +/* + * Copyright 2014 6WIND S.A. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_IVSHMEM_SERVER_H__ +# define __VIR_IVSHMEN_SERVER_H__ + +int virStartIvshmemServer(const char *shm_name, const char *unix_socket_path, + size_t size, unsigned vectors); + +int virStopIvshmemServer(const char *shm_name); + +#endif /* __VIR_IVSHMEM_SERVER_H__ */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml index 7177612..22c1d14 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml @@ -19,7 +19,7 @@ <controller type='pci' index='0' model='pci-root'/> <memballoon model='virtio'/> <shmem name='shmem0' model='ivshmem'> - <server path='/tmp/socket-shmem0'/> + <server path='/tmp/socket-shmem0' start='no'/> <msi vectors='32' ioeventfd='on'/> </shmem> <shmem name='shmem1' model='ivshmem'> -- 1.9.3
[Cc'ing David due to some questions/ideas about the ivshmem server] On Fri, Aug 22, 2014 at 12:47:05PM +0200, Maxime Leroy wrote:
With the new start param, we are able to start the ivshmem-server.
With this xml:
<shmem name='ivshmem0' model='ivshmem'> <server path='/tmp/ivshmem0.sock' start='yes'/> <size unit='M'>32</size> <msi vectors='32' ioeventfd='on'/> </shmem>
Libvirt will execute an ivshmem-server:
/usr/bin/ivshmem-server -m ivshmem0 -S /tmp/ivshmem0.sock \ -p /var/run/libvirt/ivshmem-server/ivshd-ivshmem0.pid -n 32
Signed-off-by: Maxime Leroy <maxime.leroy@6wind.com> --- configure.ac | 4 + docs/formatdomain.html.in | 5 +- docs/schemas/domaincommon.rng | 3 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/domain_conf.c | 19 +++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 5 + src/qemu/qemu_command.c | 7 ++ src/qemu/qemu_process.c | 10 ++ src/util/virivshmemserver.c | 141 ++++++++++++++++++++++++ src/util/virivshmemserver.h | 28 +++++ tests/qemuxml2argvdata/qemuxml2argv-ivshmem.xml | 2 +- 13 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 src/util/virivshmemserver.c create mode 100644 src/util/virivshmemserver.h
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9fcceae..fe2fc1a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -34,6 +34,7 @@ #include "virarch.h" #include "virerror.h" #include "virfile.h" +#include "virivshmemserver.h" #include "virnetdev.h" #include "virstring.h" #include "virtime.h" @@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
What if the server is already running? Either 2 domains have server start='yes' or the user started it already. We should not fail in these cases, I guess.
return 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index baa866a..aaf03a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -67,6 +67,7 @@ #include "virstring.h" #include "virhostdev.h" #include "storage/storage_driver.h" +#include "virivshmemserver.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -4684,6 +4685,15 @@ void qemuProcessStop(virQEMUDriverPtr driver, } }
+ /* stop runnning ivshmem server */ + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM && + shmem->data.ivshmem.server.start == VIR_TRISTATE_BOOL_YES) { + virStopIvshmemServer(shmem->name); + } + } +
It would be great if the domain with server start='yes' didn't have to be started first and killed last. Or if we could just signal the server (let's say SIGUSR1) that it should unlink the shmem and quit if no domains are connected. That way we could just send a signal to the server whenever any domain is disconnecting (maybe some check that no domain is being started might be good too ;) ).
vm->taint = 0; vm->pid = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); diff --git a/src/util/virivshmemserver.c b/src/util/virivshmemserver.c new file mode 100644 index 0000000..89130b1 --- /dev/null +++ b/src/util/virivshmemserver.c @@ -0,0 +1,141 @@ +/* + * Copyright 2014 6WIND S.A. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> +#include <string.h> +#include <signal.h> + +#include "virivshmemserver.h" +#include "vircommand.h" +#include "viralloc.h" +#include "virerror.h" +#include "virlog.h" +#include "virstring.h" +#include "virfile.h" +#include "virpidfile.h" +#include "virprocess.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.ivshmemserver") + +#define IVSHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/ivshmem-server" + +static char *virIvshmemServerPidFileBasename(const char *shm_name) { + char *pidfilebase; + + ignore_value(virAsprintf(&pidfilebase, "ivshd-%s", shm_name)); + return pidfilebase; +} + +/** + * virStartIvshmemServer: + * @shm_name: the name of the shared memory + * @unix_socket_path: the path to the socket unix file + * @size: the size of the shared memory (optionnal) + * @vectors: the number of vectors (optionnal) + * + * Start an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStartIvshmemServer(const char *shm_name, const char *unix_socket_path, + size_t size, unsigned vectors) +{ + char *ivshmserver_pidbase = NULL; + char *pidfile = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + + if (!virFileIsExecutable(IVSHMEMSERVER)) { + virReportSystemError(errno, _(" %s is not available"), + IVSHMEMSERVER);
IVSHMEM_SERVER (with underscore) would be a bit more readable. I'm not sure new file is needed for the stop/start, but shouldn't hurt.
+ goto cleanup; + } + + if (virFileMakePath(IVSHMEM_STATE_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + IVSHMEM_STATE_DIR); + goto cleanup; + } + + /* construct pidfile name */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (!(pidfile = virPidFileBuildPath(IVSHMEM_STATE_DIR, ivshmserver_pidbase))) + goto cleanup; + + cmd = virCommandNewArgList(IVSHMEMSERVER, "-m", shm_name, + "-S", unix_socket_path, "-p", pidfile, NULL); + + if (size) + virCommandAddArgFormat(cmd, "-l %zu", size); + + if (vectors) + virCommandAddArgFormat(cmd, "-n %u", vectors); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virReportSystemError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start %s for %s shm"), + IVSHMEMSERVER, shm_name); + goto cleanup; + } + ret = 0; + cleanup: + virCommandFree(cmd); + VIR_FREE(ivshmserver_pidbase); + VIR_FREE(pidfile); + return ret; +} + +/** + * virStopIvshmemServer: + * @shm_name: the name of the shared memory + * + * Stop an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStopIvshmemServer(const char *shm_name) +{ + char *ivshmserver_pidbase = NULL; + pid_t ivshmserver_pid; + int ret = -1; + + /* get pid of the ivshmem server */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase, + &ivshmserver_pid)) + goto cleanup; + + virProcessKill(ivshmserver_pid, SIGTERM); + virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);
The pidfile support could be added to the server too, maybe... Martin
Hello Martin, On 08/26/2014 11:33 AM, Martin Kletzander wrote:
[Cc'ing David due to some questions/ideas about the ivshmem server]
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
What if the server is already running? Either 2 domains have server start='yes' or the user started it already. We should not fail in these cases, I guess.
[snip]
It would be great if the domain with server start='yes' didn't have to be started first and killed last. Or if we could just signal the server (let's say SIGUSR1) that it should unlink the shmem and quit if no domains are connected. That way we could just send a signal to the server whenever any domain is disconnecting (maybe some check that no domain is being started might be good too ;) ).
Why not. If we want to automagically start/stop the server, we don't need the 'start' field, but we need a way to find if a server is running for this shared mem instance. We can try to bind the unix socket and check if a EADDRINUSE is returned. If EADDRINUSE, server is running, nothing to do. Else libvirt starts the server with the option "-q" (new option I can add which means "quit on last client disconnect") On stop, libvirt does nothing about the ivshmem-server. With this, if the server had been started with "-q" (by libvirt or by the user), then the server will quit on the last disconnect. If the user did not start the server with -q, it is his reponsibility to stop it. The 'start' field can disappear. No need to send any signal to ivshmem-server. What do you think of this ? [snip]
+/** + * virStopIvshmemServer: + * @shm_name: the name of the shared memory + * + * Stop an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStopIvshmemServer(const char *shm_name) +{ + char *ivshmserver_pidbase = NULL; + pid_t ivshmserver_pid; + int ret = -1; + + /* get pid of the ivshmem server */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase, + &ivshmserver_pid)) + goto cleanup; + + virProcessKill(ivshmserver_pid, SIGTERM); + virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);
The pidfile support could be added to the server too, maybe...
I am not sure I understand the point. The only thing this code does here is retrieve the current pid for the ivshmem-server, it kills the associated pid then remove the pidfile. So what do you mean by "pidfile support" ? Do you suggest that, on exit, the ivshmem-server should remove the pidfile it first created ? -- David Marchand
On Thu, Aug 28, 2014 at 10:34:11AM +0200, David Marchand wrote:
Hello Martin,
On 08/26/2014 11:33 AM, Martin Kletzander wrote:
[Cc'ing David due to some questions/ideas about the ivshmem server]
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
What if the server is already running? Either 2 domains have server start='yes' or the user started it already. We should not fail in these cases, I guess.
[snip]
It would be great if the domain with server start='yes' didn't have to be started first and killed last. Or if we could just signal the server (let's say SIGUSR1) that it should unlink the shmem and quit if no domains are connected. That way we could just send a signal to the server whenever any domain is disconnecting (maybe some check that no domain is being started might be good too ;) ).
Why not.
If we want to automagically start/stop the server, we don't need the 'start' field, but we need a way to find if a server is running for this shared mem instance.
We can try to bind the unix socket and check if a EADDRINUSE is returned. If EADDRINUSE, server is running, nothing to do. Else libvirt starts the server with the option "-q" (new option I can add which means "quit on last client disconnect")
On stop, libvirt does nothing about the ivshmem-server.
With this, if the server had been started with "-q" (by libvirt or by the user), then the server will quit on the last disconnect. If the user did not start the server with -q, it is his reponsibility to stop it.
The 'start' field can disappear. No need to send any signal to ivshmem-server.
What do you think of this ?
Great! Really, and it's even easier than what I thought of. We just need to kill the server if we fail to start QEMU after starting the server. There is one caveat, though. There is a race between libvirt checking whether the socket exists and last domain disconnecting. We also need to be sure that the server is running and accepts the connection from QEMU. And we need to be able to specify the SELinux context of the socket before it is created. Adding SELinux support into ivshmem-server would be too much, however. Second and third points could be completely eliminated by the server accepting LISTEN_FDS environment variable which would say that libvirt is passing an FD with the socket already opened (libvirt would be sure QEMU is already connected to the socket, and it would take care of the permissions). To see how this works you can have a looks at our daemon code for libvirtd or virtlock, but even easier would probably be to check systemd's documentation on socket activation (even though this has nothing to do with systemd). I coded it up without systemd based on: http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html and some other pages. Trying to understand it from libvirt sources might be an overkill. I'm not sure, though, what to do with the first point (race between libvirt creating the socket to see that it exists and ivshmem disconnecting). Maybe libvirt could do this (if QEMU would support it): 1: try to create the socket (if it exists, continue with 4) 2: connect to the socket 3: if connecting fails, goto 1; 4: if libvirt was the one who created the socket, start the server and pass the FD of the socket to it 5: start qemu and pass the socket to it (qemu already supports that with other devices like netdevs, etc. This would take care of all three points. No race, no permission issues, nothing. What do you think?
[snip]
+/** + * virStopIvshmemServer: + * @shm_name: the name of the shared memory + * + * Stop an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStopIvshmemServer(const char *shm_name) +{ + char *ivshmserver_pidbase = NULL; + pid_t ivshmserver_pid; + int ret = -1; + + /* get pid of the ivshmem server */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase, + &ivshmserver_pid)) + goto cleanup; + + virProcessKill(ivshmserver_pid, SIGTERM); + virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);
The pidfile support could be added to the server too, maybe...
I am not sure I understand the point.
The only thing this code does here is retrieve the current pid for the ivshmem-server, it kills the associated pid then remove the pidfile.
So what do you mean by "pidfile support" ? Do you suggest that, on exit, the ivshmem-server should remove the pidfile it first created ?
Disregard this comment, our above idea takes care of anything I might have meant in here :) Martin
On 08/28/2014 12:34 PM, Martin Kletzander wrote:
Great! Really, and it's even easier than what I thought of. We just need to kill the server if we fail to start QEMU after starting the server.
There is one caveat, though. There is a race between libvirt checking whether the socket exists and last domain disconnecting. We also need to be sure that the server is running and accepts the connection from QEMU. And we need to be able to specify the SELinux context of the socket before it is created. Adding SELinux support into ivshmem-server would be too much, however.
Second and third points could be completely eliminated by the server accepting LISTEN_FDS environment variable which would say that libvirt is passing an FD with the socket already opened (libvirt would be sure QEMU is already connected to the socket, and it would take care of the permissions). To see how this works you can have a looks at our daemon code for libvirtd or virtlock, but even easier would probably be to check systemd's documentation on socket activation (even though this has nothing to do with systemd). I coded it up without systemd based on:
http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
and some other pages. Trying to understand it from libvirt sources might be an overkill.
I'm not sure, though, what to do with the first point (race between libvirt creating the socket to see that it exists and ivshmem disconnecting). Maybe libvirt could do this (if QEMU would support it):
1: try to create the socket (if it exists, continue with 4)
2: connect to the socket
3: if connecting fails, goto 1;
4: if libvirt was the one who created the socket, start the server and pass the FD of the socket to it
5: start qemu and pass the socket to it (qemu already supports that with other devices like netdevs, etc.
This would take care of all three points. No race, no permission issues, nothing.
What do you think?
- Mmm, I had felt that there could be a race in the socket check, yes. The LISTEN_FDS support in the server is not that big of a change. I think I can take care of that. - Did not think of the other points. I think there is still some problem with your proposition, I need more time to think about it (may be some prototyping to be sure).
[snip]
+/** + * virStopIvshmemServer: + * @shm_name: the name of the shared memory + * + * Stop an Ivshmem Server + * + * Returns 0 in case of success or -1 in case of failure. + */ +int +virStopIvshmemServer(const char *shm_name) +{ + char *ivshmserver_pidbase = NULL; + pid_t ivshmserver_pid; + int ret = -1; + + /* get pid of the ivshmem server */ + if (!(ivshmserver_pidbase = virIvshmemServerPidFileBasename(shm_name))) + goto cleanup; + if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase, + &ivshmserver_pid)) + goto cleanup; + + virProcessKill(ivshmserver_pid, SIGTERM); + virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);
The pidfile support could be added to the server too, maybe...
I am not sure I understand the point.
The only thing this code does here is retrieve the current pid for the ivshmem-server, it kills the associated pid then remove the pidfile.
So what do you mean by "pidfile support" ? Do you suggest that, on exit, the ivshmem-server should remove the pidfile it first created ?
Disregard this comment, our above idea takes care of anything I might have meant in here :)
Yes, I thought so, no problem. Thanks Martin. -- David Marchand
On 08/28/2014 02:26 PM, David Marchand wrote:
I'm not sure, though, what to do with the first point (race between libvirt creating the socket to see that it exists and ivshmem disconnecting). Maybe libvirt could do this (if QEMU would support it):
1: try to create the socket (if it exists, continue with 4)
2: connect to the socket
3: if connecting fails, goto 1;
4: if libvirt was the one who created the socket, start the server and pass the FD of the socket to it
5: start qemu and pass the socket to it (qemu already supports that with other devices like netdevs, etc.
This would take care of all three points. No race, no permission issues, nothing.
What do you think?
- Mmm, I had felt that there could be a race in the socket check, yes. The LISTEN_FDS support in the server is not that big of a change. I think I can take care of that.
- Did not think of the other points. I think there is still some problem with your proposition, I need more time to think about it (may be some prototyping to be sure).
I had misunderstood something about listen()/connect(). Ok, your proposition looks good to me. At least for the server, this should be transparent as long as the server handles LISTEN_FDS env variable or an option to tell it which fd he should listen on. For the ivshmem part in QEMU itself, I think adding a property to ivshmem pci class should do the trick, will see if I (or Maxime) can do this. Are there any other points concerning the server ? -- David Marchand
On Thu, Aug 28, 2014 at 03:46:47PM +0200, David Marchand wrote:
On 08/28/2014 02:26 PM, David Marchand wrote:
I'm not sure, though, what to do with the first point (race between libvirt creating the socket to see that it exists and ivshmem disconnecting). Maybe libvirt could do this (if QEMU would support it):
1: try to create the socket (if it exists, continue with 4)
2: connect to the socket
3: if connecting fails, goto 1;
4: if libvirt was the one who created the socket, start the server and pass the FD of the socket to it
5: start qemu and pass the socket to it (qemu already supports that with other devices like netdevs, etc.
This would take care of all three points. No race, no permission issues, nothing.
What do you think?
- Mmm, I had felt that there could be a race in the socket check, yes. The LISTEN_FDS support in the server is not that big of a change. I think I can take care of that.
- Did not think of the other points. I think there is still some problem with your proposition, I need more time to think about it (may be some prototyping to be sure).
I had misunderstood something about listen()/connect(). Ok, your proposition looks good to me.
At least for the server, this should be transparent as long as the server handles LISTEN_FDS env variable or an option to tell it which fd he should listen on.
Parameter is fine, too, I was probably just thinking about the LISTEN_FDS stuff too much due to having some trouble implementing it in libvirtd.
For the ivshmem part in QEMU itself, I think adding a property to ivshmem pci class should do the trick, will see if I (or Maxime) can do this.
Great. One more minor thing, though. In libvirt, we need to have the option to know whether QEMU supports that newly added option. We are introspecting such things using QMP, so if it's a device property, we should be able to get that.
Are there any other points concerning the server ?
Not that I know of (yet). Feel free to Cc me on the patches for the ivshmem stuff in qemu-devel. Martin
On 2014/8/22 18:47, Maxime Leroy wrote:
+# util/virivshmemserver.h +virStartIvshmemServer; +virStopIvshmemServer;
I think function name virIvshmemStartServer is better. So is the stop function.
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.
return 0; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index baa866a..aaf03a3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -67,6 +67,7 @@ #include "virstring.h" #include "virhostdev.h" #include "storage/storage_driver.h" +#include "virivshmemserver.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -4684,6 +4685,15 @@ void qemuProcessStop(virQEMUDriverPtr driver, } }
+ /* stop runnning ivshmem server */ + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM &&
You can use switch (for extension other shmem in future).
On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui <moon.wangrui@huawei.com> wrote:
On 2014/8/22 18:47, Maxime Leroy wrote:
+# util/virivshmemserver.h +virStartIvshmemServer; +virStopIvshmemServer;
I think function name virIvshmemStartServer is better. So is the stop function.
What about virIvshmemServerStart ?
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.
Calling virStartIvshmemServer in qemuProcessStart should be better ? Maxime
On 2014/8/28 5:20, Maxime Leroy wrote:
On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui <moon.wangrui@huawei.com> wrote:
On 2014/8/22 18:47, Maxime Leroy wrote:
+# util/virivshmemserver.h +virStartIvshmemServer; +virStopIvshmemServer;
I think function name virIvshmemStartServer is better. So is the stop function.
What about virIvshmemServerStart ?
It looks fine.
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.
Calling virStartIvshmemServer in qemuProcessStart should be better ?
Looks better, too. But we'd better to wait for other opinions.
Maxime
On Thu, Aug 28, 2014 at 02:35:10PM +0800, Wang Rui wrote:
On 2014/8/28 5:20, Maxime Leroy wrote:
On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui <moon.wangrui@huawei.com> wrote:
On 2014/8/22 18:47, Maxime Leroy wrote:
+# util/virivshmemserver.h +virStartIvshmemServer; +virStopIvshmemServer;
I think function name virIvshmemStartServer is better. So is the stop function.
What about virIvshmemServerStart ?
It looks fine.
Yes, it looks better.
@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd, return -1; virCommandAddArg(cmd, devstr); VIR_FREE(devstr); + + if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) { + if (virStartIvshmemServer(dev->name, ivshmem->server.path, + ivshmem->size, ivshmem->msi.vectors)) + return -1; + } }
I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.
Calling virStartIvshmemServer in qemuProcessStart should be better ?
Looks better, too. But we'd better to wait for other opinions.
It should be started as late as possible and stopped if no client connects there. This will need more changes I think, we'll discuss it with David in the other thread. Martin
Hello Martin, Let me summarize the points we need for the next patchset version: 1) merge patches - doc: schema, conf: Parse and format and tests ( for xml2xml ) into doc: schema: conf: Add shmem node - qemu: Build comand, qemu: Add cap flag CAPS_IVSHMEM, and tests (for xml2argv) into qemu: add ivshmem support 2) parsing shmem - remove <shmem> 'model' attribute - use _uip instand of ui - check size >= 1M - remove loop to parse childreen nodes - path attribute is optional, default path is /var/run/libvirt/ivshmem-server/<name>-sock 3) tests: - add pci addresses in the XML 4) xml format - no ivshmem server: <shmem name='shmem0'> (name is a mandatory attribute) <size unit='M'>32</size> (optionnal, default value: 4MB, size >= 1M and power of 2) </shmem> - ivshmem server with no path to the socket file <shmem name='shmem0'> (name is a mandatory attribute) <server> <msi vectors='32' ioeventfd='on'/> (optionnal) </server> <size unit='M'>32</size> (optionnal) </shmem> default path is : /var/run/libvirt/ivshmem-server/<name>-sock - ivshmem server with path to a specific socket file <shmem name='shmem0'> <server path='/tmp/shmem0-sock'> <msi vectors='32' ioeventfd='on'/> (optionnal) </server> <size unit='M'>32</size> (optionnal) </shmem> The name attribute is only needed if libvirt starts the ivshmem server. In the case of the ivshmem-server is already running, the path attribute is enough. To simplify, I think the name attribute should be always mandatory ? What do you think about this new xml format ? 5) ivshmem server - remame virStartIvshmemServer to virIvshmemServerStart - call virIvshmemServerStart in qemuProcessStart instead to qemuBuild*CommandLine - rename IVSHMEMSERVER to IVSHMEM_SERVER - autostart/stop ivshmem server I really like the idea to use the new option '-q' to stop automatically the server and the new option to pass the fd to ivshmem-server and qemu. It's an elegant solution. ;) Anyway, I need to wait to see if theses options can be integrated in qemu before to submit a new patchset for libvirt. -- Maxime
On Wed, Sep 03, 2014 at 12:56:00PM +0200, Maxime Leroy wrote:
Hello Martin,
Let me summarize the points we need for the next patchset version:
1) merge patches - doc: schema, conf: Parse and format and tests ( for xml2xml ) into doc: schema: conf: Add shmem node - qemu: Build comand, qemu: Add cap flag CAPS_IVSHMEM, and tests (for xml2argv) into qemu: add ivshmem support
2) parsing shmem - remove <shmem> 'model' attribute - use _uip instand of ui - check size >= 1M
I don't know how and where we ended up with this, but feel free to keep this in the parsing code, we can make it lax (or move it to qemu.*PortParse() any time in the future.
- remove loop to parse childreen nodes - path attribute is optional, default path is /var/run/libvirt/ivshmem-server/<name>-sock
3) tests: - add pci addresses in the XML
4) xml format
- no ivshmem server:
<shmem name='shmem0'> (name is a mandatory attribute) <size unit='M'>32</size> (optionnal, default value: 4MB, size >= 1M and power of 2)
I also had problems running qemu with size > 4MB, but I don't know where the problem was. We should do anything we can so that qemu doesn't fail (if there's the possibility). Just add the restrictions into the documentation, please.
</shmem>
- ivshmem server with no path to the socket file
<shmem name='shmem0'> (name is a mandatory attribute) <server> <msi vectors='32' ioeventfd='on'/> (optionnal) </server> <size unit='M'>32</size> (optionnal) </shmem>
default path is : /var/run/libvirt/ivshmem-server/<name>-sock
- ivshmem server with path to a specific socket file
<shmem name='shmem0'> <server path='/tmp/shmem0-sock'> <msi vectors='32' ioeventfd='on'/> (optionnal) </server> <size unit='M'>32</size> (optionnal) </shmem>
The name attribute is only needed if libvirt starts the ivshmem server. In the case of the ivshmem-server is already running, the path attribute is enough. To simplify, I think the name attribute should be always mandatory ?
That's fine with me, we can always make it optional later on.
What do you think about this new xml format ?
Looks great, I'd just add one little bit, which I probably mentioned it earlier. I think that whatever the last patch (ivshmem server starting) will enable in the XML (e.g. <server start="yes">) should be disabled in the first part (probably during parsing). Would that be OK with you?
5) ivshmem server
- remame virStartIvshmemServer to virIvshmemServerStart - call virIvshmemServerStart in qemuProcessStart instead to qemuBuild*CommandLine - rename IVSHMEMSERVER to IVSHMEM_SERVER - autostart/stop ivshmem server
I really like the idea to use the new option '-q' to stop automatically the server and the new option to pass the fd to ivshmem-server and qemu. It's an elegant solution. ;)
Good to hear that.
Anyway, I need to wait to see if theses options can be integrated in qemu before to submit a new patchset for libvirt.
Feel free to leave the server part for later if you want to concentrate on the first shmem part only now. Everything looks fine, I think the next version might get in (unless somebody finds something both of us missed) :) Have a nice day, Martin
participants (4)
-
David Marchand -
Martin Kletzander -
Maxime Leroy -
Wang Rui