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

[Laine Stump]

On 01/14/2011 08:22 AM, Daniel P. Berrange wrote:
> 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.


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.