* [PATCH 0/2] virtio fix false offload claims
@ 2017-07-07 19:52 Stephen Hemminger
2017-07-07 19:52 ` [PATCH 1/2] virtio: don't falsely claim to do IP checksum Stephen Hemminger
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Stephen Hemminger @ 2017-07-07 19:52 UTC (permalink / raw)
To: yliu, maxime.coquelin; +Cc: dev, Stephen Hemminger
While doing code for Hyper-V, noticed that the virtio driver was
confused about receive versus transmit offloads. The virtio
checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO
and LRO are not the same.
This may break some program that was assuming it was getting offloads
that it wasn't.
Stephen Hemminger (2):
virtio: don't falsely claim to do IP checksum
virtio: don't claim to support LRO
drivers/net/virtio/virtio_ethdev.c | 30 +++++-------------------------
1 file changed, 5 insertions(+), 25 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] virtio: don't falsely claim to do IP checksum 2017-07-07 19:52 [PATCH 0/2] virtio fix false offload claims Stephen Hemminger @ 2017-07-07 19:52 ` Stephen Hemminger 2017-07-07 19:52 ` [PATCH 2/2] virtio: don't claim to support LRO Stephen Hemminger 2017-07-08 3:12 ` [PATCH 0/2] virtio fix false offload claims Yuanhan Liu 2 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2017-07-07 19:52 UTC (permalink / raw) To: yliu, maxime.coquelin; +Cc: dev, Stephen Hemminger The virtio driver is confused about the meaning of the ip_checksum flag. In DPDK, ip_checksum means the hardware is capable of checking the Layer 3 IP checksum. But KVM/QEMU does not do that. The flag VIRTIO_NET_F_GUEST_CSUM controls whether the receive side does Layer 4 (TCP/UDP) checksum offload. Fix by erroring out any requests to do IP checksum. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- drivers/net/virtio/virtio_ethdev.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index 5c826f47740c..ca607904fa37 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -1664,8 +1664,13 @@ virtio_dev_configure(struct rte_eth_dev *dev) PMD_INIT_LOG(DEBUG, "configure"); req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES; - if (rxmode->hw_ip_checksum) - req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM); + + /* Virtio does L4 checksum but not L3! */ + if (rxmode->hw_ip_checksum) { + PMD_DRV_LOG(NOTICE, + "virtio does not support IP checksum"); + return -ENOTSUP; + } if (rxmode->enable_lro) req_features |= (1ULL << VIRTIO_NET_F_GUEST_TSO4) | @@ -1678,13 +1683,6 @@ virtio_dev_configure(struct rte_eth_dev *dev) return ret; } - if (rxmode->hw_ip_checksum && - !vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM)) { - PMD_DRV_LOG(NOTICE, - "rx ip checksum not available on this host"); - return -ENOTSUP; - } - if (rxmode->enable_lro && (!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) || !vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) { -- 2.11.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] virtio: don't claim to support LRO 2017-07-07 19:52 [PATCH 0/2] virtio fix false offload claims Stephen Hemminger 2017-07-07 19:52 ` [PATCH 1/2] virtio: don't falsely claim to do IP checksum Stephen Hemminger @ 2017-07-07 19:52 ` Stephen Hemminger 2017-07-08 3:12 ` [PATCH 0/2] virtio fix false offload claims Yuanhan Liu 2 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2017-07-07 19:52 UTC (permalink / raw) To: yliu, maxime.coquelin; +Cc: dev, Stephen Hemminger The current virtio supports Transmit Segmentation Offload, but does not really support Large Receive Offload. The driver was confusing the two offloads. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- drivers/net/virtio/virtio_ethdev.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index ca607904fa37..00a3122780ba 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -1659,11 +1659,8 @@ virtio_dev_configure(struct rte_eth_dev *dev) { const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode; struct virtio_hw *hw = dev->data->dev_private; - uint64_t req_features; - int ret; PMD_INIT_LOG(DEBUG, "configure"); - req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES; /* Virtio does L4 checksum but not L3! */ if (rxmode->hw_ip_checksum) { @@ -1671,23 +1668,10 @@ virtio_dev_configure(struct rte_eth_dev *dev) "virtio does not support IP checksum"); return -ENOTSUP; } - if (rxmode->enable_lro) - req_features |= - (1ULL << VIRTIO_NET_F_GUEST_TSO4) | - (1ULL << VIRTIO_NET_F_GUEST_TSO6); - - /* if request features changed, reinit the device */ - if (req_features != hw->req_guest_features) { - ret = virtio_init_device(dev, req_features); - if (ret < 0) - return ret; - } - if (rxmode->enable_lro && - (!vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) || - !vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4))) { + if (rxmode->enable_lro) { PMD_DRV_LOG(NOTICE, - "lro not available on this host"); + "virtio does not support Large Receive Offload"); return -ENOTSUP; } @@ -1913,8 +1897,6 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) } tso_mask = (1ULL << VIRTIO_NET_F_GUEST_TSO4) | (1ULL << VIRTIO_NET_F_GUEST_TSO6); - if ((host_features & tso_mask) == tso_mask) - dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_TCP_LRO; dev_info->tx_offload_capa = 0; if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) { -- 2.11.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] virtio fix false offload claims 2017-07-07 19:52 [PATCH 0/2] virtio fix false offload claims Stephen Hemminger 2017-07-07 19:52 ` [PATCH 1/2] virtio: don't falsely claim to do IP checksum Stephen Hemminger 2017-07-07 19:52 ` [PATCH 2/2] virtio: don't claim to support LRO Stephen Hemminger @ 2017-07-08 3:12 ` Yuanhan Liu 2017-08-23 9:30 ` Olivier MATZ 2 siblings, 1 reply; 8+ messages in thread From: Yuanhan Liu @ 2017-07-08 3:12 UTC (permalink / raw) To: Stephen Hemminger; +Cc: maxime.coquelin, dev, stable On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > While doing code for Hyper-V, noticed that the virtio driver was > confused about receive versus transmit offloads. The virtio > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > and LRO are not the same. > > This may break some program that was assuming it was getting offloads > that it wasn't. Applied to dpdk-next-virtio. And I think they should be backported to stable releases, thus, Cc: stable@dpdk.org Thanks. --yliu > > Stephen Hemminger (2): > virtio: don't falsely claim to do IP checksum > virtio: don't claim to support LRO > > drivers/net/virtio/virtio_ethdev.c | 30 +++++------------------------- > 1 file changed, 5 insertions(+), 25 deletions(-) > > -- > 2.11.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] virtio fix false offload claims 2017-07-08 3:12 ` [PATCH 0/2] virtio fix false offload claims Yuanhan Liu @ 2017-08-23 9:30 ` Olivier MATZ 2017-08-23 15:31 ` Stephen Hemminger 0 siblings, 1 reply; 8+ messages in thread From: Olivier MATZ @ 2017-08-23 9:30 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Stephen Hemminger, maxime.coquelin, dev, stable Hello, On Sat, Jul 08, 2017 at 11:12:22AM +0800, Yuanhan Liu wrote: > On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > > While doing code for Hyper-V, noticed that the virtio driver was > > confused about receive versus transmit offloads. The virtio > > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > > and LRO are not the same. > > > > This may break some program that was assuming it was getting offloads > > that it wasn't. > > Applied to dpdk-next-virtio. > > And I think they should be backported to stable releases, thus, > > Cc: stable@dpdk.org > > Thanks. > > --yliu > > > > Stephen Hemminger (2): > > virtio: don't falsely claim to do IP checksum > > virtio: don't claim to support LRO > > > > drivers/net/virtio/virtio_ethdev.c | 30 +++++------------------------- > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > -- > > 2.11.0 I think these 2 commits break the virtio offload, which can be tested as described in this test plan: http://dpdk.org/ml/archives/dev/2016-October/048092.html First, about checksum: the description of rxmode->hw_ip_checksum is: hw_ip_checksum : 1, /**< IP/UDP/TCP checksum offload enable. */ So, while I agree the name is not well chosen, it is valid to set it for virtio to enable L4 checksum. Then about LRO: setting rxmode->enable_lro is a way to tell the host that the guest is ok to receive tso packets. From the guest point of view, it is like enabling lro on a physical driver. Again, it is valid and useful to do this. Before removing these features, it would have been nice to have a quick look at the commits that introduced them. Olivier ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] virtio fix false offload claims 2017-08-23 9:30 ` Olivier MATZ @ 2017-08-23 15:31 ` Stephen Hemminger 2017-08-23 16:14 ` Olivier MATZ 0 siblings, 1 reply; 8+ messages in thread From: Stephen Hemminger @ 2017-08-23 15:31 UTC (permalink / raw) To: Olivier MATZ; +Cc: Yuanhan Liu, maxime.coquelin, dev, stable On Wed, 23 Aug 2017 11:30:26 +0200 Olivier MATZ <olivier.matz@6wind.com> wrote: > Hello, > > On Sat, Jul 08, 2017 at 11:12:22AM +0800, Yuanhan Liu wrote: > > On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > > > While doing code for Hyper-V, noticed that the virtio driver was > > > confused about receive versus transmit offloads. The virtio > > > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > > > and LRO are not the same. > > > > > > This may break some program that was assuming it was getting offloads > > > that it wasn't. > > > > Applied to dpdk-next-virtio. > > > > And I think they should be backported to stable releases, thus, > > > > Cc: stable@dpdk.org > > > > Thanks. > > > > --yliu > > > > > > Stephen Hemminger (2): > > > virtio: don't falsely claim to do IP checksum > > > virtio: don't claim to support LRO > > > > > > drivers/net/virtio/virtio_ethdev.c | 30 +++++------------------------- > > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > > > -- > > > 2.11.0 > > I think these 2 commits break the virtio offload, which can be tested as > described in this test plan: > http://dpdk.org/ml/archives/dev/2016-October/048092.html > > First, about checksum: the description of rxmode->hw_ip_checksum is: > > hw_ip_checksum : 1, /**< IP/UDP/TCP checksum offload enable. */ > > So, while I agree the name is not well chosen, it is valid to set it > for virtio to enable L4 checksum. > > Then about LRO: setting rxmode->enable_lro is a way to tell the host that the > guest is ok to receive tso packets. From the guest point of view, it is like > enabling lro on a physical driver. Again, it is valid and useful to do this. > > Before removing these features, it would have been nice to have a quick look at > the commits that introduced them. I am ok with keeping LRO as long as the documentation changed. And virtio driver did some enforcement. For checksums, the hw_ip_checksum flag either needs to be more fine grain (IP, UDP, TCP) which would be best, or virtio would have to check IP checksum in software. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] virtio fix false offload claims 2017-08-23 15:31 ` Stephen Hemminger @ 2017-08-23 16:14 ` Olivier MATZ 2017-08-23 16:41 ` Stephen Hemminger 0 siblings, 1 reply; 8+ messages in thread From: Olivier MATZ @ 2017-08-23 16:14 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Yuanhan Liu, maxime.coquelin, dev, stable On Wed, Aug 23, 2017 at 08:31:35AM -0700, Stephen Hemminger wrote: > On Wed, 23 Aug 2017 11:30:26 +0200 > Olivier MATZ <olivier.matz@6wind.com> wrote: > > > Hello, > > > > On Sat, Jul 08, 2017 at 11:12:22AM +0800, Yuanhan Liu wrote: > > > On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > > > > While doing code for Hyper-V, noticed that the virtio driver was > > > > confused about receive versus transmit offloads. The virtio > > > > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > > > > and LRO are not the same. > > > > > > > > This may break some program that was assuming it was getting offloads > > > > that it wasn't. > > > > > > Applied to dpdk-next-virtio. > > > > > > And I think they should be backported to stable releases, thus, > > > > > > Cc: stable@dpdk.org > > > > > > Thanks. > > > > > > --yliu > > > > > > > > Stephen Hemminger (2): > > > > virtio: don't falsely claim to do IP checksum > > > > virtio: don't claim to support LRO > > > > > > > > drivers/net/virtio/virtio_ethdev.c | 30 +++++------------------------- > > > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > > > > > -- > > > > 2.11.0 > > > > I think these 2 commits break the virtio offload, which can be tested as > > described in this test plan: > > http://dpdk.org/ml/archives/dev/2016-October/048092.html > > > > First, about checksum: the description of rxmode->hw_ip_checksum is: > > > > hw_ip_checksum : 1, /**< IP/UDP/TCP checksum offload enable. */ > > > > So, while I agree the name is not well chosen, it is valid to set it > > for virtio to enable L4 checksum. > > > > Then about LRO: setting rxmode->enable_lro is a way to tell the host that the > > guest is ok to receive tso packets. From the guest point of view, it is like > > enabling lro on a physical driver. Again, it is valid and useful to do this. > > > > Before removing these features, it would have been nice to have a quick look at > > the commits that introduced them. > > I am ok with keeping LRO as long as the documentation changed. And virtio > driver did some enforcement. > > For checksums, the hw_ip_checksum flag either needs to be more fine grain (IP, UDP, TCP) > which would be best, or virtio would have to check IP checksum in software. > For checksum, yes, the rxconf should be more fine-grained and renamed. But apart from the name which is confusing, it was not wrong. Setting hw_ip_checksum=1 means: "allow the driver to return packets with checksums flags != unknown". These flags are good,bad,unknown,none for both l3 and l4. So virtio driver always return unknown for l3, and none|unknown|good|bad for l4, depending on what the host passed. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] virtio fix false offload claims 2017-08-23 16:14 ` Olivier MATZ @ 2017-08-23 16:41 ` Stephen Hemminger 0 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2017-08-23 16:41 UTC (permalink / raw) To: Olivier MATZ; +Cc: Yuanhan Liu, maxime.coquelin, dev, stable On Wed, 23 Aug 2017 18:14:44 +0200 Olivier MATZ <olivier.matz@6wind.com> wrote: > On Wed, Aug 23, 2017 at 08:31:35AM -0700, Stephen Hemminger wrote: > > On Wed, 23 Aug 2017 11:30:26 +0200 > > Olivier MATZ <olivier.matz@6wind.com> wrote: > > > > > Hello, > > > > > > On Sat, Jul 08, 2017 at 11:12:22AM +0800, Yuanhan Liu wrote: > > > > On Fri, Jul 07, 2017 at 12:52:48PM -0700, Stephen Hemminger wrote: > > > > > While doing code for Hyper-V, noticed that the virtio driver was > > > > > confused about receive versus transmit offloads. The virtio > > > > > checksum offload is L4 (TCP/UDP) only, not IPv4. Also, TSO > > > > > and LRO are not the same. > > > > > > > > > > This may break some program that was assuming it was getting offloads > > > > > that it wasn't. > > > > > > > > Applied to dpdk-next-virtio. > > > > > > > > And I think they should be backported to stable releases, thus, > > > > > > > > Cc: stable@dpdk.org > > > > > > > > Thanks. > > > > > > > > --yliu > > > > > > > > > > Stephen Hemminger (2): > > > > > virtio: don't falsely claim to do IP checksum > > > > > virtio: don't claim to support LRO > > > > > > > > > > drivers/net/virtio/virtio_ethdev.c | 30 +++++------------------------- > > > > > 1 file changed, 5 insertions(+), 25 deletions(-) > > > > > > > > > > -- > > > > > 2.11.0 > > > > > > I think these 2 commits break the virtio offload, which can be tested as > > > described in this test plan: > > > http://dpdk.org/ml/archives/dev/2016-October/048092.html > > > > > > First, about checksum: the description of rxmode->hw_ip_checksum is: > > > > > > hw_ip_checksum : 1, /**< IP/UDP/TCP checksum offload enable. */ > > > > > > So, while I agree the name is not well chosen, it is valid to set it > > > for virtio to enable L4 checksum. > > > > > > Then about LRO: setting rxmode->enable_lro is a way to tell the host that the > > > guest is ok to receive tso packets. From the guest point of view, it is like > > > enabling lro on a physical driver. Again, it is valid and useful to do this. > > > > > > Before removing these features, it would have been nice to have a quick look at > > > the commits that introduced them. > > > > I am ok with keeping LRO as long as the documentation changed. And virtio > > driver did some enforcement. > > > > For checksums, the hw_ip_checksum flag either needs to be more fine grain (IP, UDP, TCP) > > which would be best, or virtio would have to check IP checksum in software. > > > > For checksum, yes, the rxconf should be more fine-grained and renamed. > > But apart from the name which is confusing, it was not wrong. > Setting hw_ip_checksum=1 means: "allow the driver to return packets with > checksums flags != unknown". These flags are good,bad,unknown,none for > both l3 and l4. So virtio driver always return unknown for l3, and > none|unknown|good|bad for l4, depending on what the host passed. You are right, I forgot that there now is way to indicate unknown for checksums. Before that it was limited good/bad. Let's revert both these patches, and update the documentation? ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-23 16:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-07 19:52 [PATCH 0/2] virtio fix false offload claims Stephen Hemminger 2017-07-07 19:52 ` [PATCH 1/2] virtio: don't falsely claim to do IP checksum Stephen Hemminger 2017-07-07 19:52 ` [PATCH 2/2] virtio: don't claim to support LRO Stephen Hemminger 2017-07-08 3:12 ` [PATCH 0/2] virtio fix false offload claims Yuanhan Liu 2017-08-23 9:30 ` Olivier MATZ 2017-08-23 15:31 ` Stephen Hemminger 2017-08-23 16:14 ` Olivier MATZ 2017-08-23 16:41 ` Stephen Hemminger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.