Re: [PATCH 2/4] tun: TUNSETFEATURES to set gso features.
Rusty Russell wrote: ethtool is useful for setting (some) device fields, but it's root-only. Finer feature control is available through a tun-specific ioctl. (Includes Mark McLoughlin [EMAIL PROTECTED]'s fix to hold rtnl sem). Signed-off-by: Rusty Russell [EMAIL PROTECTED] Agree. And it's often way more convenient to muck with the TUN settings directly on the fd instead of using external tool. Acked-by: Max Krasnyansky [EMAIL PROTECTED] ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/4] tun: Interface to query tun/tap features.
Rusty Russell wrote: The problem with introducing checksum offload and gso to tun is they need to set dev-features to enable GSO and/or checksumming, which is supposed to be done before register_netdevice(), ie. as part of TUNSETIFF. Unfortunately, TUNSETIFF has always just ignored flags it doesn't understand, so there's no good way of detecting whether the kernel supports new IFF_ flags. This patch implements a TUNGETFEATURES ioctl which returns all the valid IFF flags. It could be extended later to include other features. snip Looks good. Dave, do you want me to put all outstanding TUN patches into a git tree so that you can pull them in one shot ? Otherwise if you're ok with applying them one by one please apply this one. Acked-by: Max Krasnyansky [EMAIL PROTECTED] ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/4] tun: Allow GSO using virtio_net_hdr
Rusty Russell wrote:
Add a IFF_VNET_HDR flag. This uses the same ABI as virtio_net (ie. prepending
struct virtio_net_hdr to packets) to indicate GSO and checksum information.
Signed-off-by: Rusty Russell [EMAIL PROTECTED]
---
drivers/net/tun.c | 90
-
include/linux/if_tun.h |2 +
2 files changed, 91 insertions(+), 1 deletion(-)
diff -r d94590c1550a drivers/net/tun.c
--- a/drivers/net/tun.c Thu Jun 26 00:21:11 2008 +1000
+++ b/drivers/net/tun.c Thu Jun 26 00:21:59 2008 +1000
@@ -63,6 +63,7 @@
#include linux/if_tun.h
#include linux/crc32.h
#include linux/nsproxy.h
+#include linux/virtio_net.h
#include net/net_namespace.h
#include net/netns/generic.h
@@ -283,12 +284,24 @@ static __inline__ ssize_t tun_get_user(s
struct tun_pi pi = { 0, __constant_htons(ETH_P_IP) };
struct sk_buff *skb;
size_t len = count, align = 0;
+ struct virtio_net_hdr gso = { 0 };
if (!(tun-flags TUN_NO_PI)) {
if ((len -= sizeof(pi)) count)
return -EINVAL;
if(memcpy_fromiovec((void *)pi, iv, sizeof(pi)))
+ return -EFAULT;
+ }
+
+ if (tun-flags TUN_VNET_HDR) {
+ if ((len -= sizeof(gso)) count)
+ return -EINVAL;
+
+ if (gso.hdr_len len)
+ return -EINVAL;
+
+ if (memcpy_fromiovec((void *)gso, iv, sizeof(gso)))
return -EFAULT;
}
Unless I'm missing something the 'if (gso.hdr_len len)' must be after
memcpy_fromiovec().
@@ -322,8 +335,45 @@ static __inline__ ssize_t tun_get_user(s
break;
};
- if (tun-flags TUN_NOCHECKSUM)
+ if (gso.flags VIRTIO_NET_HDR_F_NEEDS_CSUM) {
+ if (!skb_partial_csum_set(skb, gso.csum_start,
+ gso.csum_offset)) {
+ tun-dev-stats.rx_dropped++;
+ kfree_skb(skb);
+ return -EINVAL;
+ }
+ } else if (tun-flags TUN_NOCHECKSUM)
skb-ip_summed = CHECKSUM_UNNECESSARY;
+
+ if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
+ pr_debug(GSO!\n);
+ switch (gso.gso_type ~VIRTIO_NET_HDR_GSO_ECN) {
+ case VIRTIO_NET_HDR_GSO_TCPV4:
+ skb_shinfo(skb)-gso_type = SKB_GSO_TCPV4;
+ break;
+ case VIRTIO_NET_HDR_GSO_TCPV6:
+ skb_shinfo(skb)-gso_type = SKB_GSO_TCPV6;
+ break;
+ default:
+ tun-dev-stats.rx_dropped++;
+ kfree_skb(skb);
+ return -EINVAL;
+ }
We should use stats.rx_frame_errors instead of stats.rx_dropped to indicated
that we dropped it because something was wrong with the framing (headers,
etc). Applies to both of the cases above.
+
+ if (gso.gso_type VIRTIO_NET_HDR_GSO_ECN)
+ skb_shinfo(skb)-gso_type |= SKB_GSO_TCP_ECN;
+
+ skb_shinfo(skb)-gso_size = gso.gso_size;
+ if (skb_shinfo(skb)-gso_size == 0) {
+ tun-dev-stats.rx_dropped++;
+ kfree_skb(skb);
+ return -EINVAL;
+ }
Same here.
Everything else looks good.
Max
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/4] lguest: Use GSO/IFF_VNET_HDR extensions on tun/tap
Rusty Russell wrote:
On Thursday 26 June 2008 05:07:18 Anthony Liguori wrote:
Rusty Russell wrote:
@@ -1563,6 +1561,16 @@ static void setup_tun_net(char *arg)
/* Tell Guest what MAC address to use. */
add_feature(dev, VIRTIO_NET_F_MAC);
add_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY);
+ /* Expect Guest to handle everything except UFO */
+ add_feature(dev, VIRTIO_NET_F_CSUM);
You're setting this feature twice.
Hmm, not in the version here?
+ add_feature(dev, VIRTIO_NET_F_GUEST_CSUM);
You set this feature, but I never see the virtio-net driver acknowledge
the feature. Curiously, my implementation with KVM is struggling
because UDP packet checksums are not correct so the DHCP client is
ignoring them. If I disable CSUM offload, things it works fine (using
the virtio-net header). The problem is only host=guest, guest=host is
fine.
OK, found this: wrong args to skb_partial_csum_set. It was found by Mark
McLoughlin before, I just lost the fix when I extracted this into a separate
patch. I chose to move the call to skb_partial_csum_set(), rather than use
his fix (which assumed a tap not tun device).
Here's two fixes on top of previous patch:
diff -u b/drivers/net/tun.c b/drivers/net/tun.c
--- b/drivers/net/tun.c Thu Jun 26 00:21:59 2008 +1000
+++ b/drivers/net/tun.c Thu Jun 26 14:35:03 2008 +1000
@@ -298,11 +298,11 @@
if ((len -= sizeof(gso)) count)
return -EINVAL;
- if (gso.hdr_len len)
- return -EINVAL;
-
if (memcpy_fromiovec((void *)gso, iv, sizeof(gso)))
return -EFAULT;
+
+ if (gso.hdr_len len)
+ return -EINVAL;
}
Yep, looks better now.
if ((tun-flags TUN_TYPE_MASK) == TUN_TAP_DEV) {
@@ -324,6 +324,16 @@
return -EFAULT;
}
+ if (gso.flags VIRTIO_NET_HDR_F_NEEDS_CSUM) {
+ if (!skb_partial_csum_set(skb, gso.csum_start,
+ gso.csum_offset)) {
+ tun-dev-stats.rx_dropped++;
+ kfree_skb(skb);
+ return -EINVAL;
+ }
+ } else if (tun-flags TUN_NOCHECKSUM)
+ skb-ip_summed = CHECKSUM_UNNECESSARY;
+
switch (tun-flags TUN_TYPE_MASK) {
case TUN_TUN_DEV:
skb_reset_mac_header(skb);
@@ -335,16 +345,6 @@
break;
};
- if (gso.flags VIRTIO_NET_HDR_F_NEEDS_CSUM) {
- if (!skb_partial_csum_set(skb, gso.csum_start,
- gso.csum_offset)) {
- tun-dev-stats.rx_dropped++;
- kfree_skb(skb);
- return -EINVAL;
- }
- } else if (tun-flags TUN_NOCHECKSUM)
- skb-ip_summed = CHECKSUM_UNNECESSARY;
-
if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
pr_debug(GSO!\n);
switch (gso.gso_type ~VIRTIO_NET_HDR_GSO_ECN) {
Do you want to resent the GSO patch with all the latest fixes ? ie other
things (stat counters) I pointed out in the prev email.
I'll ack it.
Thanx
Max
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/4] tun: Interface to query tun/tap features.
From: Max Krasnyansky [EMAIL PROTECTED] Date: Tue, 01 Jul 2008 21:59:02 -0700 Dave, do you want me to put all outstanding TUN patches into a git tree so that you can pull them in one shot ? Otherwise if you're ok with applying them one by one please apply this one. Acked-by: Max Krasnyansky [EMAIL PROTECTED] I'll apply Rusty's patches after I give them a review too. Thanks Max. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
