Thanks for the bugfix! On Wed, Nov 23, 2016 at 12:19 PM Barret Rhoden <[email protected]> wrote:
> On 2016-11-23 at 10:00 brho <[email protected]> wrote: > > Commit 24f29c1568ab ("Fix virtio net handling of the header.") breaks my > ability to ssh into a Linux guest. > > > > From the guest, I see: > > > > ``` > > tc@box:~$ Sending discover... > > Sending discover... > > Sending discover... > > Sending discover... > > Sending discover... > > Sending discover... > > ``` > > > > where the 'sending discover' is new. > > > > This is on my vmm branch. If I revert 24f29c15, things work as usual. > > > > One thing I noticed about that commit is that previously we'd always > strip the VIRTIO_HEADER_SIZE from every iov, not just the first one. > > > > Which behavior is correct? Is the header a set of bytes at the > beginning of the area mapped by the IOVs? Or is it something prepended to > every IOV? If not the latter (old behavior), then why does the guest's > networking fail in such a way that stripping 12 bytes off every IOV makes > it work? > > > > here's a packet from snoopy/tcpdump that went out the same time that > 'Sending Discover' went out. > > (this is from running snoopy -N0. on mlx4, i think you'll need -p too, > due to some promisc errors.) > > 028288 ms > ether(s=1c:b7:2c:ee:52:69 d=00:00:00:00:00:00 pr=ffff ln=354) > dump(ff ff ff ff 1c b7 2c ee 52 69 08 00 45 00 01 48 > .......,.Ri..E.. > 00 00 00 00 40 11 79 a6 00 00 00 00 ff ff ff ff H....@ > .y........ > 00 44 00 43 01 34 d5 7f 01 01 06 00 ce 30 d1 7f > ..D.C.4.......0. > 06 41 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ..A............. > 00 00 00 00 1c b7 2c ee 52 69 00 00 00 00 00 00 > .......,.Ri..... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > ................ > 00 00 00 00 63 82 53 63 35 01 01 3d 07 01 1c b7 > .....c.Sc5..=... > 2c ee 52 69 32 04 64 6b 0a 64 39 02 02 40 37 07 > .,.Ri2.dk.d9..@7 > 01 03 06 0c 0f 1c 2a 3c 0c 75 64 68 63 70 20 31 > .......*<.udhcp > 2e 32 34 2e 32 0c 03 62 6f 78 ff 00 00 00 00 00 > 1.24.2..box..... > 00 00 00 00) > > > tc@box:~$ ifconfig > eth0 Link encap:Ethernet HWaddr 1C:B7:2C:EE:52:69 > > the source addr is right, though akaros wrote that in there. the dest > is all 0s, which is wrong. > > the protocol is 0xff 0xff, which is also wrong. the dump() is the > payload of the packet. the first 4 bytes are 0xff. add those to the > protocol, and it looks like that should be the destination field (6 > 0xff bytes for the broadcast). > > later on in the dump, we have the MAC from linux (5th byte in the dump, > 0x1c 0xb7, etc). > > so it looks like we had an extra 12 bytes of probably zeros at the > front of the packet. then probably akaros (in devether) wrote the > source addr, and pumped it down to the NIC. > > > i added this patch to virtio-net: > > diff --git a/user/vmm/virtio_net.c b/user/vmm/virtio_net.c > index 6201acb8358d..600d2416c1de 100644 > --- a/user/vmm/virtio_net.c > +++ b/user/vmm/virtio_net.c > @@ -32,6 +32,7 @@ > #include <vmm/virtio.h> > #include <vmm/virtio_mmio.h> > #include <vmm/virtio_net.h> > +#include <parlib/ros_debug.h> > > #define VIRTIO_HEADER_SIZE 12 > > @@ -202,6 +203,9 @@ void net_transmitq_fn(void *_vq) > > /* Find the beginning of the ethernet frame. */ > for (int i = 0; i < olen; i++) { > + fprintf(stderr, "IOV %d, base %p, len > %d\n-----------\n", i, > + iov[i].iov_base, iov[i].iov_len); > + hexdump(stderr, iov[i].iov_base, iov[i].iov_len); > /* This conditional also catches cases when > iov_len is 0 */ > if (iov[i].iov_len <= bytestostrip) { > /* The virtio net header is bigger or > equal to the current iov. > @@ -211,6 +215,12 @@ void net_transmitq_fn(void *_vq) > } > /* The virtio net header is smaller than the full > iov. > * We stop advancing here and write. */ > + > + fprintf(stderr, " bytestostrip now %d\n", > bytestostrip); > + fprintf(stderr, " writing to FD %d base %p, > len %d\n", > + fd, iov[i].iov_base + bytestostrip, > + iov[i].iov_len - bytestostrip); > + > ret = write(fd, iov[i].iov_base + bytestostrip, > iov[i].iov_len - bytestostrip); > assert(ret == iov[i].iov_len - bytestostrip); > > > > and get: (they are all just one IOV) > > IOV 0, base 0x3bce7006, len 354 > ----------- > 3bce7006: 00 00 00 00 00 00 00 00 00 00 10 00 ff ff ff ff ................ > 3bce7016: ff ff 1c b7 2c ee 52 69 08 00 45 00 01 48 00 00 ....,.Ri..E..H.. > 3bce7026: 00 00 40 11 79 a6 00 00 00 00 ff ff ff ff 00 44 [email protected] > 3bce7036: 00 43 01 34 07 74 01 01 06 00 94 eb 38 59 00 06 .C.4.t......8Y.. > 3bce7046: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 3bce7056: 00 00 1c b7 2c ee 52 69 00 00 00 00 00 00 00 00 ....,.Ri........ > 3bce7066: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > ... > 3bce7126: 00 00 63 82 53 63 35 01 03 3d 07 01 1c b7 2c ee ..c.Sc5..=....,. > 3bce7136: 52 69 32 04 64 6b 0a 64 36 04 64 6b 0a 43 39 02 Ri2.dk.d6.dk.C9. > 3bce7146: 02 40 37 07 01 03 06 0c 0f 1c 2a 3c 0c 75 64 68 .@7.......*<.udh > 3bce7156: 63 70 20 31 2e 32 34 2e 32 0c 03 62 6f 78 ff 00 cp 1.24.2..box.. > 3bce7166: 00 00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 ................ > bytestostrip now 0 > writing to FD 20 base 0x3bce7006, len 354 > > > so pretty weird. bytes to strip is always 0, though i'd expect it to > be 12, since iov_len > bytestostrip. > > then, we write to the kernel the original iovbase. the kernel probably > writes in the src in the first part of the field, but i'd expect that > 0x10 to show up in the destination. it turns out that the packet > pasted above has 0x10, but many others do not, and they just have 0x00 > for the first 12 bytes. > > anyway, i printed a little more, and bytestostrip is 0 at the top of the > loop! > > the problem is that we were setting it == 12 at the top of > net_transmitq_fn(), but we should be setting it == 12 for every loop > around the infinite loop near L189 (for the > virtio_next_avaiil_vq_desc()). > > the following diff fixed it: > > diff --git a/user/vmm/virtio_net.c b/user/vmm/virtio_net.c > index 6201acb8358d..18687e04e87c 100644 > --- a/user/vmm/virtio_net.c > +++ b/user/vmm/virtio_net.c > @@ -172,7 +172,7 @@ void net_transmitq_fn(void *_vq) > void *stripped; > int ret; > int fd = open(data_path, O_RDWR); > - int bytestostrip = VIRTIO_HEADER_SIZE; > + int bytestostrip; > > if (fd == -1) > VIRTIO_DEV_ERRX(vq->vqdev, "Could not open data file for > ether1."); > @@ -201,6 +201,7 @@ void net_transmitq_fn(void *_vq) > */ > > /* Find the beginning of the ethernet frame. */ > + bytestostrip = VIRTIO_HEADER_SIZE; > for (int i = 0; i < olen; i++) { > /* This conditional also catches cases when > iov_len is 0 */ > if (iov[i].iov_len <= bytestostrip) { > > > Barret > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
