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