diff mbox

[v5] virtio-net: dynamic network offloads configuration

Message ID 20130520081814.GA8162@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin May 20, 2013, 8:18 a.m. UTC 
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>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

changes from v4:
    rebased to latest master

I know it's late in the cycle, but it was posted well
before the freeze, and apparently was forgotten, and since it's a known
WHQL blocker I think we should add this - the sooner latest windows
drivers pass WHQL and update to a configuration that can handle e.g.
flexible BAR sizes, the better.

So I rebased this patch myself, but don't have the latest windows
driver bits to test it.
Dmitry, could you please test and report on list?

Thanks!

 hw/net/virtio-net.c            | 99 +++++++++++++++++++++++++++++++++++-------
 include/hw/i386/pc.h           |  4 ++
 include/hw/virtio/virtio-net.h | 13 ++++++
 3 files changed, 101 insertions(+), 15 deletions(-)

Comments

Paolo Bonzini May 20, 2013, 10:52 a.m. UTC | #1 
Il 20/05/2013 10:18, Michael S. Tsirkin ha scritto:
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index beeead7..b315ac9 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -31,6 +31,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 */

Any reason not to use 22?

Is there any hidden reason why bits 2..4 are not used in the spec?

Paolo
Michael S. Tsirkin May 20, 2013, 1:36 p.m. UTC | #2 
On Mon, May 20, 2013 at 12:52:57PM +0200, Paolo Bonzini wrote:
> Il 20/05/2013 10:18, Michael S. Tsirkin ha scritto:
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index beeead7..b315ac9 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -31,6 +31,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 */
> 
> Any reason not to use 22?

There are several
- 22 is already used by VIRTIO_NET_F_MQ
- guest drivers (in development) and spec upstream both use 2

> 
> Is there any hidden reason why bits 2..4 are not used in the spec?
> 
> Paolo

I don't know for sure, sorry.
BTW please send questions on virtio spec on virtio
mailing list, not here.

Thanks!
Paolo Bonzini May 20, 2013, 1:39 p.m. UTC | #3 
Il 20/05/2013 15:36, Michael S. Tsirkin ha scritto:
> On Mon, May 20, 2013 at 12:52:57PM +0200, Paolo Bonzini wrote:
>> Il 20/05/2013 10:18, Michael S. Tsirkin ha scritto:
>>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>>> index beeead7..b315ac9 100644
>>> --- a/include/hw/virtio/virtio-net.h
>>> +++ b/include/hw/virtio/virtio-net.h
>>> @@ -31,6 +31,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 */
>>
>> Any reason not to use 22?
> 
> There are several
> - 22 is already used by VIRTIO_NET_F_MQ

Oops, old copy here.

> - guest drivers (in development) and spec upstream both use 2
> 
> I don't know for sure, sorry.
> BTW please send questions on virtio spec on virtio
> mailing list, not here.

I could not find the message with the spec update, now I found it.  It
was a bit weird because the series had one patch for virtio-spec and one
for QEMU.

Paolo
Michael S. Tsirkin May 20, 2013, 1:46 p.m. UTC | #4 
On Mon, May 20, 2013 at 03:39:33PM +0200, Paolo Bonzini wrote:
> Il 20/05/2013 15:36, Michael S. Tsirkin ha scritto:
> > On Mon, May 20, 2013 at 12:52:57PM +0200, Paolo Bonzini wrote:
> >> Il 20/05/2013 10:18, Michael S. Tsirkin ha scritto:
> >>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> >>> index beeead7..b315ac9 100644
> >>> --- a/include/hw/virtio/virtio-net.h
> >>> +++ b/include/hw/virtio/virtio-net.h
> >>> @@ -31,6 +31,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 */
> >>
> >> Any reason not to use 22?
> > 
> > There are several
> > - 22 is already used by VIRTIO_NET_F_MQ
> 
> Oops, old copy here.
> 
> > - guest drivers (in development) and spec upstream both use 2
> > 
> > I don't know for sure, sorry.
> > BTW please send questions on virtio spec on virtio
> > mailing list, not here.
> 
> I could not find the message with the spec update, now I found it.
>  It
> was a bit weird because the series had one patch for virtio-spec and one
> for QEMU.
> 
> Paolo

It's now in Rusty's git tree:
https://github.com/rustyrussell/virtio-spec
Anthony Liguori May 20, 2013, 2:24 p.m. UTC | #5 
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, May 20, 2013 at 12:52:57PM +0200, Paolo Bonzini wrote:
>> Il 20/05/2013 10:18, Michael S. Tsirkin ha scritto:
>> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>> > index beeead7..b315ac9 100644
>> > --- a/include/hw/virtio/virtio-net.h
>> > +++ b/include/hw/virtio/virtio-net.h
>> > @@ -31,6 +31,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 */
>> 
>> Any reason not to use 22?
>
> There are several
> - 22 is already used by VIRTIO_NET_F_MQ
> - guest drivers (in development) and spec upstream both use 2
>
>> 
>> Is there any hidden reason why bits 2..4 are not used in the spec?
>> 
>> Paolo
>
> I don't know for sure, sorry.

When virtio-net was originally written, it had features for offload in
those bits.  However, the original lguest implementation did not
implement those features so they only existed as feature bits.

After virtio-pci was introduced and we made a serious attempt to get
offload working, we discovered those feature were not sufficient to do
it.  That lead to a new set of feature bits being introduced for
offload which is the set currently used.

This all predated the spec so by the time it was introduced, these bits
were already deprecated and therefore not included.  They've never
actually been used in a real life implementation so there's no risk at
reusing them at this point.

Regards,

Anthony Liguori

> BTW please send questions on virtio spec on virtio
> mailing list, not here.
>
> Thanks!
>
> -- 
> MST
Anthony Liguori May 22, 2013, 10:58 p.m. UTC | #6 
Applied.  Thanks.

Regards,

Anthony Liguori
diff mbox

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index bed0822..1ea9556 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -359,6 +359,34 @@  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)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    return virtio_net_guest_offloads_by_features(vdev->guest_features);
+}
+
 static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -369,12 +397,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++) {
@@ -420,6 +445,43 @@  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)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    uint64_t offloads;
+    size_t s;
+
+    if (!((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & 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)
 {
@@ -590,6 +652,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));
@@ -1110,6 +1174,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) & vdev->guest_features) {
+        qemu_put_be64(f, n->curr_guest_offloads);
+    }
 }
 
 static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
@@ -1167,15 +1235,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,
-                    (vdev->guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
-                    (vdev->guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
-                    (vdev->guest_features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
-                    (vdev->guest_features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
-                    (vdev->guest_features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
-        }
     }
 
     if (version_id >= 9) {
@@ -1209,6 +1268,16 @@  static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
 
+    if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & 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/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2bd7090..93d8357 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -217,6 +217,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/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index beeead7..b315ac9 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -31,6 +31,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. */
@@ -190,6 +192,7 @@  typedef struct VirtIONet {
     size_t config_size;
     char *netclient_name;
     char *netclient_type;
+    uint64_t curr_guest_offloads;
 } VirtIONet;
 
 #define VIRTIO_NET_CTRL_MAC    1
@@ -229,6 +232,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), \
@@ -249,6 +261,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)
 
 #define DEFINE_VIRTIO_NET_PROPERTIES(_state, _field)                           \