[libvirt] [PATCH] Add XML config switch to enable/disable vhost-net support
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' (So far the strongest opinion has been for the current "name='vhost|qemu'") Oh, and also - sorry Eric, but I didn't have the brain cells left tonight to add this new bit to the documentation, and I really want to get the patch up/in now, so that will have to wait for a followup next week :-) docs/schemas/domain.rng | 13 ++++++++ src/conf/domain_conf.c | 27 +++++++++++++++++- src/conf/domain_conf.h | 10 ++++++ src/qemu/qemu_command.c | 71 +++++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 -- 5 files changed, 108 insertions(+), 16 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index a524e4b..6d0654d 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1005,6 +1005,19 @@ </element> </optional> <optional> + <element name="driver"> + <optional> + <attribute name="name"> + <choice> + <value>qemu</value> + <value>vhost</value> + </choice> + </attribute> + </optional> + <empty/> + </element> + </optional> + <optional> <ref name="address"/> </optional> <optional> 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; union { struct { char *dev; @@ -1264,6 +1273,7 @@ VIR_ENUM_DECL(virDomainControllerModel) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainFSAccessMode) VIR_ENUM_DECL(virDomainNet) +VIR_ENUM_DECL(virDomainNetBackend) VIR_ENUM_DECL(virDomainChrDevice) VIR_ENUM_DECL(virDomainChrChannelTarget) VIR_ENUM_DECL(virDomainChrConsoleTarget) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 86c5bb5..9eb54a1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -302,24 +302,61 @@ cleanup: } -int +static int qemuOpenVhostNet(virDomainNetDefPtr net, - unsigned long long qemuCmdFlags) + unsigned long long qemuCmdFlags, + int *vhostfd) { - /* If qemu supports vhost-net mode (including the -netdev command - * option), the nic model is virtio, and we can open - * /dev/vhost_net, assume that vhost-net mode is available and - * return the fd to /dev/vhost_net. Otherwise, return -1. - */ + *vhostfd = -1; /* assume we won't use vhost */ + /* If the config says explicitly to not use vhost, return now */ + if (net->backend_specified && + (net->backend == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)) { + return 0; + } + + /* If qemu doesn't support vhost-net mode (including the -netdev command + * option), don't try to open the device. + */ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_VNET_HOST && qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV && - qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE && - net->model && STREQ(net->model, "virtio"))) - return -1; + qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { + if (net->backend_specified && + (net->backend == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net is not supported with " + "this QEMU binary")); + return -1; + } + return 0; + } - return open("/dev/vhost-net", O_RDWR, 0); + /* If the nic model isn't virtio, don't try to open. */ + if (!(net->model && STREQ(net->model, "virtio"))) { + if (net->backend_specified && + (net->backend == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net is only supported for " + "virtio network interfaces")); + return -1; + } + return 0; + } + + *vhostfd = open("/dev/vhost-net", O_RDWR, 0); + + /* If the config says explicitly to use vhost and we couldn't open it, + * report an error. + */ + if ((*vhostfd < 0) && net->backend_specified && + (net->backend == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net was requested for an interface, " + "but is unavailable")); + return -1; + } + return 0; } @@ -3278,7 +3315,10 @@ qemuBuildCommandLine(virConnectPtr conn, net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ - int vhostfd = qemuOpenVhostNet(net, qemuCmdFlags); + int vhostfd; + + if (qemuOpenVhostNet(net, qemuCmdFlags, &vhostfd) < 0) + goto error; if (vhostfd >= 0) { virCommandTransferFD(cmd, vhostfd); @@ -4618,6 +4658,13 @@ qemuParseCommandLineNet(virCapsPtr caps, } else if (STREQ(keywords[i], "model")) { def->model = values[i]; values[i] = NULL; + } else if (STREQ(keywords[i], "vhost")) { + if ((values[i] == NULL) || STREQ(values[i], "on")) { + def->backend = VIR_DOMAIN_NET_BACKEND_TYPE_VHOST; + } else if (STREQ(keywords[i], "off")) { + def->backend = VIR_DOMAIN_NET_BACKEND_TYPE_QEMU; + } + def->backend_specified = 1; } } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 4c42a10..5439184 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -116,9 +116,6 @@ int qemuNetworkIfaceConnect(virConnectPtr conn, unsigned long long qemCmdFlags) ATTRIBUTE_NONNULL(1); -int qemuOpenVhostNet(virDomainNetDefPtr net, - unsigned long long qemuCmdFlags); - int qemuPhysIfaceConnect(virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, -- 1.7.3.4
On 01/12/2011 11:45 PM, 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, ... (So far the strongest opinion has been for the current "name='vhost|qemu'")
Also, using name= as the attribute name matches the fact that we already have <driver name='xyz'> for <disk>.
Oh, and also - sorry Eric, but I didn't have the brain cells left tonight to add this new bit to the documentation, and I really want to get the patch up/in now, so that will have to wait for a followup next week :-)
I'll have to live with that, but come closer to the 0.8.8 release date, watch for the reminders if you haven't followed through :) Meanwhile, https://bugzilla.redhat.com/show_bug.cgi?id=643050#c3 hints that we may yet need to make an explicit third state (particularly if virsh dumpxml always lists the chosen driver rather than staying silent in the default case) - it's easier to add a third state later than it is to wish we hadn't added it up front, so I'm glad that your initial patch only proposes two valid strings in the XML.
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index a524e4b..6d0654d 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1005,6 +1005,19 @@ </element> </optional> <optional> + <element name="driver"> + <optional> + <attribute name="name">
Thinking aloud: This permits <driver/> with no attributes, which looks weird. For comparison with the <disk> element, the rng requires the driver element has to have at least one attribute from the set (name, cache), which means name is optional there. Which means on the one hand, <disk> will never have <driver/> without attributes, but on the other hand, why should name be mandatory here if it is optional there. I guess that means this is fine to keep the <optional> here.
@@ -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"),
s/Unkown/Unknown/
+ backend); + goto error; + } + def->backend = b; + def->backend_specified = 1;
It seems like our debate on IRC has been whether this two-variable tracking is better than a three-way enum value. If we go with the three-way enum (default, qemu, vhost), then you only need one variable, but this line of code needs one extra check that rejects STREQ(backend,"default") (or else the XML parsing would silently accept name='default' but discard it). [for comparison with your sndbuf patch - there, you have to use sndbuf_specified, since sndbuf==0 is a valid but non-default configuration. But here, using a three-state enum provides a perfect default without having to track two variables, at the cost of having to check at parse time that only two of the three states can be explicitly used, because the default third state happens by the absence of an explicit request.]
@@ -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) {
with a tri-state enum, here, the condition would be def->backend != VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT
+/* the backend driver used for virtio interfaces */ +enum virDomainNetBackendType {
and here, you would have VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT = 0, /* try kernel, but fall back to userland */
+ 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;
and here you could lose backend_specified. I guess another advantage of keeping a tri-state enum rather than two variables is that if the request to have an explicit name='default' ever materializes in the future, we would already have the enum set up for that.
+ + *vhostfd = open("/dev/vhost-net", O_RDWR, 0);
The third argument is not necessary, since the second does not include O_CREAT. I can live with the patch as-is (once you fix the spelling and open() nits), but if you have time, I wouldn't mind a v2 with the approach of a tri-state enum. So, you have a weak: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/13/2011 03:59 PM, Eric Blake wrote:
On 01/12/2011 11:45 PM, 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, ... (So far the strongest opinion has been for the current "name='vhost|qemu'")
Also, using name= as the attribute name matches the fact that we already have<driver name='xyz'> for<disk>.
Yep, I'm in the club :-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index a524e4b..6d0654d 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1005,6 +1005,19 @@ </element> </optional> <optional> +<element name="driver"> +<optional> +<attribute name="name"> Thinking aloud: This permits<driver/> with no attributes, which looks weird. For comparison with the<disk> element, the rng requires the driver element has to have at least one attribute from the set (name, cache), which means name is optional there. Which means on the one hand,<disk> will never have<driver/> without attributes, but on the other hand, why should name be mandatory here if it is optional there. I guess that means this is fine to keep the<optional> here.
And especially since the rng is only used in tests that validate the rng, I'm inclined to leave it optional...
@@ -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"), s/Unkown/Unknown/
Right. I think this is the result of the copy-paste source I chose (search for "Unkown"). I need to send a patch for that.
+ backend); + goto error; + } + def->backend = b; + def->backend_specified = 1; It seems like our debate on IRC has been whether this two-variable tracking is better than a three-way enum value. If we go with the three-way enum (default, qemu, vhost), then you only need one variable, but this line of code needs one extra check that rejects STREQ(backend,"default") (or else the XML parsing would silently accept name='default' but discard it).
Good idea. I chose to generate an error on name='default'. Shortens up the code, eliminates non-zero default; an all around win!
+ *vhostfd = open("/dev/vhost-net", O_RDWR, 0); The third argument is not necessary, since the second does not include O_CREAT.
Okay. That's actually pre-existing code that I moved slightly, but I'll use this oppurtunity to fix it.
I can live with the patch as-is (once you fix the spelling and open() nits), but if you have time, I wouldn't mind a v2 with the approach of a tri-state enum. So, you have a weak:
V2 coming up!
On 01/13/2011 01:59 PM, Eric Blake wrote:
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.
Oh, and also - sorry Eric, but I didn't have the brain cells left tonight to add this new bit to the documentation, and I really want to get the patch up/in now, so that will have to wait for a followup next week :-)
I'll have to live with that, but come closer to the 0.8.8 release date, watch for the reminders if you haven't followed through :)
One of those friendly reminders, since I was looking for that documentation today and missed having it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* docs/formatdomain.html.in: Document virtio backend selection. --- How about I help out? :) docs/formatdomain.html.in | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c7c965..8307a0d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1388,7 +1388,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver txmode='iothread'/></b> + <b><driver name='vhost' txmode='iothread'/></b> </interface> </devices> ...</pre> @@ -1401,6 +1401,18 @@ qemu-kvm -net nic,model=? /dev/null </p> <dl> + <dt><code>name</code></dt> + <dd> + The optional <code>name</code> attribute forces which type of + backend driver to use. The value can be either 'qemu' (a + user-space backend) or 'vhost' (a kernel backend, which + requires the vhost module to be provided by the kernel); an + attempt to require the vhost driver without kernel support + will be rejected. If this attribute is not present, then the + domain defaults to 'vhost' if present, but silently falls back + to 'qemu' without error. + <span class="since">Since 0.8.8 (QEMU and KVM only)</span> + </dd> <dt><code>txmode</code></dt> <dd> The <code>txmode</code> attribute specifies how to handle -- 1.7.4
On 03/08/2011 06:38 PM, Eric Blake wrote:
* docs/formatdomain.html.in: Document virtio backend selection. ---
How about I help out? :)
docs/formatdomain.html.in | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0c7c965..8307a0d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1388,7 +1388,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver txmode='iothread'/></b> + <b><driver name='vhost' txmode='iothread'/></b> </interface> </devices> ...</pre> @@ -1401,6 +1401,18 @@ qemu-kvm -net nic,model=? /dev/null </p>
<dl> + <dt><code>name</code></dt> + <dd> + The optional <code>name</code> attribute forces which type of + backend driver to use. The value can be either 'qemu' (a + user-space backend) or 'vhost' (a kernel backend, which + requires the vhost module to be provided by the kernel); an + attempt to require the vhost driver without kernel support + will be rejected. If this attribute is not present, then the + domain defaults to 'vhost' if present, but silently falls back + to 'qemu' without error. + <span class="since">Since 0.8.8 (QEMU and KVM only)</span> + </dd> <dt><code>txmode</code></dt> <dd> The <code>txmode</code> attribute specifies how to handle
ACK - Cole
On 03/09/2011 06:48 AM, Cole Robinson wrote:
On 03/08/2011 06:38 PM, Eric Blake wrote:
* docs/formatdomain.html.in: Document virtio backend selection. ---
@@ -1401,6 +1401,18 @@ qemu-kvm -net nic,model=? /dev/null </p>
<dl> + <dt><code>name</code></dt> + <dd> + The optional <code>name</code> attribute forces which type of + backend driver to use. The value can be either 'qemu' (a + user-space backend) or 'vhost' (a kernel backend, which + requires the vhost module to be provided by the kernel); an + attempt to require the vhost driver without kernel support + will be rejected. If this attribute is not present, then the + domain defaults to 'vhost' if present, but silently falls back + to 'qemu' without error. + <span class="since">Since 0.8.8 (QEMU and KVM only)</span> + </dd> <dt><code>txmode</code></dt> <dd> The <code>txmode</code> attribute specifies how to handle
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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. Regards, Daniel
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.
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Laine Stump