Re: [Qemu-devel] [PATCH 1/1 V4] virtio-net: dynamic network offloads configuration

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Apr 22, 2013 at 12:58:26PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Apr 07, 2013 at 09:34:08AM +0300, Dmitry Fleytman wrote:
> >> From: Dmitry Fleytman <dfleytma@redhat.com>
> >> 
> >> Virtio-net driver currently negotiates network offloads
> >> on startup via features mechanism and have no ability to
> >> disable and re-enable offloads later.
> >> This patch introduced a new control command that allows
> >> to configure device network offloads state dynamically.
> >> The patch also introduces a new feature flag
> >> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >> 
> >> Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I was looking for a Reviewed-by, not just an Acked-by.
> 
> Regards,
> 
> Anthony Liguori
> 


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> >
> > Pls pick this up for 1.5.
> >
> >> ---
> >>  hw/pc.h         |  4 +++
> >>  hw/virtio-net.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++---------
> >>  hw/virtio-net.h | 13 ++++++++
> >>  3 files changed, 99 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/hw/pc.h b/hw/pc.h
> >> index 8e1dd4c..7ca4698 100644
> >> --- a/hw/pc.h
> >> +++ b/hw/pc.h
> >> @@ -221,6 +221,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >>              .property = "vectors",\
> >>              /* DEV_NVECTORS_UNSPECIFIED as a uint32_t string */\
> >>              .value    = stringify(0xFFFFFFFF),\
> >> +        },{ \
> >> +            .driver   = "virtio-net-pci", \
> >> +            .property = "ctrl_guest_offloads", \
> >> +            .value    = "off", \
> >>          },{\
> >>              .driver   = "e1000",\
> >>              .property = "romfile",\
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 5917740..a92d061 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -360,6 +360,33 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
> >>      return features;
> >>  }
> >>  
> >> +static void virtio_net_apply_guest_offloads(VirtIONet *n)
> >> +{
> >> +    tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
> >> +            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_CSUM)),
> >> +            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO4)),
> >> +            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_TSO6)),
> >> +            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_ECN)),
> >> +            !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_UFO)));
> >> +}
> >> +
> >> +static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
> >> +{
> >> +    static const uint64_t guest_offloads_mask =
> >> +        (1ULL << VIRTIO_NET_F_GUEST_CSUM) |
> >> +        (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
> >> +        (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
> >> +        (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
> >> +        (1ULL << VIRTIO_NET_F_GUEST_UFO);
> >> +
> >> +    return guest_offloads_mask & features;
> >> +}
> >> +
> >> +static inline uint64_t virtio_net_supported_guest_offloads(VirtIONet *n)
> >> +{
> >> +    return virtio_net_guest_offloads_by_features(n->vdev.guest_features);
> >> +}
> >> +
> >>  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >>  {
> >>      VirtIONet *n = to_virtio_net(vdev);
> >> @@ -371,12 +398,9 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >>      virtio_net_set_mrg_rx_bufs(n, !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)));
> >>  
> >>      if (n->has_vnet_hdr) {
> >> -        tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
> >> -                        (features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
> >> -                        (features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
> >> -                        (features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
> >> -                        (features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
> >> -                        (features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
> >> +        n->curr_guest_offloads =
> >> +            virtio_net_guest_offloads_by_features(features);
> >> +        virtio_net_apply_guest_offloads(n);
> >>      }
> >>  
> >>      for (i = 0;  i < n->max_queues; i++) {
> >> @@ -422,6 +446,42 @@ static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> >>      return VIRTIO_NET_OK;
> >>  }
> >>  
> >> +static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
> >> +                                     struct iovec *iov, unsigned int iov_cnt)
> >> +{
> >> +    uint64_t offloads;
> >> +    size_t s;
> >> +
> >> +    if (!((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & n->vdev.guest_features)) {
> >> +        return VIRTIO_NET_ERR;
> >> +    }
> >> +
> >> +    s = iov_to_buf(iov, iov_cnt, 0, &offloads, sizeof(offloads));
> >> +    if (s != sizeof(offloads)) {
> >> +        return VIRTIO_NET_ERR;
> >> +    }
> >> +
> >> +    if (cmd == VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET) {
> >> +        uint64_t supported_offloads;
> >> +
> >> +        if (!n->has_vnet_hdr) {
> >> +            return VIRTIO_NET_ERR;
> >> +        }
> >> +
> >> +        supported_offloads = virtio_net_supported_guest_offloads(n);
> >> +        if (offloads & ~supported_offloads) {
> >> +            return VIRTIO_NET_ERR;
> >> +        }
> >> +
> >> +        n->curr_guest_offloads = offloads;
> >> +        virtio_net_apply_guest_offloads(n);
> >> +
> >> +        return VIRTIO_NET_OK;
> >> +    } else {
> >> +        return VIRTIO_NET_ERR;
> >> +    }
> >> +}
> >> +
> >>  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> >>                                   struct iovec *iov, unsigned int iov_cnt)
> >>  {
> >> @@ -591,6 +651,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> >>              status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, iov_cnt);
> >>          } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
> >>              status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt);
> >> +        } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
> >> +            status = virtio_net_handle_offloads(n, ctrl.cmd, iov, iov_cnt);
> >>          }
> >>  
> >>          s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status));
> >> @@ -1100,6 +1162,10 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> >>              qemu_put_be32(f, n->vqs[i].tx_waiting);
> >>          }
> >>      }
> >> +
> >> +    if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & n->vdev.guest_features) {
> >> +        qemu_put_be64(f, n->curr_guest_offloads);
> >> +    }
> >>  }
> >>  
> >>  static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> >> @@ -1156,15 +1222,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> >>              error_report("virtio-net: saved image requires vnet_hdr=on");
> >>              return -1;
> >>          }
> >> -
> >> -        if (n->has_vnet_hdr) {
> >> -            tap_set_offload(qemu_get_queue(n->nic)->peer,
> >> -                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
> >> -                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
> >> -                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
> >> -                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
> >> -                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
> >> -        }
> >>      }
> >>  
> >>      if (version_id >= 9) {
> >> @@ -1198,6 +1255,16 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
> >>          }
> >>      }
> >>  
> >> +    if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & n->vdev.guest_features) {
> >> +        n->curr_guest_offloads = qemu_get_be64(f);
> >> +    } else {
> >> +        n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
> >> +    }
> >> +
> >> +    if (peer_has_vnet_hdr(n)) {
> >> +        virtio_net_apply_guest_offloads(n);
> >> +    }
> >> +
> >>      virtio_net_set_queues(n);
> >>  
> >>      /* Find the first multicast entry in the saved MAC filter */
> >> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> >> index 4d1a8cd..70e362d 100644
> >> --- a/hw/virtio-net.h
> >> +++ b/hw/virtio-net.h
> >> @@ -27,6 +27,8 @@
> >>  /* The feature bitmap for virtio net */
> >>  #define VIRTIO_NET_F_CSUM       0       /* Host handles pkts w/ partial csum */
> >>  #define VIRTIO_NET_F_GUEST_CSUM 1       /* Guest handles pkts w/ partial csum */
> >> +#define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Control channel offload
> >> +                                         * configuration support */
> >>  #define VIRTIO_NET_F_MAC        5       /* Host has given MAC address. */
> >>  #define VIRTIO_NET_F_GSO        6       /* Host handles pkts w/ any GSO type */
> >>  #define VIRTIO_NET_F_GUEST_TSO4 7       /* Guest can handle TSOv4 in. */
> >> @@ -182,6 +184,7 @@ typedef struct VirtIONet {
> >>      uint16_t max_queues;
> >>      uint16_t curr_queues;
> >>      size_t config_size;
> >> +    uint64_t curr_guest_offloads;
> >>  } VirtIONet;
> >>  
> >>  #define VIRTIO_NET_CTRL_MAC    1
> >> @@ -221,6 +224,15 @@ struct virtio_net_ctrl_mq {
> >>   #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
> >>   #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
> >>  
> >> +/*
> >> + * Control network offloads
> >> + *
> >> + * Dynamic offloads are available with the
> >> + * VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature bit.
> >> + */
> >> +#define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
> >> + #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
> >> +
> >>  #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
> >>          DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
> >>          DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
> >> @@ -241,6 +253,7 @@ struct virtio_net_ctrl_mq {
> >>          DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
> >>          DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
> >>          DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true), \
> >> +        DEFINE_PROP_BIT("ctrl_guest_offloads", _state, _field, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true), \
> >>          DEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ, false)
> >>  
> >>  #endif
> >> -- 
> >> 1.8.1.4