Re: [libvirt] [PATCH] Add XML config switch to enable/disable vhost-net support

On Thu, Jan 13, 2011 at 01:45:28AM -0500, Laine Stump 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 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.

> 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.