Re: [Qemu-devel] [PATCH] tests/vhost-user-bridge: add scattering of incoming packets

[Michael S. Tsirkin]

On Wed, Feb 17, 2016 at 03:02:28PM +0200, Victor Kaplansky wrote:
> This patch adds to the vubr test the scattering of incoming
> packets to the chain of RX buffer.  Also, this patch corrects the
> size of the header preceding the packet in RX buffers.
> 
> Note that this patch doesn't add the support for mergeable
> buffers.
> 
> Signed-off-by: Victor Kaplansky <address@hidden>
> ---
>  tests/vhost-user-bridge.c | 95 
> +++++++++++++++++++++++++++++++----------------
>  1 file changed, 63 insertions(+), 32 deletions(-)
> 
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 9fb09f1d..7f746317 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -47,6 +47,7 @@
>  #include <arpa/inet.h>
>  #include <ctype.h>
>  #include <netdb.h>
> +#include <qemu/osdep.h>
>  
>  #include <linux/vhost.h>
>  
> @@ -186,6 +187,8 @@ typedef struct VubrVirtq {
>  
>  #define VHOST_MEMORY_MAX_NREGIONS    8
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
> +/* v1.0 compliant. */
> +#define VIRTIO_F_VERSION_1           32
>  
>  #define VHOST_LOG_PAGE 4096
>  
> @@ -294,6 +297,7 @@ typedef struct VubrDev {
>      struct sockaddr_in backend_udp_dest;
>      int ready;
>      uint64_t features;
> +    int hdrlen;
>  } VubrDev;
>  
>  static const char *vubr_request_str[] = {
> @@ -484,7 +488,8 @@ vubr_backend_udp_recvbuf(VubrDev *dev, uint8_t *buf, 
> size_t buflen)
>  static void
>  vubr_consume_raw_packet(VubrDev *dev, uint8_t *buf, uint32_t len)
>  {
> -    int hdrlen = sizeof(struct virtio_net_hdr_v1);
> +    int hdrlen = dev->hdrlen;
> +    DPRINT("    hdrlen = %d\n", dev->hdrlen);
>  
>      if (VHOST_USER_BRIDGE_DEBUG) {
>          print_buffer(buf, len);
> @@ -546,6 +551,7 @@ vubr_post_buffer(VubrDev *dev, VubrVirtq *vq, uint8_t 
> *buf, int32_t len)
>      struct vring_avail *avail = vq->avail;
>      struct vring_used *used = vq->used;
>      uint64_t log_guest_addr = vq->log_guest_addr;
> +    int32_t remained_len = len;
>  
>      unsigned int size = vq->size;
>  
> @@ -560,36 +566,49 @@ vubr_post_buffer(VubrDev *dev, VubrVirtq *vq, uint8_t 
> *buf, int32_t len)
>      uint16_t d_index = avail->ring[a_index];
>  
>      int i = d_index;
> +    uint32_t written_len = 0;
>  
> -    DPRINT("Post packet to guest on vq:\n");
> -    DPRINT("    size             = %d\n", vq->size);
> -    DPRINT("    last_avail_index = %d\n", vq->last_avail_index);
> -    DPRINT("    last_used_index  = %d\n", vq->last_used_index);
> -    DPRINT("    a_index = %d\n", a_index);
> -    DPRINT("    u_index = %d\n", u_index);
> -    DPRINT("    d_index = %d\n", d_index);
> -    DPRINT("    desc[%d].addr    = 0x%016"PRIx64"\n", i, desc[i].addr);
> -    DPRINT("    desc[%d].len     = %d\n", i, desc[i].len);
> -    DPRINT("    desc[%d].flags   = %d\n", i, desc[i].flags);
> -    DPRINT("    avail->idx = %d\n", avail_index);
> -    DPRINT("    used->idx  = %d\n", used->idx);
> -
> -    if (!(desc[i].flags & VRING_DESC_F_WRITE)) {
> -        /* FIXME: we should find writable descriptor. */
> -        fprintf(stderr, "Error: descriptor is not writable. Exiting.\n");
> -        exit(1);
> -    }
> +    do {
> +        DPRINT("Post packet to guest on vq:\n");
> +        DPRINT("    size             = %d\n", vq->size);
> +        DPRINT("    last_avail_index = %d\n", vq->last_avail_index);
> +        DPRINT("    last_used_index  = %d\n", vq->last_used_index);
> +        DPRINT("    a_index = %d\n", a_index);
> +        DPRINT("    u_index = %d\n", u_index);
> +        DPRINT("    d_index = %d\n", d_index);
> +        DPRINT("    desc[%d].addr    = 0x%016"PRIx64"\n", i, desc[i].addr);
> +        DPRINT("    desc[%d].len     = %d\n", i, desc[i].len);
> +        DPRINT("    desc[%d].flags   = %d\n", i, desc[i].flags);
> +        DPRINT("    avail->idx = %d\n", avail_index);
> +        DPRINT("    used->idx  = %d\n", used->idx);
> +
> +        if (!(desc[i].flags & VRING_DESC_F_WRITE)) {
> +            /* FIXME: we should find writable descriptor. */
> +            fprintf(stderr, "Error: descriptor is not writable. Exiting.\n");
> +            exit(1);
> +        }
>  
> -    void *chunk_start = (void *)gpa_to_va(dev, desc[i].addr);
> -    uint32_t chunk_len = desc[i].len;
> +        void *chunk_start = (void *)gpa_to_va(dev, desc[i].addr);
> +        uint32_t chunk_len = desc[i].len;
> +        uint32_t chunk_write_len = MIN(remained_len, chunk_len);
>  
> -    if (len <= chunk_len) {
> -        memcpy(chunk_start, buf, len);
> -        vubr_log_write(dev, desc[i].addr, len);
> -    } else {
> -        fprintf(stderr,
> -                "Received too long packet from the backend. Dropping...\n");
> -        return;
> +        memcpy(chunk_start, buf + written_len, chunk_write_len);
> +        vubr_log_write(dev, desc[i].addr, chunk_write_len);
> +        remained_len -= chunk_write_len;
> +        written_len += chunk_write_len;
> +
> +        if ((remained_len == 0) || !(desc[i].flags & VRING_DESC_F_NEXT)) {
> +            break;
> +        }
> +
> +        i = desc[i].next;
> +    } while (1);
> +
> +    if (remained_len > 0) {
> +            fprintf(stderr,
> +                    "Too long packet for RX, remained remained_len = %d, 
> Dropping...\n",
> +                    remained_len);

remained_len -> remaining_len?

> +            return;
>      }
>  
>      /* Add descriptor to the used ring. */
> @@ -697,7 +716,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
>      VubrVirtq *rx_vq = &dev->vq[0];
>      uint8_t buf[4096];
>      struct virtio_net_hdr_v1 *hdr = (struct virtio_net_hdr_v1 *)buf;
> -    int hdrlen = sizeof(struct virtio_net_hdr_v1);
> +    int hdrlen = dev->hdrlen;
>      int buflen = sizeof(buf);
>      int len;
>  
> @@ -706,6 +725,7 @@ vubr_backend_recv_cb(int sock, void *ctx)
>      }
>  
>      DPRINT("\n\n   ***   IN UDP RECEIVE CALLBACK    ***\n\n");
> +    DPRINT("    hdrlen = %d\n", hdrlen);
>  
>      uint16_t avail_index = atomic_mb_read(&rx_vq->avail->idx);
>  
> @@ -717,10 +737,12 @@ vubr_backend_recv_cb(int sock, void *ctx)
>          return;
>      }
>  
> +    memset(buf, 0, hdrlen);
> +    /* TODO: support mergeable buffers. */
> +    if (hdrlen == 12)
> +        hdr->num_buffers = 1;
>      len = vubr_backend_udp_recvbuf(dev, buf + hdrlen, buflen - hdrlen);
>  
> -    *hdr = (struct virtio_net_hdr_v1) { };
> -    hdr->num_buffers = 1;
>      vubr_post_buffer(dev, rx_vq, buf, len + hdrlen);
>  }
>  
> @@ -754,7 +776,8 @@ vubr_get_features_exec(VubrDev *dev, VhostUserMsg *vmsg)
>              ((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>               (1ULL << VHOST_F_LOG_ALL) |
>               (1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) |
> -             (1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
> +             (1ULL << VHOST_USER_F_PROTOCOL_FEATURES) |
> +             (1ULL << VIRTIO_F_VERSION_1));
>  
>      vmsg->size = sizeof(vmsg->payload.u64);
>  

I'm not sure you should advertize VIRTIO_F_VERSION_1 as long as
you don't also do CPU to LE/LE to CPU transformations
when it's negotiated.


Also, you must advertize ANY_LAYOUT with VIRTIO_F_VERSION_1.

> @@ -768,7 +791,15 @@ static int
>  vubr_set_features_exec(VubrDev *dev, VhostUserMsg *vmsg)
>  {
>      DPRINT("u64: 0x%016"PRIx64"\n", vmsg->payload.u64);
> +
>      dev->features = vmsg->payload.u64;
> +    if ((dev->features & (1ULL << VIRTIO_F_VERSION_1)) ||
> +        (dev->features & (1ULL << VIRTIO_NET_F_MRG_RXBUF))) {
> +        dev->hdrlen = 12;
> +    } else {
> +        dev->hdrlen = 10;
> +    }
> +
>      return 0;
>  }
>  
> -- 
> Victor