On 09/18/2014 10:15 AM, Ján Tomko wrote:

Add options for tuning segment offloading: <driver> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> which control the respective host_ and guest_ properties of the virtio-net device. --- docs/formatdomain.html.in | 24 ++- docs/schemas/domaincommon.rng | 44 ++++- src/conf/domain_conf.c | 218 ++++++++++++++++++++- src/conf/domain_conf.h | 15 ++ .../qemuxml2argv-net-virtio-disable-offloads.xml | 35 ++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c03ebb..5dd70a4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + </driver> + </b> </interface> </devices> ...</pre> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null processor, resulting in much higher throughput. <span class="since">Since 1.0.6 (QEMU and KVM only)</span> </dd> + <dt><code>host offloading options</code></dt>

Should this be <code>host</host> offloading options? or host TCP options (in some way to indicate that these are TCP level options). Probably also want your disclaimer use of these should be for only those that know what they are doing...

+ <dd> + The <code>csum</code>, <code>gso</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>

Should read: ecn, and ufo Should you "spell out" what each translates to? csum (checksums), gso (generic segmentation offload), tso (tcp segmentation offload v4 and v6), ecn (congestion notification), and ufo (UDP fragment offloads)

+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn host offloads off.

s/turn host offloads off/turn off host TCP options/ Saying "offloads off" aloud just doesn't seem right.

+ By default, the supported offloads are enabled by QEMU.

s/the supported offloads/the TCP options/

+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> + <dt>guest offloading options</dt>

Similar in here... Does the host setting override the guest value or can the host say "tso4=off" while the guest has "tso4=on"? Curious mainly.

+ <dd> + The <code>csum</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>

same here with the "and" - although I suppose you could just reference the <host> bits "above"... Another way to say it is guests can use the same options except gso

+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn guest offloads off.

s/turn guest offloads off/turn off guest TCP options/

+ By default, the supported offloads are enabled by QEMU.

s/the supported offloads/the TCP options/ And of course the usage disclaimer!

+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> </dl>

<h5><a name="elementsBackendOptions">Setting network backend-specific options</a></h5> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19dc82f..1e00afd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2364,7 +2364,49 @@ </optional> </group> </choice> - <empty/> + <interleave> + <optional> + <element name='host'> + <attribute name='csum'> + <ref name="virOnOff"/> + </attribute> + <attribute name='gso'> + <ref name="virOnOff"/> + </attribute> + <attribute name='tso4'> + <ref name="virOnOff"/> + </attribute> + <attribute name='tso6'> + <ref name="virOnOff"/> + </attribute> + <attribute name='ecn'> + <ref name="virOnOff"/> + </attribute> + <attribute name='ufo'> + <ref name="virOnOff"/> + </attribute> + </element> + </optional> + <optional> + <element name='guest'> + <attribute name='csum'> + <ref name="virOnOff"/> + </attribute> + <attribute name='tso4'> + <ref name="virOnOff"/> + </attribute> + <attribute name='tso6'> + <ref name="virOnOff"/> + </attribute> + <attribute name='ecn'> + <ref name="virOnOff"/> + </attribute> + <attribute name='ufo'> + <ref name="virOnOff"/> + </attribute> + </element> + </optional> + </interleave> </element> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ccec1c..cdaafa6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *str = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -7385,6 +7386,115 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.queues = q; }

How about something like this? Not that you have anything wrong, but it avoids the chance that some "change" in the future requires 11 similar changes... #define XPATH_TCP_OPTION(PATH,OPTION) \ if ((str = virXPathString("string(./driver/"#PATH"/@"#OPTION")", \ ctxt))) { \ if ((val = virTristateSwitchTypeFromString(str)) <= 0) { \ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ _("unknown " #PATH " " #OPTION " mode '%s'"), \ str); \ goto error; \ } \ def->driver.virtio.PATH.OPTION = val; \ } \ VIR_FREE(str); XPATH_TCP_OPTION(host, csum); XPATH_TCP_OPTION(host, gso); XPATH_TCP_OPTION(host, tso4); XPATH_TCP_OPTION(host, tso6); XPATH_TCP_OPTION(host, ecn); XPATH_TCP_OPTION(host, ufo); XPATH_TCP_OPTION(guest, csum); XPATH_TCP_OPTION(guest, tso4); XPATH_TCP_OPTION(guest, tso6); XPATH_TCP_OPTION(guest, ecn); XPATH_TCP_OPTION(guest, ufo); #undef XPATH_TCP_OPTION Either way doesn't matter to me... Just trying to avoid repetitive code.

+ if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host csum mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.csum = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/host/@gso)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host gso mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.gso = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/host/@tso4)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host tso4 mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.tso4 = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/host/@tso6)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host tso6 mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.tso6 = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/host/@ecn)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host ecn mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.ecn = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/host/@ufo)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown host ufo mode '%s'"), + str); + goto error; + } + def->driver.virtio.host.ufo = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/guest/@csum)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown guest csum mode '%s'"), + str); + goto error; + } + def->driver.virtio.guest.csum = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/guest/@tso4)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown guest tso4 mode '%s'"), + str); + goto error; + } + def->driver.virtio.guest.tso4 = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/guest/@tso6)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown guest tso6 mode '%s'"), + str); + goto error; + } + def->driver.virtio.guest.tso6 = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/guest/@ecn)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown guest ecn mode '%s'"), + str); + goto error; + } + def->driver.virtio.guest.ecn = val; + } + VIR_FREE(str); + if ((str = virXPathString("string(./driver/guest/@ufo)", ctxt))) { + if ((val = virTristateSwitchTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown guest ufo mode '%s'"), + str); + goto error; + } + def->driver.virtio.guest.ufo = val; + } }

def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; @@ -7442,6 +7552,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(ioeventfd); VIR_FREE(event_idx); VIR_FREE(queues); + VIR_FREE(str); VIR_FREE(filter); VIR_FREE(type); VIR_FREE(internal); @@ -16486,6 +16597,80 @@ virDomainActualNetDefFormat(virBufferPtr buf,

static int +virDomainVirtioNetGuestOptsFormat(char **outstr, + virDomainNetDefPtr def) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + if (def->driver.virtio.guest.csum) { + virBufferAsprintf(&buf, "csum='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.guest.csum)); + } + if (def->driver.virtio.guest.tso4) { + virBufferAsprintf(&buf, "tso4='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.guest.tso4)); + } + if (def->driver.virtio.guest.tso6) { + virBufferAsprintf(&buf, "tso6='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.guest.tso6)); + } + if (def->driver.virtio.guest.ecn) { + virBufferAsprintf(&buf, "ecn='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.guest.ecn)); + } + if (def->driver.virtio.guest.ufo) { + virBufferAsprintf(&buf, "ufo='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.guest.ufo)); + } + virBufferTrim(&buf, " ", -1); + + if (virBufferCheckError(&buf) < 0) + return -1; + + *outstr = virBufferContentAndReset(&buf); + return 0; +} + + +static int +virDomainVirtioNetHostOptsFormat(char **outstr, + virDomainNetDefPtr def) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + if (def->driver.virtio.host.csum) { + virBufferAsprintf(&buf, "csum='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.csum)); + } + if (def->driver.virtio.host.gso) { + virBufferAsprintf(&buf, "gso='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.gso)); + } + if (def->driver.virtio.host.tso4) { + virBufferAsprintf(&buf, "tso4='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.tso4)); + } + if (def->driver.virtio.host.tso6) { + virBufferAsprintf(&buf, "tso6='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.tso6)); + } + if (def->driver.virtio.host.ecn) { + virBufferAsprintf(&buf, "ecn='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.ecn)); + } + if (def->driver.virtio.host.ufo) { + virBufferAsprintf(&buf, "ufo='%s' ", + virTristateSwitchTypeToString(def->driver.virtio.host.ufo)); + } + virBufferTrim(&buf, " ", -1); + + if (virBufferCheckError(&buf) < 0) + return -1; + + *outstr = virBufferContentAndReset(&buf); + return 0; +} + + +static int virDomainVirtioNetDriverFormat(char **outstr, virDomainNetDefPtr def) { @@ -16536,7 +16721,6 @@ virDomainNetDefFormat(virBufferPtr buf, virDomainHostdevDefPtr hostdef = NULL; char macstr[VIR_MAC_STRING_BUFLEN];

- if (publicActual) { if (!(typeStr = virDomainNetTypeToString(actualType))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -16695,14 +16879,36 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<model type='%s'/>\n", def->model); if (STREQ(def->model, "virtio")) { - char *str; + char *str = NULL, *gueststr = NULL, *hoststr = NULL; + int rc = 0;

- if (virDomainVirtioNetDriverFormat(&str, def) < 0) - return -1; + if (virDomainVirtioNetDriverFormat(&str, def) < 0 || + virDomainVirtioNetGuestOptsFormat(&gueststr, def) < 0 || + virDomainVirtioNetHostOptsFormat(&hoststr, def) < 0) + rc = -1;

- if (str) - virBufferAsprintf(buf, "<driver %s/>\n", str); + if (!gueststr && !hoststr) { + if (str) + virBufferAsprintf(buf, "<driver %s/>\n", str); + } else { + if (str) + virBufferAsprintf(buf, "<driver %s>\n", str); + else + virBufferAddLit(buf, "<driver>\n"); + virBufferAdjustIndent(buf, 2); + if (hoststr) + virBufferAsprintf(buf, "<host %s/>\n", hoststr); + if (gueststr) + virBufferAsprintf(buf, "<guest %s/>\n", gueststr); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</driver>\n"); + } VIR_FREE(str); + VIR_FREE(hoststr); + VIR_FREE(gueststr); + + if (rc < 0) + return -1;

ha ha - interesting way around the merry go round :-) ACK - whether or not you use the macro John

} } if (def->backend.tap || def->backend.vhost) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 640a4c5..1210cc3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -895,6 +895,21 @@ struct _virDomainNetDef { virTristateSwitch ioeventfd; virTristateSwitch event_idx; unsigned int queues; /* Multiqueue virtio-net */ + struct { + virTristateSwitch csum; + virTristateSwitch gso; + virTristateSwitch tso4; + virTristateSwitch tso6; + virTristateSwitch ecn; + virTristateSwitch ufo; + } host; + struct { + virTristateSwitch csum; + virTristateSwitch tso4; + virTristateSwitch tso6; + virTristateSwitch ecn; + virTristateSwitch ufo; + } guest; } virtio; } driver; struct { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml new file mode 100644 index 0000000..e368c43 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest7'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='00:22:44:66:88:aa'/> + <model type='virtio'/> + <driver> + <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + </driver> + </interface> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 675403a..31e358c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -261,6 +261,7 @@ mymain(void) DO_TEST("net-user"); DO_TEST("net-virtio"); DO_TEST("net-virtio-device"); + DO_TEST("net-virtio-disable-offloads"); DO_TEST("net-eth"); DO_TEST("net-eth-ifname"); DO_TEST("net-virtio-network-portgroup");

On 09/24/2014 09:44 AM, Ján Tomko wrote:

On 09/24/2014 01:24 AM, John Ferlan wrote:

...

On 09/18/2014 10:15 AM, Ján Tomko wrote:

...

Add options for tuning segment offloading: <driver> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> which control the respective host_ and guest_ properties of the virtio-net device. --- docs/formatdomain.html.in | 24 ++- docs/schemas/domaincommon.rng | 44 ++++- src/conf/domain_conf.c | 218 ++++++++++++++++++++- src/conf/domain_conf.h | 15 ++ .../qemuxml2argv-net-virtio-disable-offloads.xml | 35 ++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c03ebb..5dd70a4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3868,7 +3868,11 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> + </driver> + </b> </interface> </devices> ...</pre> @@ -3972,6 +3976,24 @@ qemu-kvm -net nic,model=? /dev/null processor, resulting in much higher throughput. <span class="since">Since 1.0.6 (QEMU and KVM only)</span> </dd> + <dt><code>host offloading options</code></dt>

Should this be <code>host</host> offloading options? or host TCP options (in some way to indicate that these are TCP level options). Probably also want your disclaimer use of these should be for only those that know what they are doing...

Well ufo is an UDP option.

Yeah - good point. I guess I associated the rest/most with TCP... Of course you could use "TCP/UDP" instead of just TCP... I'm OK without the TCP reference though - it was a "extra" suggestion.

...

...

+ <dd> + The <code>csum</code>, <code>gso</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>

Should read: ecn, and ufo

Should you "spell out" what each translates to?

csum (checksums), gso (generic segmentation offload), tso (tcp segmentation offload v4 and v6), ecn (congestion notification), and ufo (UDP fragment offloads)

I thought not spelling them out was the equivalent of the "only use this if you know what you're doing" disclaimer.

Yes - I do agree that by not spelling them out it may cause someone to "pause" before adding them. However, of course, we're engineers so we have this "desire" to try anyway and/or know what these new knobs/things do. I guess it's one of those things that I don't like - that is seeing an acronym on a website or in documentation that's not spelled out.

...

...

+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn host offloads off.

s/turn host offloads off/turn off host TCP options/

Saying "offloads off" aloud just doesn't seem right.

...

+ By default, the supported offloads are enabled by QEMU.

s/the supported offloads/the TCP options/

...

+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> + <dt>guest offloading options</dt>

Similar in here... Does the host setting override the guest value or can the host say "tso4=off" while the guest has "tso4=on"? Curious mainly.

It can say that, but I doubt it'll work.

...

...

+ <dd> + The <code>csum</code>, <code>tso4</code>, + <code>tso6</code>, <code>ecn</code>, <code>ufo</code>

same here with the "and" - although I suppose you could just reference the <host> bits "above"...

Another way to say it is guests can use the same options except gso

...

+ attributes with possible values <code>on</code> + and <code>off</code> can be used to turn guest offloads off.

s/turn guest offloads off/turn off guest TCP options/

How about 'turn off guest offload options'?

That's fine - it was the "offloads off" that didn't read well.

...

...

+ By default, the supported offloads are enabled by QEMU.

s/the supported offloads/the TCP options/

...

And of course the usage disclaimer!

...

+ <span class="since">Since 1.2.9 (QEMU only)</span> + </dd> </dl>

<h5><a name="elementsBackendOptions">Setting network backend-specific options</a></h5>

...

...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ccec1c..cdaafa6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6897,6 +6897,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *str = NULL; char *filter = NULL; char *internal = NULL; char *devaddr = NULL; @@ -7385,6 +7386,115 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->driver.virtio.queues = q; }

How about something like this? Not that you have anything wrong, but it avoids the chance that some "change" in the future requires 11 similar changes...

I was worried it wouldn't be clear enough and since we use similar repetition all over domain_conf, it would be better handled separately.

That's fine - like I said - I didn't see anything wrong with the code - maybe something for the todo list to reduce the "need" for all the repetition and silly errors some of us make... John

On 09/18/2014 10:15 AM, Ján Tomko wrote:

Format the segment offloading options specified by <driver> <host .../> <guest .../> </driver> on virtio-net command line. --- src/qemu/qemu_command.c | 40 ++++++++++++++++++++++ .../qemuxml2argv-net-virtio-disable-offloads.args | 10 ++++++ tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 52 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ce320de..ee49db5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4438,6 +4438,46 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",event_idx=%s", virTristateSwitchTypeToString(net->driver.virtio.event_idx)); } + if (net->driver.virtio.guest.csum) {

s/guest/host

+ virBufferAsprintf(&buf, ",csum=%s", + virTristateSwitchTypeToString(net->driver.virtio.guest.csum));

s/guest/host

+ } + if (net->driver.virtio.host.gso) { + virBufferAsprintf(&buf, ",gso=%s", + virTristateSwitchTypeToString(net->driver.virtio.host.gso)); + } + if (net->driver.virtio.host.tso4) { + virBufferAsprintf(&buf, ",host_tso4=%s", + virTristateSwitchTypeToString(net->driver.virtio.host.tso4)); + } + if (net->driver.virtio.host.tso6) { + virBufferAsprintf(&buf, ",host_tso6=%s", + virTristateSwitchTypeToString(net->driver.virtio.host.tso6)); + } + if (net->driver.virtio.host.ecn) { + virBufferAsprintf(&buf, ",host_ecn=%s", + virTristateSwitchTypeToString(net->driver.virtio.host.ecn)); + } + if (net->driver.virtio.host.ufo) { + virBufferAsprintf(&buf, ",host_ufo=%s", + virTristateSwitchTypeToString(net->driver.virtio.host.ufo)); + }

if (net->driver.virtio.guest.csum) { virBufferAsprintf(&buf, ",guest_csum=%s", virTristateSwitchTypeToString(net->driver.virtio.guest.csum)); }

+ if (net->driver.virtio.guest.tso4) { + virBufferAsprintf(&buf, ",guest_tso4=%s", + virTristateSwitchTypeToString(net->driver.virtio.guest.tso4)); + } + if (net->driver.virtio.guest.tso6) { + virBufferAsprintf(&buf, ",guest_tso6=%s", + virTristateSwitchTypeToString(net->driver.virtio.guest.tso6)); + } + if (net->driver.virtio.guest.ecn) { + virBufferAsprintf(&buf, ",guest_ecn=%s", + virTristateSwitchTypeToString(net->driver.virtio.guest.ecn)); + } + if (net->driver.virtio.guest.ufo) { + virBufferAsprintf(&buf, ",guest_ufo=%s", + virTristateSwitchTypeToString(net->driver.virtio.guest.ufo)); + } } if (usingVirtio && vhostfdSize > 1) { /* As advised at http://www.linux-kvm.org/page/Multiqueue diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args new file mode 100644 index 0000000..3328988 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-hda /dev/HostVG/QEMUGuest7 \ +-device virtio-net-pci,csum=off,gso=off,\ +host_tso4=off,host_tso6=off,host_ecn=off,host_ufo=off,\ +guest_tso4=off,guest_tso6=off,guest_ecn=off,guest_ufo=off,\

This may need a "guest_csum=off," ACK with that John

+vlan=0,id=net0,mac=00:22:44:66:88:aa,bus=pci.0,addr=0x3 \ +-net user,vlan=0,name=hostnet0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 275e699..d3da1e9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -965,6 +965,8 @@ mymain(void) DO_TEST("net-virtio", NONE); DO_TEST("net-virtio-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_TX_ALG); + DO_TEST("net-virtio-disable-offloads", + QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("net-virtio-netdev", QEMU_CAPS_DEVICE, QEMU_CAPS_NETDEV, QEMU_CAPS_NODEFCONFIG); DO_TEST("net-virtio-s390",