On Wed, Mar 27, 2013 at 8:11 PM, Michael S. Tsirkin [off-list ref] wrote:

On Wed, Mar 27, 2013 at 07:48:18PM +0200, Dmitry Fleytman wrote:

quoted

Michael, thanks for review.
See my comments inline.

On Wed, Mar 27, 2013 at 5:26 PM, Michael S. Tsirkin [off-list ref]

wrote:

quoted

    On Wed, Mar 27, 2013 at 03:43:18PM +0200, Dmitry Fleytman wrote:
    > From: Dmitry Fleytman [off-list ref]
    >
    > Virtio-net driver currently negotiates network offloads
    > on startup via features mechanism and have no ability to
    > change offloads state 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_OFFLD.
    >
    > Signed-off-by: Dmitry Fleytman [off-list ref]
    > ---
    >  hw/virtio-net.c | 110
    +++++++++++++++++++++++++++++++++++++++++++++++---------
    >  hw/virtio-net.h |  19 ++++++++++
    >  2 files changed, 113 insertions(+), 16 deletions(-)
    >
    > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
    > index 4bb49eb..0c23126 100644
    > --- a/hw/virtio-net.c
    > +++ b/hw/virtio-net.c
    > @@ -21,7 +21,7 @@
    >  #include "hw/virtio-net.h"
    >  #include "hw/vhost_net.h"
    >
    > -#define VIRTIO_NET_VM_VERSION    11
    > +#define VIRTIO_NET_VM_VERSION    12

    This will break cross-version migration, we do it differently now:
    - disable feature with old machine types

    - only pass offloads around if feature set

    - same for receive side



[DF] Not sure I got your idea.
[DF] Current logic aimed ensure proper operation in all old <--> new

migration

quoted

cases:

AFAIK if you change VIRTIO_NET_VM_VERSION old host willl fail to parse
it.

Oh, it could be I missed this. I'll double-check.

 > [DF] In case of old<-->new migration host doesn't expose

quoted

VIRTIO_NET_F_CTRL_OFFLD

quoted

[DF] offload feature so guest is unable to use it, after migration to a

newer

quoted

host nothing changes.
[DF] In case of new<-->old migration all offloads negotiated on startup

via

quoted

features will be enabled (old logic)
[DF] and guest will be unable to change offloads state via control

channel

quoted

anymore.
[DF] The second case is a little bit problematic because some offloads

that

quoted

were originally disabled
[DF] may become enabled after migration but it doesn't look like there

is a

quoted

better solution.
[DF] Do I miss something?


    >
    >  #define MAC_TABLE_ENTRIES    64
    >  #define MAX_VLAN    (1 << 12)   /* Per 802.1Q definition */
    > @@ -360,6 +360,48 @@ static uint32_t

virtio_net_bad_features(VirtIODevice

quoted

    *vdev)
    >      return features;
    >  }
    >
    > +static void virtio_net_apply_offloads(VirtIONet *n)
    > +{
    > +    tap_set_offload(qemu_get_subqueue(n->nic, 0)->peer,
    > +                    !!(n->curr_offloads &
    VIRTIO_NET_OFFLOAD_GUEST_CSUM),
    > +                    !!(n->curr_offloads &
    VIRTIO_NET_OFFLOAD_GUEST_TSO4),
    > +                    !!(n->curr_offloads &
    VIRTIO_NET_OFFLOAD_GUEST_TSO6),
    > +                    !!(n->curr_offloads &

VIRTIO_NET_OFFLOAD_GUEST_ECN),

quoted

    > +                    !!(n->curr_offloads &
    VIRTIO_NET_OFFLOAD_GUEST_UFO));
    > +}
    > +
    > +static uint32_t virtio_net_offloads_by_features(uint32_t features)
    > +{
    > +    uint32_t offloads = 0;
    > +
    > +    if ((1 << VIRTIO_NET_F_GUEST_CSUM) & features) {
    > +        offloads |= VIRTIO_NET_OFFLOAD_GUEST_CSUM;
    > +    }
    > +
    > +    if ((1 << VIRTIO_NET_F_GUEST_TSO4) & features) {
    > +        offloads |= VIRTIO_NET_OFFLOAD_GUEST_TSO4;
    > +    }
    > +
    > +    if ((1 << VIRTIO_NET_F_GUEST_TSO6) & features) {
    > +        offloads |= VIRTIO_NET_OFFLOAD_GUEST_TSO6;
    > +    }
    > +
    > +    if ((1 << VIRTIO_NET_F_GUEST_ECN) & features) {
    > +        offloads |= VIRTIO_NET_OFFLOAD_GUEST_ECN;
    > +    }
    > +
    > +    if ((1 << VIRTIO_NET_F_GUEST_UFO) & features) {
    > +        offloads |= VIRTIO_NET_OFFLOAD_GUEST_UFO;
    > +    }
    > +
    > +    return offloads;
    > +}
    > +
    > +static inline uint32_t virtio_net_supported_offloads(VirtIONet *n)
    > +{
    > +    return

virtio_net_offloads_by_features(n->vdev.guest_features);

quoted

    > +}
    > +
    >  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t
    features)
    >  {
    >      VirtIONet *n = to_virtio_net(vdev);
    > @@ -371,12 +413,8 @@ static void

virtio_net_set_features(VirtIODevice

quoted

    *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);

quoted

    > +        n->curr_offloads =

virtio_net_offloads_by_features(features);

quoted

    > +        virtio_net_apply_offloads(n);
    >      }
    >
    >      for (i = 0;  i < n->max_queues; i++) {
    > @@ -422,6 +460,42 @@ static int

virtio_net_handle_rx_mode(VirtIONet *n,

quoted

    uint8_t cmd,
    >      return VIRTIO_NET_OK;
    >  }
    >
    > +static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
    > +                                     struct iovec *iov, unsigned

int

quoted

    iov_cnt)
    > +{
    > +    uint32_t offloads;
    > +    size_t s;
    > +
    > +    if (!((1 << VIRTIO_NET_F_CTRL_OFFLD) &

n->vdev.guest_features)) {

quoted

    > +        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_OFFLOADS_SET) {
    > +        uint32_t supported_offloads;
    > +
    > +        if (!n->has_vnet_hdr) {
    > +            return VIRTIO_NET_ERR;
    > +        }
    > +
    > +        supported_offloads = virtio_net_supported_offloads(n);
    > +        if (offloads & ~supported_offloads) {
    > +            return VIRTIO_NET_ERR;
    > +        }
    > +
    > +        n->curr_offloads = offloads;
    > +        virtio_net_apply_offloads(n);

    In the spec you said that only features
    available in the offloads can be
    enabled. Probably a good idea to validate
    and error if that's not the case.


[DF] Not sure I understood correctly this part,
[DF] Isn't following check enough to ensure this:
[DF] > +        supported_offloads = virtio_net_supported_offloads(n);
[DF] > +        if (offloads & ~supported_offloads) {
[DF] > +            return VIRTIO_NET_ERR;



    > +
    > +        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)
    >  {
    > @@ -590,6 +664,8 @@ static void virtio_net_handle_ctrl(VirtIODevice
    *vdev, VirtQueue *vq)
    >              status = virtio_net_handle_vlan_table(n, ctrl.cmd,

iov,

quoted

    iov_cnt);
    >          } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) {
    >              status = virtio_net_handle_mq(n, ctrl.cmd, &elem);
    > +        } else if (ctrl.class == VIRTIO_NET_CTRL_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

quoted

    (status));
    > @@ -1099,6 +1175,7 @@ static void virtio_net_save(QEMUFile *f, void
    *opaque)
    >              qemu_put_be32(f, n->vqs[i].tx_waiting);
    >          }
    >      }
    > +    qemu_put_be32(f, n->curr_offloads);

    >  }
    >
    >  static int virtio_net_load(QEMUFile *f, void *opaque, int

version_id)

quoted

    > @@ -1155,15 +1232,6 @@ static int virtio_net_load(QEMUFile *f, void
    *opaque, int version_id)
    >              error_report("virtio-net: saved image requires

vnet_hdr=

quoted

    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)

quoted

    & 1,
    > -                    (n->vdev.guest_features >>

VIRTIO_NET_F_GUEST_TSO4)

quoted

    & 1,
    > -                    (n->vdev.guest_features >>

VIRTIO_NET_F_GUEST_TSO6)

quoted

    & 1,
    > -                    (n->vdev.guest_features >>

VIRTIO_NET_F_GUEST_ECN)

quoted

    & 1,
    > -                    (n->vdev.guest_features >>

VIRTIO_NET_F_GUEST_UFO)

quoted

    & 1);
    > -        }
    >      }
    >
    >      if (version_id >= 9) {
    > @@ -1197,6 +1265,16 @@ static int virtio_net_load(QEMUFile *f, void
    *opaque, int version_id)
    >          }
    >      }
    >
    > +    if (version_id >= 11) {
    > +        n->curr_offloads = qemu_get_be32(f);
    > +    } else {
    > +        n->curr_offloads = virtio_net_supported_offloads(n);
    > +    }
    > +
    > +    if (n->has_vnet_hdr) {
    > +        virtio_net_apply_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..649a4e8 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

quoted

    csum */
    >  #define VIRTIO_NET_F_GUEST_CSUM 1       /* Guest handles pkts w/

partial

quoted

    csum */
    > +#define VIRTIO_NET_F_CTRL_OFFLD 2       /* Control channel offload
    > +                                         * configuration support

*/

quoted

    >  #define VIRTIO_NET_F_MAC        5       /* Host has given MAC

address. *

quoted

    /
    >  #define VIRTIO_NET_F_GSO        6       /* Host handles pkts w/

any GSO

quoted

    type */
    >  #define VIRTIO_NET_F_GUEST_TSO4 7       /* Guest can handle TSOv4

in. */

quoted

    > @@ -182,6 +184,7 @@ typedef struct VirtIONet {
    >      uint16_t max_queues;
    >      uint16_t curr_queues;
    >      size_t config_size;
    > +    uint32_t curr_offloads;
    >  } VirtIONet;
    >
    >  #define VIRTIO_NET_CTRL_MAC    1
    > @@ -221,6 +224,21 @@ 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_OFFLD feature bit.
    > + */
    > +#define VIRTIO_NET_CTRL_OFFLOADS   5
    > + #define VIRTIO_NET_CTRL_OFFLOADS_SET        0
    > +
    > +#define VIRTIO_NET_OFFLOAD_GUEST_CSUM       1
    > +#define VIRTIO_NET_OFFLOAD_GUEST_TSO4       2
    > +#define VIRTIO_NET_OFFLOAD_GUEST_TSO6       4
    > +#define VIRTIO_NET_OFFLOAD_GUEST_ECN        8
    > +#define VIRTIO_NET_OFFLOAD_GUEST_UFO       16
    > +
    >  #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 +259,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_offload", _state, _field,
    VIRTIO_NET_F_CTRL_OFFLD, true), \
    >          DEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ,

false)

quoted

    >
    >  #endif
    > --
    > 1.8.1.4