On Wed, Mar 27, 2013 at 8:11 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
AFAIK if you change VIRTIO_NET_VM_VERSION old host willl fail to parseOn 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 <mst@xxxxxxxxxx> wrote:
>
> On Wed, Mar 27, 2013 at 03:43:18PM +0200, Dmitry Fleytman wrote:
> > From: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
> >
> > 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 <dfleytma@xxxxxxxxxx>
> > ---
> > 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:
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 Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
