virtio-net: iovec handling cleanup
cover virtio-net: iovec handling cleanup
virtio net is using lots of manual mangling of iovecs;
I don't see any obvious crashes but it does look scary since guest can manipulate iovecs. This patchset changes it to use iov_* functions which are safer.
On a recent linux kernel, new code will actually do less iovec processing because it is able to utilize support for mergeable buffer header size added in tun/tap in linux 2.6.35 - though I was unable to measure any speed difference.
This also removes the restriction on buffer layout, aligning qemu with recent versions of virtio spec which imply that device should be prepared to handle e.g. virtio net header inline with the packet data in the same buffer.
Tsted on x86 kvm only for now, would appreciate cross-endianness etc testing/reports. This is also why I split this to so many small patches to make bisect easier in case of problems.
Michael S. Tsirkin (14):
virtio-net: track host/guest header length
iov: add const annotation
iov: add iov_cpy
virtio-net: avoid sg copy
virtio-net: use safe iov operations for rx
virtio-net: refactor receive_hdr
virtio-net: first s/g is always at start of buf
virtio-net: switch tx to safe iov functions
virtio-net: simplify rx code
virtio: don't mark unaccessed memory as dirty
virtio-net: fix used len for tx
virtio-net: minor code simplification
virtio-net: test peer header support at init time
virtio-net: enable mrg buf header in tap on linux
hw/vhost_net.c | 13 ----- hw/virtio-net.c | 177 +++++++++++++++++++++++++++++++------------------------- hw/virtio.c | 2 +- iov.c | 25 +++++++- iov.h | 11 +++- 5 files changed, 132 insertions(+), 96 deletions(-)
Signature · 1 line
MST
PATCH 1/14 virtio-net: track host/guest header length
Tracking these in device state instead of
re-calculating on each packet. No functional
changes.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
hw/virtio-net.c +17 -12
@@ -41,6 +41,8 @@ typedef struct VirtIONetint32_t tx_burst;int tx_waiting;uint32_t has_vnet_hdr; +size_t host_hdr_len; +size_t guest_hdr_len;uint8_t has_ufo;struct {VirtQueueElement elem;
@@ -231,6 +233,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)if (peer_has_vnet_hdr (n)) {tap_using_vnet_hdr (n->nic->nc.peer,1 ); + n->host_hdr_len =sizeof (struct virtio_net_hdr ); }else { features &= ~(0x1 <<VIRTIO_NET_F_CSUM ); features &= ~(0x1 <<VIRTIO_NET_F_HOST_TSO4 );
@@ -278,6 +281,8 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)VirtIONet *n =to_virtio_net (vdev); n->mergeable_rx_bufs = !!(features & (1 <<VIRTIO_NET_F_MRG_RXBUF )); + n->guest_hdr_len = n->mergeable_rx_bufs ? +sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr );if (n->has_vnet_hdr) { tap_set_offload(n->nic->nc.peer,
@@ -593,18 +598,13 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque;struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -size_t guest_hdr_len, offset, i, host_hdr_len; +size_t offset, i;if (!virtio_net_can_receive (&n->nic->nc))return -1 ;/* hdr_len refers to the header we supply to the guest */ - guest_hdr_len = n->mergeable_rx_bufs ? -sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr ); - - - host_hdr_len = n->has_vnet_hdr ?sizeof (struct virtio_net_hdr ) :0 ; -if (!virtio_net_has_buffers (n, size + guest_hdr_len - host_hdr_len)) +if (!virtio_net_has_buffers (n, size + n->guest_hdr_len - n->host_hdr_len))return 0 ;if (!receive_filter (n, buf, size))
@@ -626,7 +626,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t"i %zd mergeable %d offset %zd, size %zd, " "guest hdr len %zd, host hdr len %zd guest features 0x%x" , i, n->mergeable_rx_bufs, offset, size, - guest_hdr_len, host_hdr_len, n->vdev.guest_features); + n->guest_hdr_len, n->host_hdr_len, n->vdev.guest_features);exit (1 ); }
@@ -635,7 +635,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -if (!n->mergeable_rx_bufs && elem.in_sg[0 ].iov_len != guest_hdr_len) { +if (!n->mergeable_rx_bufs && elem.in_sg[0 ].iov_len != n->guest_hdr_len) {error_report ("virtio-net header not in first element" );exit (1 ); }
@@ -647,8 +647,9 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset, guest_hdr_len); - total += guest_hdr_len; + buf + offset, size - offset, + n->guest_hdr_len); + total += n->guest_hdr_len; }/* copy in packet. ugh */
@@ -665,7 +666,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t"i %zd mergeable %d offset %zd, size %zd, " "guest hdr len %zd, host hdr len %zd" , i, n->mergeable_rx_bufs, - offset, size, guest_hdr_len, host_hdr_len); + offset, size, n->guest_hdr_len, n->host_hdr_len);#endif return size; }
@@ -900,6 +901,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)qemu_get_buffer (f, n->mac,ETH_ALEN ); n->tx_waiting =qemu_get_be32 (f); n->mergeable_rx_bufs =qemu_get_be32 (f); + n->guest_hdr_len = n->mergeable_rx_bufs ? +sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr );if (version_id >=3 ) n->status =qemu_get_be16 (f);
@@ -938,6 +941,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)if (n->has_vnet_hdr) {tap_using_vnet_hdr (n->nic->nc.peer,1 ); + n->host_hdr_len =sizeof (struct virtio_net_hdr ); tap_set_offload(n->nic->nc.peer, (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1, (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
@@ -1037,6 +1041,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, n->tx_waiting =0 ; n->tx_burst = net->txburst; n->mergeable_rx_bufs =0 ; + n->guest_hdr_len =sizeof (struct virtio_net_hdr ); n->promisc =1 ;/* for compatibility */ n->mac_table.macs =g_malloc0 (MAC_TABLE_ENTRIES *ETH_ALEN );
Signature · 1 line
MST
PATCH 2/14 iov: add const annotation
In reply to Michael S. Tsirkiniov_from_buf does not change iov, make it const.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- iov.c | 2 +- iov.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
iov.c +1 -1
@@ -26,7 +26,7 @@# include <sys/socket.h> #endif -size_t iov_from_buf (struct iovec *iov,unsigned int iov_cnt, +size_t iov_from_buf (const struct iovec *iov,unsigned int iov_cnt,size_t offset,const void *buf,size_t bytes) {size_t done;
iov.h +1 -1
@@ -36,7 +36,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt);* such "large" value is -1 (sinice size_t is unsigned), * so specifying `-1' as `bytes' means 'up to the end of iovec'. */ -size_t iov_from_buf (struct iovec *iov,unsigned int iov_cnt, +size_t iov_from_buf (const struct iovec *iov,unsigned int iov_cnt,size_t offset,const void *buf,size_t bytes);size_t iov_to_buf (const struct iovec *iov,const unsigned int iov_cnt,size_t offset,void *buf,size_t bytes);
Signature · 1 line
MST
PATCH 3/14 iov: add iov_cpy
In reply to Michael S. TsirkinAdd API to copy part of iovec safely.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- iov.c | 23 +++++++++++++++++++++++ iov.h | 9 +++++++++ 2 files changed, 32 insertions(+)
iov.c +23 -0
@@ -228,3 +228,26 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,fprintf (fp,"\n" ); } } + +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes) +{ +size_t len; +unsigned int i, j; +for (i =0 , j =0 ; i < iov_cnt && j < dst_iov_cnt && bytes; i++) { +if (offset >= iov[i].iov_len) { + offset -= iov[i].iov_len; +continue ; + } + len =MIN (bytes, iov[i].iov_len - offset); + + dst_iov[j].iov_base = iov[i].iov_base + offset; + dst_iov[j].iov_len = len; + j++; + bytes -= len; + offset =0 ; + } +assert (offset ==0 ); +return j; +}
iov.h +9 -0
@@ -86,3 +86,12 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt,*/ void iov_hexdump (const struct iovec *iov,const unsigned int iov_cnt,FILE *fp,const char *prefix,size_t limit); + +/* +* Partial copy of vector from iov to dst_iov (data is not copied). +* dst_iov overlaps iov at a specified offset. +* size of dst_iov is at most bytes. Actual size is returned. +*/ +size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 03/14] iov: add iov_cpy
Quoted 13 lines · Michael S. Tsirkin
Add API to copy part of iovec safely.
Signed-off-by Michael S. Tsirkin <[email protected]>--- iov.c | 23 +++++++++++++++++++++++ iov.h | 9 +++++++++ 2 files changed, 32 insertions(+)iov.c +0
-0
iov.c void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt, · line 234
Quoted 7 lines
iov.c +4
-0@@ -228,3 +228,26 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,fprintf (fp,"\n" ); } } + +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes)
I think the readability f the cde wuld be imprved if yu added the missing 'o' to iov_cpy.
Otherwise:
Reviewed-by Anthony Liguori <[email protected]>
Regards,
Anthony Liguori
Quoted 38 lines
iov.c +19
-0+{ +size_t len; +unsigned int i, j; +for (i =0 , j =0 ; i < iov_cnt && j < dst_iov_cnt && bytes; i++) { +if (offset >= iov[i].iov_len) { + offset -= iov[i].iov_len; +continue ; + } + len =MIN (bytes, iov[i].iov_len - offset); + + dst_iov[j].iov_base = iov[i].iov_base + offset; + dst_iov[j].iov_len = len; + j++; + bytes -= len; + offset =0 ; + } +assert (offset ==0 ); +return j; +}iov.h +9
-0@@ -86,3 +86,12 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt,*/ void iov_hexdump (const struct iovec *iov,const unsigned int iov_cnt,FILE *fp,const char *prefix,size_t limit); + +/* +* Partial copy of vector from iov to dst_iov (data is not copied). +* dst_iov overlaps iov at a specified offset. +* size of dst_iov is at most bytes. Actual size is returned. +*/ +size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 03/14] iov: add iov_cpy
Quoted 10 lines · Anthony Liguori
Quoted 8 lines · Michael S. Tsirkin
Add API to copy part of iovec safely.
Signed-off-by Michael S. Tsirkin <[email protected]>--- iov.c | 23 +++++++++++++++++++++++ iov.h | 9 +++++++++ 2 files changed, 32 insertions(+)
Quoted 15 lines
iov.c void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt, · line 234
Quoted 12 lines
iov.c +4
-0@@ -228,3 +228,26 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,fprintf (fp,"\n" ); } } + +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes)I think the readability f the cde wuld be imprved if yu added the missing 'o' to
iov_cpy.
I was thinking about memcpy but maybe that's not a good model to follow. Will fix.
Quoted 46 lines
Otherwise:
Reviewed-by Anthony Liguori <[email protected]>Regards,
Anthony Liguori
Quoted 38 lines
iov.c +19
-0+{ +size_t len; +unsigned int i, j; +for (i =0 , j =0 ; i < iov_cnt && j < dst_iov_cnt && bytes; i++) { +if (offset >= iov[i].iov_len) { + offset -= iov[i].iov_len; +continue ; + } + len =MIN (bytes, iov[i].iov_len - offset); + + dst_iov[j].iov_base = iov[i].iov_base + offset; + dst_iov[j].iov_len = len; + j++; + bytes -= len; + offset =0 ; + } +assert (offset ==0 ); +return j; +}iov.h +9
-0@@ -86,3 +86,12 @@ ssize_t iov_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt,*/ void iov_hexdump (const struct iovec *iov,const unsigned int iov_cnt,FILE *fp,const char *prefix,size_t limit); + +/* +* Partial copy of vector from iov to dst_iov (data is not copied). +* dst_iov overlaps iov at a specified offset. +* size of dst_iov is at most bytes. Actual size is returned. +*/ +size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);Signature · 1 line
MST
PATCH 4/14 virtio-net: avoid sg copy
In reply to Michael S. TsirkinAvoid tweaking iovec during receive. This removes
the need to copy the vector.
Note: we already have an evil cast in work_around_broken_dhclient and unfortunately this adds another one.
const on buf is ignored by this function anyway so arguably this is not making things much worse.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 45 +++++++++++++++++++++------------------------ iov.h | 8 ++++---- 2 files changed, 25 insertions(+), 28 deletions(-)
hw/virtio-net.c +21 -24
@@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)* we should provide a mechanism to disable it to avoid polluting the host * cache. */ -static void work_around_broken_dhclient (struct virtio_net_hdr *hdr, +static void work_around_broken_dhclient (const struct virtio_net_hdr *hdr,const uint8_t *buf,size_t size) { if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
@@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps *//* FIXME this cast is evil */ net_checksum_calculate ((uint8_t *)buf, size); - hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; + ((struct virtio_net_hdr *)hdr)->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; } } -static int receive_header (VirtIONet *n,struct iovec *iov,int iovcnt, -const void *buf,size_t size,size_t hdr_len) +static int receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, +const void *buf,size_t size) { -struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)iov[0 ].iov_base;int offset =0 ; - hdr->flags =0 ; - hdr->gso_type =VIRTIO_NET_HDR_GSO_NONE ; -if (n->has_vnet_hdr) { -memcpy (hdr, buf,sizeof (*hdr)); - offset =sizeof (*hdr); -work_around_broken_dhclient (hdr, buf + offset, size - offset); +work_around_broken_dhclient (buf, buf + offset, size - offset); + offset =sizeof (struct virtio_net_hdr ); +iov_from_buf (iov, iov_cnt,0 , buf, offset); + }else { +struct virtio_net_hdr hdr = { + .flags =0 , + .gso_type =VIRTIO_NET_HDR_GSO_NONE + }; +iov_from_buf (iov, iov_cnt,0 , &hdr,sizeof hdr); } -/* We only ever receive a struct virtio_net_hdr from the tapfd, -* but we may be passing along a larger header to the guest. -*/ - iov[0 ].iov_base += hdr_len; - iov[0 ].iov_len -= hdr_len; -return offset; }
@@ -598,7 +594,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque;struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -size_t offset, i; +const struct iovec *sg = elem.in_sg; +size_t offset, i, guest_offset;if (!virtio_net_can_receive (&n->nic->nc))return -1 ;
@@ -615,7 +612,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_twhile (offset < size) {VirtQueueElement elem;int len, total; -struct iovec sg[VIRTQUEUE_MAX_SIZE ]; +const struct iovec *sg = elem.in_sg; total =0 ;
@@ -640,20 +637,20 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -memcpy (&sg, &elem.in_sg[0 ],sizeof (sg[0 ]) * elem.in_num); -if (i ==0 ) {if (n->mergeable_rx_bufs) mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset, - n->guest_hdr_len); + buf + offset, size - offset); total += n->guest_hdr_len; + guest_offset = n->guest_hdr_len; + }else { + guest_offset =0 ; }/* copy in packet. ugh */ - len =iov_from_buf (sg, elem.in_num,0 , + len =iov_from_buf (sg, elem.in_num, guest_offset, buf + offset, size - offset); total += len; offset += len;
iov.h +4 -4
@@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,/* * Partial copy of vector from iov to dst_iov (data is not copied). * dst_iov overlaps iov at a specified offset. -* size of dst_iov is at most bytes. Actual size is returned. +* size of dst_iov is at most bytes. dst vector count is returned. */ -size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, -const struct iovec *iov,unsigned int iov_cnt, -size_t offset,size_t bytes); +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 04/14] virtio-net: avoid sg copy
Quoted 43 lines · Michael S. Tsirkin
Avoid tweaking iovec during receive. This removes
the need to copy the vector.
Note: we already have an evil cast inwork_around_broken_dhclientand unfortunately this adds another one.
const on buf is ignored by this function anyway so arguably this is not making things much worse.Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/virtio-net.c | 45 +++++++++++++++++++++------------------------ iov.h | 8 ++++---- 2 files changed, 25 insertions(+), 28 deletions(-)hw/virtio-net.c +4
-5@@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)* we should provide a mechanism to disable it to avoid polluting the host * cache. */ -static void work_around_broken_dhclient (struct virtio_net_hdr *hdr, +static void work_around_broken_dhclient (const struct virtio_net_hdr *hdr,const uint8_t *buf,size_t size) { if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */@@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps *//* FIXME this cast is evil */ net_checksum_calculate ((uint8_t *)buf, size); - hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; + ((struct virtio_net_hdr *)hdr)->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; } } -static int receive_header (VirtIONet *n,struct iovec *iov,int iovcnt, -const void *buf,size_t size,size_t hdr_len) +static int receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, +const void *buf,size_t size) { -struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)iov[0 ].iov_base;int offset =0 ;
hw/virtio-net.c static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, · line 528
Quoted 10 lines
hw/virtio-net.c +1
-6- hdr->flags =0 ; - hdr->gso_type =VIRTIO_NET_HDR_GSO_NONE ; -if (n->has_vnet_hdr) { -memcpy (hdr, buf,sizeof (*hdr)); - offset =sizeof (*hdr); -work_around_broken_dhclient (hdr, buf + offset, size - offset); +work_around_broken_dhclient (buf, buf + offset, size - offset);
I think it would be less ugly to cast here.
Quoted 71 lines
hw/virtio-net.c +8
-6+ offset =sizeof (struct virtio_net_hdr ); +iov_from_buf (iov, iov_cnt,0 , buf, offset); + }else { +struct virtio_net_hdr hdr = { + .flags =0 , + .gso_type =VIRTIO_NET_HDR_GSO_NONE + }; +iov_from_buf (iov, iov_cnt,0 , &hdr,sizeof hdr); } -/* We only ever receive a struct virtio_net_hdr from the tapfd, -* but we may be passing along a larger header to the guest. -*/ - iov[0 ].iov_base += hdr_len; - iov[0 ].iov_len -= hdr_len; -return offset;}
hw/virtio-net.c +8
-7@@ -598,7 +594,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque;struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -size_t offset, i; +const struct iovec *sg = elem.in_sg; +size_t offset, i, guest_offset;if (!virtio_net_can_receive (&n->nic->nc))return -1 ;@@ -615,7 +612,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_twhile (offset < size) {VirtQueueElement elem;int len, total; -struct iovec sg[VIRTQUEUE_MAX_SIZE ]; +const struct iovec *sg = elem.in_sg; total =0 ;@@ -640,20 +637,20 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -memcpy (&sg, &elem.in_sg[0 ],sizeof (sg[0 ]) * elem.in_num); -if (i ==0 ) {if (n->mergeable_rx_bufs) mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset, - n->guest_hdr_len); + buf + offset, size - offset); total += n->guest_hdr_len; + guest_offset = n->guest_hdr_len; + }else { + guest_offset =0 ; }/* copy in packet. ugh */ - len =iov_from_buf (sg, elem.in_num,0 , + len =iov_from_buf (sg, elem.in_num, guest_offset, buf + offset, size - offset); total += len; offset += len;iov.h +0
-0@@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,/* * Partial copy of vector from iov to dst_iov (data is not copied).
iov.h void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt, · line 97
Quoted 10 lines
iov.h +4
-4* dst_iov overlaps iov at a specified offset. -* size of dst_iov is at most bytes. Actual size is returned. +* size of dst_iov is at most bytes. dst vector count is returned. */ -size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, -const struct iovec *iov,unsigned int iov_cnt, -size_t offset,size_t bytes); +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);
I think this belongs in an earlier patch.
Regards,
Anthony Liguori
Quoted 2 lines
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 04/14] virtio-net: avoid sg copy
Quoted 46 lines · Anthony Liguori
Quoted 44 lines · Michael S. Tsirkin
Avoid tweaking iovec during receive. This removes
the need to copy the vector.
Note: we already have an evil cast inwork_around_broken_dhclientand unfortunately this adds another one.
const on buf is ignored by this function anyway so arguably this is not making things much worse.Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/virtio-net.c | 45 +++++++++++++++++++++------------------------ iov.h | 8 ++++---- 2 files changed, 25 insertions(+), 28 deletions(-)hw/virtio-net.c +4
-5@@ -503,7 +503,7 @@ static int virtio_net_has_buffers(VirtIONet *n, int bufsize)* we should provide a mechanism to disable it to avoid polluting the host * cache. */ -static void work_around_broken_dhclient (struct virtio_net_hdr *hdr, +static void work_around_broken_dhclient (const struct virtio_net_hdr *hdr,const uint8_t *buf,size_t size) { if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */@@ -513,31 +513,27 @@ static void work_around_broken_dhclient(struct virtio_net_hdr *hdr, (buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps *//* FIXME this cast is evil */ net_checksum_calculate ((uint8_t *)buf, size); - hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; + ((struct virtio_net_hdr *)hdr)->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM ; } } -static int receive_header (VirtIONet *n,struct iovec *iov,int iovcnt, -const void *buf,size_t size,size_t hdr_len) +static int receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, +const void *buf,size_t size) { -struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)iov[0 ].iov_base;int offset =0 ;
Quoted 11 lines
hw/virtio-net.c static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
Quoted 9 lines
hw/virtio-net.c +1
-6- hdr->flags =0 ; - hdr->gso_type =VIRTIO_NET_HDR_GSO_NONE ; -if (n->has_vnet_hdr) { -memcpy (hdr, buf,sizeof (*hdr)); - offset =sizeof (*hdr); -work_around_broken_dhclient (hdr, buf + offset, size - offset); +work_around_broken_dhclient (buf, buf + offset, size - offset);I think it would be less ugly to cast here.
I was thinking about this but could not decide either way. Will do.
Quoted 75 lines
Quoted 75 lines
hw/virtio-net.c +8
-6+ offset =sizeof (struct virtio_net_hdr ); +iov_from_buf (iov, iov_cnt,0 , buf, offset); + }else { +struct virtio_net_hdr hdr = { + .flags =0 , + .gso_type =VIRTIO_NET_HDR_GSO_NONE + }; +iov_from_buf (iov, iov_cnt,0 , &hdr,sizeof hdr); } -/* We only ever receive a struct virtio_net_hdr from the tapfd, -* but we may be passing along a larger header to the guest. -*/ - iov[0 ].iov_base += hdr_len; - iov[0 ].iov_len -= hdr_len; -return offset;}
hw/virtio-net.c +8
-7@@ -598,7 +594,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque;struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -size_t offset, i; +const struct iovec *sg = elem.in_sg; +size_t offset, i, guest_offset;if (!virtio_net_can_receive (&n->nic->nc))return -1 ;@@ -615,7 +612,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_twhile (offset < size) {VirtQueueElement elem;int len, total; -struct iovec sg[VIRTQUEUE_MAX_SIZE ]; +const struct iovec *sg = elem.in_sg; total =0 ;@@ -640,20 +637,20 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -memcpy (&sg, &elem.in_sg[0 ],sizeof (sg[0 ]) * elem.in_num); -if (i ==0 ) {if (n->mergeable_rx_bufs) mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset, - n->guest_hdr_len); + buf + offset, size - offset); total += n->guest_hdr_len; + guest_offset = n->guest_hdr_len; + }else { + guest_offset =0 ; }/* copy in packet. ugh */ - len =iov_from_buf (sg, elem.in_num,0 , + len =iov_from_buf (sg, elem.in_num, guest_offset, buf + offset, size - offset); total += len; offset += len;iov.h +1
-1@@ -90,8 +90,8 @@ void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt,/* * Partial copy of vector from iov to dst_iov (data is not copied). * dst_iov overlaps iov at a specified offset. -* size of dst_iov is at most bytes. Actual size is returned. +* size of dst_iov is at most bytes. dst vector count is returned. */
Quoted 8 lines
iov.h void iov_hexdump(const struct iovec *iov, const unsigned int iov_cnt, · line 97
Quoted 6 lines
iov.h +3
-3-size_t iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, -const struct iovec *iov,unsigned int iov_cnt, -size_t offset,size_t bytes); +unsigned iov_cpy (struct iovec *dst_iov,unsigned int dst_iov_cnt, +const struct iovec *iov,unsigned int iov_cnt, +size_t offset,size_t bytes);I think this belongs in an earlier patch.
Good catch thanks.
Quoted 6 lines
Regards,
Anthony Liguori
Quoted 2 lines
Signature · 1 line
MST
PATCH 5/14 virtio-net: use safe iov operations for rx
In reply to Michael S. TsirkinAvoid magling iov manually: use safe iov operations for processing packets incoming to guest. This also removes the requirement for virtio header to fit the first s/g entry exactly.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
hw/virtio-net.c +14 -11
@@ -593,8 +593,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)static ssize_t virtio_net_receive (NetClientState *nc,const uint8_t *buf,size_t size) {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque; -struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -const struct iovec *sg = elem.in_sg; +struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE ]; +struct virtio_net_hdr_mrg_rxbuf mhdr; +unsigned mhdr_cnt =0 ;size_t offset, i, guest_offset;if (!virtio_net_can_receive (&n->nic->nc))
@@ -632,14 +633,13 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -if (!n->mergeable_rx_bufs && elem.in_sg[0 ].iov_len != n->guest_hdr_len) { -error_report ("virtio-net header not in first element" ); -exit (1 ); - } -if (i ==0 ) { -if (n->mergeable_rx_bufs) - mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; +if (n->mergeable_rx_bufs) { + mhdr_cnt =iov_cpy (mhdr_sg,ARRAY_SIZE (mhdr_sg), + sg, elem.in_num, + offsetof(typeof (mhdr ), num_buffers), +sizeof (mhdr.num_buffers)); + } offset +=receive_header (n, sg, elem.in_num, buf + offset, size - offset);
@@ -672,8 +672,11 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_tvirtqueue_fill (n->rx_vq, &elem, total, i++); } -if (mhdr) { -stw_p (&mhdr->num_buffers, i); +if (mhdr_cnt) { +stw_p (&mhdr.num_buffers, i); +iov_from_buf (mhdr_sg, mhdr_cnt, +0 , + &mhdr.num_buffers,sizeof mhdr.num_buffers); }virtqueue_flush (n->rx_vq, i);
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 05/14] virtio-net: use safe iov operations for rx
Quoted 6 lines · Michael S. Tsirkin
Avoid magling iov manually: use safe iov operations for processing packets incoming to guest. This also removes the requirement for virtio header to fit the first s/g entry exactly.
Signed-off-by Michael S. Tsirkin <[email protected]>
Reviewed-by Anthony Liguori <[email protected]>
Regards,
Anthony Liguori
Quoted 57 lines
--- hw/virtio-net.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)hw/virtio-net.c +14
-11@@ -593,8 +593,9 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)static ssize_t virtio_net_receive (NetClientState *nc,const uint8_t *buf,size_t size) {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque; -struct virtio_net_hdr_mrg_rxbuf *mhdr =NULL ; -const struct iovec *sg = elem.in_sg; +struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE ]; +struct virtio_net_hdr_mrg_rxbuf mhdr; +unsigned mhdr_cnt =0 ;size_t offset, i, guest_offset;if (!virtio_net_can_receive (&n->nic->nc))@@ -632,14 +633,13 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_texit (1 ); } -if (!n->mergeable_rx_bufs && elem.in_sg[0 ].iov_len != n->guest_hdr_len) { -error_report ("virtio-net header not in first element" ); -exit (1 ); - } -if (i ==0 ) { -if (n->mergeable_rx_bufs) - mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0 ].iov_base; +if (n->mergeable_rx_bufs) { + mhdr_cnt =iov_cpy (mhdr_sg,ARRAY_SIZE (mhdr_sg), + sg, elem.in_num, + offsetof(typeof (mhdr ), num_buffers), +sizeof (mhdr.num_buffers)); + } offset +=receive_header (n, sg, elem.in_num, buf + offset, size - offset);@@ -672,8 +672,11 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_tvirtqueue_fill (n->rx_vq, &elem, total, i++); } -if (mhdr) { -stw_p (&mhdr->num_buffers, i); +if (mhdr_cnt) { +stw_p (&mhdr.num_buffers, i); +iov_from_buf (mhdr_sg, mhdr_cnt, +0 , + &mhdr.num_buffers,sizeof mhdr.num_buffers); }virtqueue_flush (n->rx_vq, i);Signature · 1 line
MST
PATCH 6/14 virtio-net: refactor receive_hdr
In reply to Michael S. TsirkinNow that we know host hdr length, we don't need to duplicate the logic in receive_hdr: caller can figure out the offset itself.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
hw/virtio-net.c +7 -11
@@ -517,15 +517,13 @@ static void work_around_broken_dhclient(const struct virtio_net_hdr *hdr, } } -static int receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, -const void *buf,size_t size) +static void receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, +const void *buf,size_t size) { -int offset =0 ; -if (n->has_vnet_hdr) { -work_around_broken_dhclient (buf, buf + offset, size - offset); - offset =sizeof (struct virtio_net_hdr ); -iov_from_buf (iov, iov_cnt,0 , buf, offset); +work_around_broken_dhclient (buf, buf + n->host_hdr_len, + size - n->host_hdr_len); +iov_from_buf (iov, iov_cnt,0 , buf,sizeof (struct virtio_net_hdr )); }else {struct virtio_net_hdr hdr = { .flags = 0,
@@ -533,8 +531,6 @@ static int receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, };iov_from_buf (iov, iov_cnt,0 , &hdr,sizeof hdr); } - -return offset; }static int receive_filter (VirtIONet *n,const uint8_t *buf,int size)
@@ -641,8 +637,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t sizeof(mhdr.num_buffers)); } - offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset); +receive_header (n, sg, elem.in_num, buf + offset, size - offset); + offset += n->host_hdr_len; total += n->guest_hdr_len; guest_offset = n->guest_hdr_len; }else {
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 06/14] virtio-net: refactor receive_hdr
Quoted 5 lines · Michael S. Tsirkin
Now that we know host hdr length, we don't need to duplicate the logic in
receive_hdr: caller can figure out the offset itself.Signed-off-by Michael S. Tsirkin <[email protected]>
Reviewed-by Anthony Liguori <[email protected]>
Regards,
Anthony Liguori
Quoted 51 lines
--- hw/virtio-net.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)hw/virtio-net.c +7
-11@@ -517,15 +517,13 @@ static void work_around_broken_dhclient(const struct virtio_net_hdr *hdr, } } -static int receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, -const void *buf,size_t size) +static void receive_header (VirtIONet *n,const struct iovec *iov,int iov_cnt, +const void *buf,size_t size) { -int offset =0 ; -if (n->has_vnet_hdr) { -work_around_broken_dhclient (buf, buf + offset, size - offset); - offset =sizeof (struct virtio_net_hdr ); -iov_from_buf (iov, iov_cnt,0 , buf, offset); +work_around_broken_dhclient (buf, buf + n->host_hdr_len, + size - n->host_hdr_len); +iov_from_buf (iov, iov_cnt,0 , buf,sizeof (struct virtio_net_hdr )); }else {struct virtio_net_hdr hdr = { .flags = 0,@@ -533,8 +531,6 @@ static int receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt, };iov_from_buf (iov, iov_cnt,0 , &hdr,sizeof hdr); } - -return offset; }static int receive_filter (VirtIONet *n,const uint8_t *buf,int size)@@ -641,8 +637,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t sizeof(mhdr.num_buffers)); } - offset +=receive_header (n, sg, elem.in_num, - buf + offset, size - offset); +receive_header (n, sg, elem.in_num, buf + offset, size - offset); + offset += n->host_hdr_len; total += n->guest_hdr_len; guest_offset = n->guest_hdr_len; }else {Signature · 1 line
MST
PATCH 7/14 virtio-net: first s/g is always at start of buf
In reply to Michael S. TsirkinWe know offset is 0, assert that.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
hw/virtio-net.c +3 -2
@@ -630,6 +630,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t }if (i ==0 ) { +assert (offset ==0 );if (n->mergeable_rx_bufs) { mhdr_cnt = iov_cpy(mhdr_sg, ARRAY_SIZE(mhdr_sg), sg, elem.in_num,
@@ -637,8 +638,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t sizeof(mhdr.num_buffers)); } -receive_header (n, sg, elem.in_num, buf + offset, size - offset); - offset += n->host_hdr_len; +receive_header (n, sg, elem.in_num, buf, size); + offset = n->host_hdr_len; total += n->guest_hdr_len; guest_offset = n->guest_hdr_len; }else {
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 07/14] virtio-net: first s/g is always at start of buf
Quoted 3 lines · Michael S. Tsirkin
We know offset is 0, assert that.
Signed-off-by Michael S. Tsirkin <[email protected]>
Reviewed-by Anthony Liguori <[email protected]>
Regards,
Anthony Liguori
Quoted 29 lines
--- hw/virtio-net.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)hw/virtio-net.c +3
-2@@ -630,6 +630,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t }if (i ==0 ) { +assert (offset ==0 );if (n->mergeable_rx_bufs) { mhdr_cnt = iov_cpy(mhdr_sg, ARRAY_SIZE(mhdr_sg), sg, elem.in_num,@@ -637,8 +638,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t sizeof(mhdr.num_buffers)); } -receive_header (n, sg, elem.in_num, buf + offset, size - offset); - offset += n->host_hdr_len; +receive_header (n, sg, elem.in_num, buf, size); + offset = n->host_hdr_len; total += n->guest_hdr_len; guest_offset = n->guest_hdr_len; }else {Signature · 1 line
MST
PATCH 8/14 virtio-net: switch tx to safe iov functions
In reply to Michael S. TsirkinAvoid mangling iovec manually: use safe iov_*
functions.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
hw/virtio-net.c +19 -12
@@ -714,10 +714,10 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq) }while (virtqueue_pop (vq, &elem)) { -ssize_t ret, len =0 ; +ssize_t ret, len;unsigned int out_num = elem.out_num;struct iovec *out_sg = &elem.out_sg[0 ]; -unsigned hdr_len; +struct iovec sg[VIRTQUEUE_MAX_SIZE ];/* hdr_len refers to the header received from the guest */ hdr_len = n->mergeable_rx_bufs ?
@@ -729,18 +729,25 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)exit (1 ); } -/* ignore the header if GSO is not supported */ -if (!n->has_vnet_hdr) { - out_num--; - out_sg++; - len += hdr_len; - }else if (n->mergeable_rx_bufs) { -/* tapfd expects a struct virtio_net_hdr */ - hdr_len -=sizeof (struct virtio_net_hdr ); - out_sg->iov_len -= hdr_len; - len += hdr_len; +/* +* If host wants to see the guest header as is, we can +* pass it on unchanged. Otherwise, copy just the parts +* that host is interested in. +*/ +assert (n->host_hdr_len <= n->guest_hdr_len); +if (n->host_hdr_len != n->guest_hdr_len) { +unsigned sg_num =iov_cpy (sg,ARRAY_SIZE (sg), + out_sg, out_num, +0 , n->host_hdr_len); + sg_num +=iov_cpy (sg + sg_num,ARRAY_SIZE (sg) - sg_num, + out_sg, out_num, + n->guest_hdr_len,-1 ); + out_num = sg_num; + out_sg = sg; } + len = hdr_len; + ret =qemu_sendv_packet_async (&n->nic->nc, out_sg, out_num, virtio_net_tx_complete);if (ret ==0 ) {
Signature · 1 line
MST
PATCH 9/14 virtio-net: simplify rx code
In reply to Michael S. TsirkinRemove code duplication using guest header length that we track. Drop specific layout requirement for rx buffers: things work using generic iovec functions in any case.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
hw/virtio-net.c +2 -7
@@ -719,12 +719,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)struct iovec *out_sg = &elem.out_sg[0 ];struct iovec sg[VIRTQUEUE_MAX_SIZE ]; -/* hdr_len refers to the header received from the guest */ - hdr_len = n->mergeable_rx_bufs ? -sizeof (struct virtio_net_hdr_mrg_rxbuf ) : -sizeof (struct virtio_net_hdr ); - -if (out_num <1 || out_sg->iov_len != hdr_len) { +if (out_num <1 ) {error_report ("virtio-net header not in first element" );exit (1 ); }
@@ -746,7 +741,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq) out_sg = sg; } - len = hdr_len; + len = n->guest_hdr_len; ret =qemu_sendv_packet_async (&n->nic->nc, out_sg, out_num, virtio_net_tx_complete);
Signature · 1 line
MST
PATCH 10/14 virtio: don't mark unaccessed memory as dirty
In reply to Michael S. Tsirkinoffset of accessed buffer is calculated using iov_length, so it can exceed accessed len. If that happens
math in len - offset wraps around, and size becomes wrong. As real value is 0, so this is harmless but unnecessary.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/virtio.c +1 -1
@@ -241,7 +241,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem, elem->in_sg[i].iov_len, 1, size); - offset += elem->in_sg[i].iov_len; + offset += size; }for (i =0 ; i < elem->out_num; i++)
Signature · 1 line
MST
PATCH 11/14 virtio-net: fix used len for tx
In reply to Michael S. TsirkinThere is no out sg for TX, so used buf length for tx should always be 0.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
hw/virtio-net.c +2 -2
@@ -688,7 +688,7 @@ static void virtio_net_tx_complete(NetClientState *nc, ssize_t len) {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque; -virtqueue_push (n->tx_vq, &n->async_tx.elem, n->async_tx.len); +virtqueue_push (n->tx_vq, &n->async_tx.elem,0 );virtio_notify (&n->vdev, n->tx_vq); n->async_tx.elem.out_num = n->async_tx.len =0 ;
@@ -754,7 +754,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq) len += ret; -virtqueue_push (vq, &elem, len); +virtqueue_push (vq, &elem,0 );virtio_notify (&n->vdev, vq);if (++num_packets >= n->tx_burst) {
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 11/14] virtio-net: fix used len for tx
Quoted 2 lines · Michael S. Tsirkin
There is no out sg for TX, so used buf length for tx should always be 0.
According to the spec, the len is "Total length of the descriptor chain which was used (written to)". So I wonder if we need to pass the len here, it looks useful for guest how many bytes were sent by the driver (consider qemu may truncate the packet).
Quoted 27 lines
Signed-off-by Michael S. Tsirkin<[email protected]>--- hw/virtio-net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/virtio-net.c +2
-2@@ -688,7 +688,7 @@ static void virtio_net_tx_complete(NetClientState *nc, ssize_t len) {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque; -virtqueue_push (n->tx_vq,&n->async_tx.elem, n->async_tx.len); +virtqueue_push (n->tx_vq,&n->async_tx.elem,0 );virtio_notify (&n->vdev, n->tx_vq); n->async_tx.elem.out_num = n->async_tx.len =0 ;@@ -754,7 +754,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq) len += ret; -virtqueue_push (vq,&elem, len); +virtqueue_push (vq,&elem,0 );virtio_notify (&n->vdev, vq);if (++num_packets>= n->tx_burst) {
Re: [Qemu-devel] [PATCH 11/14] virtio-net: fix used len for tx
Quoted 6 lines · Jason Wang
Quoted 2 lines · Michael S. Tsirkin
There is no out sg for TX, so used buf length for tx should always be 0.
According to the spec, the len is "Total length of the descriptor chain which was used (written to)".
Right. And with TX no bytes are written at all.
Quoted 3 lines
So I wonder if we need to pass
the len here, it looks useful for guest how many bytes were sent by the driver (consider qemu may truncate the packet).
I thik qemu may not truncate packet.
Quoted 27 lines
Quoted 27 lines
Signed-off-by Michael S. Tsirkin<[email protected]>--- hw/virtio-net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/virtio-net.c +2
-2@@ -688,7 +688,7 @@ static void virtio_net_tx_complete(NetClientState *nc, ssize_t len) {VirtIONet *n =DO_UPCAST (NICState, nc, nc)->opaque; -virtqueue_push (n->tx_vq,&n->async_tx.elem, n->async_tx.len); +virtqueue_push (n->tx_vq,&n->async_tx.elem,0 );virtio_notify (&n->vdev, n->tx_vq); n->async_tx.elem.out_num = n->async_tx.len =0 ;@@ -754,7 +754,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq) len += ret; -virtqueue_push (vq,&elem, len); +virtqueue_push (vq,&elem,0 );virtio_notify (&n->vdev, vq);if (++num_packets>= n->tx_burst) {
PATCH 12/14 virtio-net: minor code simplification
In reply to Michael S. TsirkinDuring packet filtering, we can now use host hdr len to offset incoming buffer unconditionally.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
hw/virtio-net.c +1 -3
@@ -543,9 +543,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)if (n->promisc)return 1 ; -if (n->has_vnet_hdr) { - ptr +=sizeof (struct virtio_net_hdr ); - } + ptr += n->host_hdr_len;if (!memcmp (&ptr[12 ], vlan,sizeof (vlan))) {int vid =be16_to_cpup ((uint16_t *)(ptr +14 )) &0xfff ;
Signature · 1 line
MST
PATCH 13/14 virtio-net: test peer header support at init time
In reply to Michael S. TsirkinThere's no reason to query header support at random
times: at load or feature query.
Driver also might not query functions. Cleaner to do it at device init.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/virtio-net.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
hw/virtio-net.c +14 -9
@@ -202,16 +202,19 @@ static void virtio_net_reset(VirtIODevice *vdev)memset (n->vlans,0 ,MAX_VLAN >>3 ); } -static int peer_has_vnet_hdr (VirtIONet *n) +static void peer_test_vnet_hdr (VirtIONet *n) {if (!n->nic->nc.peer) -return 0 ; +return ;if (n->nic->nc.peer->info->type !=NET_CLIENT_OPTIONS_KIND_TAP ) -return 0 ; +return ; n->has_vnet_hdr =tap_has_vnet_hdr (n->nic->nc.peer); +} +static int peer_has_vnet_hdr (VirtIONet *n) +{return n->has_vnet_hdr; }
@@ -231,10 +234,7 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features) features |= (1 <<VIRTIO_NET_F_MAC ); -if (peer_has_vnet_hdr (n)) { -tap_using_vnet_hdr (n->nic->nc.peer,1 ); - n->host_hdr_len =sizeof (struct virtio_net_hdr ); - }else { +if (!peer_has_vnet_hdr (n)) { features &= ~(0x1 <<VIRTIO_NET_F_CSUM ); features &= ~(0x1 <<VIRTIO_NET_F_HOST_TSO4 ); features &= ~(0x1 <<VIRTIO_NET_F_HOST_TSO6 );
@@ -937,8 +937,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) }if (n->has_vnet_hdr) { -tap_using_vnet_hdr (n->nic->nc.peer,1 ); - n->host_hdr_len =sizeof (struct virtio_net_hdr ); tap_set_offload(n->nic->nc.peer, (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1, (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
@@ -1032,6 +1030,13 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, n->status =VIRTIO_NET_S_LINK_UP ; n->nic =qemu_new_nic (&net_virtio_info, conf,object_get_typename (OBJECT (dev)), dev->id, n); +peer_test_vnet_hdr (n); +if (peer_has_vnet_hdr (n)) { +tap_using_vnet_hdr (n->nic->nc.peer,1 ); + n->host_hdr_len =sizeof (struct virtio_net_hdr ); + }else { + n->host_hdr_len =0 ; + }qemu_format_nic_info_str (&n->nic->nc, conf->macaddr.a);
Signature · 1 line
MST
PATCH 14/14 virtio-net: enable mrg buf header in tap on linux
In reply to Michael S. TsirkinModern linux supports arbitrary header size, which makes it possible to pass mrg buf header to tap directly without iovec mangling. Use this capability when it is there.
This removes the need to deal with it in vhost-net as we do now.
Signed-off-by Michael S. Tsirkin <[email protected]>
--- hw/vhost_net.c | 13 ------------- hw/virtio-net.c | 26 ++++++++++++++++++-------- 2 files changed, 18 insertions(+), 21 deletions(-)
hw/vhost_net.c +0 -13
@@ -150,10 +150,6 @@ int vhost_net_start(struct vhost_net *net,if (r <0 ) { goto fail_notifiers; } -if (net->dev.acked_features & (1 <<VIRTIO_NET_F_MRG_RXBUF )) { -tap_set_vnet_hdr_len (net->nc, -sizeof (struct virtio_net_hdr_mrg_rxbuf )); - } r =vhost_dev_start (&net->dev, dev);if (r <0 ) {
@@ -179,9 +175,6 @@ fail: } net->nc->info->poll (net->nc, true);vhost_dev_stop (&net->dev, dev); -if (net->dev.acked_features & (1 <<VIRTIO_NET_F_MRG_RXBUF )) { -tap_set_vnet_hdr_len (net->nc,sizeof (struct virtio_net_hdr )); - } fail_start:vhost_dev_disable_notifiers (&net->dev, dev); fail_notifiers:
@@ -199,18 +192,12 @@ void vhost_net_stop(struct vhost_net *net, } net->nc->info->poll (net->nc, true);vhost_dev_stop (&net->dev, dev); -if (net->dev.acked_features & (1 <<VIRTIO_NET_F_MRG_RXBUF )) { -tap_set_vnet_hdr_len (net->nc,sizeof (struct virtio_net_hdr )); - }vhost_dev_disable_notifiers (&net->dev, dev); }void vhost_net_cleanup (struct vhost_net *net) {vhost_dev_cleanup (&net->dev); -if (net->dev.acked_features & (1 <<VIRTIO_NET_F_MRG_RXBUF )) { -tap_set_vnet_hdr_len (net->nc,sizeof (struct virtio_net_hdr )); - }g_free (net); }#else
hw/virtio-net.c +18 -8
@@ -228,6 +228,20 @@ static int peer_has_ufo(VirtIONet *n)return n->has_ufo; } +static void virtio_net_set_mrg_rx_bufs (VirtIONet *n,int mergeable_rx_bufs) +{ + n->mergeable_rx_bufs = mergeable_rx_bufs; + + n->guest_hdr_len = n->mergeable_rx_bufs ? +sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr ); + +if (peer_has_vnet_hdr (n) && +tap_has_vnet_hdr_len (n->nic->nc.peer, n->guest_hdr_len)) { +tap_set_vnet_hdr_len (n->nic->nc.peer, n->guest_hdr_len); + n->host_hdr_len = n->guest_hdr_len; + } +} +static uint32_t virtio_net_get_features (VirtIODevice *vdev,uint32_t features) {VirtIONet *n =to_virtio_net (vdev);
@@ -280,9 +294,7 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features) {VirtIONet *n =to_virtio_net (vdev); - n->mergeable_rx_bufs = !!(features & (1 <<VIRTIO_NET_F_MRG_RXBUF )); - n->guest_hdr_len = n->mergeable_rx_bufs ? -sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr ); +virtio_net_set_mrg_rx_bufs (n, !!(features & (1 <<VIRTIO_NET_F_MRG_RXBUF )));if (n->has_vnet_hdr) { tap_set_offload(n->nic->nc.peer,
@@ -897,9 +909,8 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)qemu_get_buffer (f, n->mac,ETH_ALEN ); n->tx_waiting =qemu_get_be32 (f); - n->mergeable_rx_bufs =qemu_get_be32 (f); - n->guest_hdr_len = n->mergeable_rx_bufs ? -sizeof (struct virtio_net_hdr_mrg_rxbuf ) :sizeof (struct virtio_net_hdr ); + +virtio_net_set_mrg_rx_bufs (n,qemu_get_be32 (f));if (version_id >=3 ) n->status =qemu_get_be16 (f);
@@ -1042,8 +1053,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, n->tx_waiting =0 ; n->tx_burst = net->txburst; - n->mergeable_rx_bufs =0 ; - n->guest_hdr_len =sizeof (struct virtio_net_hdr ); +virtio_net_set_mrg_rx_bufs (n,0 ); n->promisc =1 ;/* for compatibility */ n->mac_table.macs =g_malloc0 (MAC_TABLE_ENTRIES *ETH_ALEN );
Signature · 1 line
MST