On Wed, Mar 27, 2013 at 8:11 PM, Michael S. Tsirkin <[email protected]> wrote:
> On Wed, Mar 27, 2013 at 07:48:18PM +0200, Dmitry Fleytman wrote: > > Michael, thanks for review. > > See my comments inline. > > > > On Wed, Mar 27, 2013 at 5:26 PM, Michael S. Tsirkin <[email protected]> > wrote: > > > > On Wed, Mar 27, 2013 at 03:43:18PM +0200, Dmitry Fleytman wrote: > > > From: Dmitry Fleytman <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > 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 > > 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 > > VIRTIO_NET_F_CTRL_OFFLD > > > [DF] offload feature so guest is unable to use it, after migration to a > newer > > host nothing changes. > > [DF] In case of new<-->old migration all offloads negotiated on startup > via > > features will be enabled (old logic) > > [DF] and guest will be unable to change offloads state via control > channel > > anymore. > > [DF] The second case is a little bit problematic because some offloads > that > > were originally disabled > > [DF] may become enabled after migration but it doesn't look like there > is a > > 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 > > *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), > > > + !!(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); > > > +} > > > + > > > 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 > > *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_offloads = > virtio_net_offloads_by_features(features); > > > + 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, > > 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) > > > +{ > > > + uint32_t offloads; > > > + size_t s; > > > + > > > + if (!((1 << VIRTIO_NET_F_CTRL_OFFLD) & > 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_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, > > 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 > > (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) > > > @@ -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= > > 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) { > > > @@ -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 > > csum */ > > > #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ > partial > > csum */ > > > +#define VIRTIO_NET_F_CTRL_OFFLD 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; > > > + 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) > > > > > > #endif > > > -- > > > 1.8.1.4 > > > > >
_______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
