On 01/14/2011 08:22 AM, Daniel P. Berrange wrote:
This patch is in response to
https://bugzilla.redhat.com/show_bug.cgi?id=643050
The existing libvirt support for the vhost-net backend to the virtio network driver happens automatically - if the vhost-net device is available, it is always enabled, otherwise the standard userland virtio backend is used.
This patch makes it possible to force whether or not vhost-net is used with a bit of XML. Adding a<driver> element to the interface XML, eg:
<interface type="network"> <model type="virtio"/> <driver name="vhost"/>
will force use of vhost-net (if it's not available, the domain will fail to start). if driver name="qemu", vhost-net will not be used even if it is available.
If there is no<driver name='xxx'/> in the config, libvirt will revert to the pre-existing automatic behavior - use vhost-net if it's available, and userland backend if vhost-net isn't available. ---
Note that I don't really like the "name='vhost|qemu'" nomenclature, but am including it here just to get the patches on the list. I could live with it this way, or with any of the following (anyone have a strong opinion?) (note that in all cases, nothing specified means "try to use vhost, but fallback to userland if necessary")
vhost='on|off' vhost='required|disabled' mode='vhost|qemu' mode='kernel|user' backend='kernel|user' I wanted 'name=' becasue that matches<driver> usage in<disk>. This rules out the first options completely, since we can just
On Thu, Jan 13, 2011 at 01:45:28AM -0500, Laine Stump wrote: play with attribute values. kernel|user is nice, except when QEMU invent vhost2, and now 'kernel' is no longer unique :-( Also we used 'name=qemu' already in<disk> to refer to the in-QEMU disk backend, and there's likely to be a 'vhost' backend for disks too in the future. So in summary I think 'name=vhost|qemu' is the best optionl.
Okay, I'm convinced! :-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4df38c..04ed502 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -184,6 +184,10 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct")
+VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST, + "qemu", + "vhost") + VIR_ENUM_IMPL(virDomainChrChannelTarget, VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST, "guestfwd", @@ -2289,6 +2293,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *address = NULL; char *port = NULL; char *model = NULL; + char *backend = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -2371,6 +2376,8 @@ virDomainNetDefParseXML(virCapsPtr caps, script = virXMLPropString(cur, "path"); } else if (xmlStrEqual (cur->name, BAD_CAST "model")) { model = virXMLPropString(cur, "type"); + } else if (xmlStrEqual (cur->name, BAD_CAST "driver")) { + backend = virXMLPropString(cur, "name"); } else if (xmlStrEqual (cur->name, BAD_CAST "filterref")) { filter = virXMLPropString(cur, "filter"); VIR_FREE(filterparams); @@ -2558,6 +2565,18 @@ virDomainNetDefParseXML(virCapsPtr caps, model = NULL; }
+ if ((backend != NULL)&& + (def->model&& STREQ(def->model, "virtio"))) { + int b; + if ((b = virDomainNetBackendTypeFromString(backend))< 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Unkown interface<driver name='%s'> has been specified"), + backend); + goto error; + } + def->backend = b; + def->backend_specified = 1; + } if (filter != NULL) { switch (def->type) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -2584,6 +2603,7 @@ cleanup: VIR_FREE(script); VIR_FREE(bridge); VIR_FREE(model); + VIR_FREE(backend); VIR_FREE(filter); VIR_FREE(type); VIR_FREE(internal); @@ -6275,9 +6295,14 @@ virDomainNetDefFormat(virBufferPtr buf, if (def->ifname) virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); - if (def->model) + if (def->model) { virBufferEscapeString(buf, "<model type='%s'/>\n", def->model); + if (STREQ(def->model, "virtio")&& def->backend_specified) { + virBufferVSprintf(buf, "<driver name='%s'/>\n", + virDomainNetBackendTypeToString(def->backend)); + } + } if (def->filter) { virBufferEscapeString(buf, "<filterref filter='%s'", def->filter); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a459a22..451ccad 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -292,6 +292,13 @@ enum virDomainNetType { VIR_DOMAIN_NET_TYPE_LAST, };
+/* the backend driver used for virtio interfaces */ +enum virDomainNetBackendType { + VIR_DOMAIN_NET_BACKEND_TYPE_QEMU, /* userland */ + VIR_DOMAIN_NET_BACKEND_TYPE_VHOST, /* kernel */ + + VIR_DOMAIN_NET_BACKEND_TYPE_LAST, +};
/* the mode type for macvtap devices */ enum virDomainNetdevMacvtapType { @@ -310,6 +317,8 @@ struct _virDomainNetDef { enum virDomainNetType type; unsigned char mac[VIR_MAC_BUFLEN]; char *model; + enum virDomainNetBackendType backend; + int backend_specified : 1; I don't really like this pattern since it is at odds with the way we handle defaults elsewhere which is to include the default as the first enum value, eg
enum virDomainNetBackendType { VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT, /* Try best available */ VIR_DOMAIN_NET_BACKEND_TYPE_QEMU, /* userland */ VIR_DOMAIN_NET_BACKEND_TYPE_VHOST, /* kernel */
VIR_DOMAIN_NET_BACKEND_TYPE_LAST, };
And then we do
if (def->backend) ...print XML...
when formatting the XML, so that the 'name=default' never actually appears in the XML output.
Right. What I didn't like was that, using the VIR_ENUM_IMPL() macro, we would end up giving the user the ability to put <driver name='default'/> into the XML, which would then disappear. In his review, Eric pointed out the elegant solution of simply generating an error during parse if they try to enter that value; everything else will already do what we want. So that's what I'm doing in V2.