* [PATCH 0/2] virtio-spec: dynamic network offloads configuration
@ 2013-03-27 13:43 Dmitry Fleytman
2013-03-27 13:43 ` [PATCH 1/2] " Dmitry Fleytman
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 13:43 UTC (permalink / raw)
To: virtualization; +Cc: Yan Vugenfirer, Dmitry Fleytman, Michael S. Tsirkin
From: Dmitry Fleytman <dfleytma@redhat.com>
One of recently introduced Windows features (RSC)
requires network driver to be able to enable and disable
HW LRO offload on the fly without device reinitialization.
Current Virtio specification doesn't support this requirement.
The solution proposed by following spec patch is to add
a new control command for this purpose.
The same solution may be used in Linux driver for ethtool interface
implementation.
Patch for QEMU mainline that implements this specification change
attached as well for reference.
Dmitry Fleytman (1):
virtio-net: dynamic network offloads configuration
hw/virtio-net.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++---------
hw/virtio-net.h | 19 ++++++++++
2 files changed, 113 insertions(+), 16 deletions(-)
--
1.8.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] virtio-spec: dynamic network offloads configuration
2013-03-27 13:43 [PATCH 0/2] virtio-spec: dynamic network offloads configuration Dmitry Fleytman
@ 2013-03-27 13:43 ` Dmitry Fleytman
2013-03-27 15:20 ` Michael S. Tsirkin
2013-03-27 13:43 ` [PATCH 2/2] virtio-net: " Dmitry Fleytman
2013-03-27 15:27 ` [PATCH 0/2] virtio-spec: " Michael S. Tsirkin
2 siblings, 1 reply; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 13:43 UTC (permalink / raw)
To: virtualization; +Cc: Yan Vugenfirer, Dmitry Fleytman, Michael S. Tsirkin
From: Dmitry Fleytman <dfleytma@redhat.com>
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@redhat.com>
---
virtio-spec.lyx | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 3d2f485..9e5125e 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -60,6 +60,7 @@
\author -1930653948 "Amos Kong"
\author -608949062 "Rusty Russell,,,"
\author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
+\author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
\author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
\author 1531152142 "Paolo Bonzini,,,"
\author 1717892615 "Alexey Zaytsev,,,"
@@ -4261,6 +4262,20 @@ VIRTIO_NET_F_GUEST_CSUM
\end_inset
(1) Guest handles packets with partial checksum
+\change_inserted 460276516 1363712169
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 460276516 1363712334
+VIRTIO_NET_F_CTRL_OFFLD
+\begin_inset space ~
+\end_inset
+
+(2) Control channel offloads reconfiguration support.
+\change_unchanged
+
\end_layout
\begin_layout Description
@@ -5675,6 +5690,134 @@ virtqueue_pairs = 1
\end_layout
+\begin_layout Subsection*
+
+\change_inserted 460276516 1363765850
+Offloads State Configuration
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363765861
+If the VIRTIO_NET_F_CTRL_OFFLOADS feature is negotiated, the driver can
+ send control commands for dynamic offloads state configuration.
+\end_layout
+
+\begin_layout Subsubsection*
+
+\change_inserted 460276516 1363765928
+Setting offloads state
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363713225
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765996
+
+u32 offloads;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765997
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766044
+
+#define VIRTIO_NET_OFFLOAD_GUEST_CSUM 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766051
+
+#define VIRTIO_NET_OFFLOAD_GUEST_TSO4 2
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766055
+
+#define VIRTIO_NET_OFFLOAD_GUEST_TSO6 4
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766064
+
+#define VIRTIO_NET_OFFLOAD_GUEST_ECN 8
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766035
+
+#define VIRTIO_NET_OFFLOAD_GUEST_UFO 16
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766031
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765865
+
+#define VIRTIO_NET_CTRL_OFFLOADS 5
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765867
+
+ #define VIRTIO_NET_CTRL_OFFLOADS_SET 0
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766082
+The class VIRTIO_NET_CTRL_OFFLOADS has one command: VIRTIO_NET_CTRL_OFFLOADS_SET
+ applies the new offloads configuration.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766435
+u32 value passed as command data is a bitmask, bits set define offloads
+ to be enabled, bits cleared - offloads to be disabled.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766757
+There is a corresponding device feature for each offload.
+ Upon feature negotiation corresponding offload gets enabled to preserve
+ backward compartibility.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766720
+Corresponding feature must be negotiated at startup in order to allow dynamic
+ change of specific offload state.
+\end_layout
+
\begin_layout Chapter*
Appendix D: Block Device
\end_layout
--
1.8.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] virtio-net: dynamic network offloads configuration
2013-03-27 13:43 [PATCH 0/2] virtio-spec: dynamic network offloads configuration Dmitry Fleytman
2013-03-27 13:43 ` [PATCH 1/2] " Dmitry Fleytman
@ 2013-03-27 13:43 ` Dmitry Fleytman
2013-03-27 15:26 ` Michael S. Tsirkin
2013-03-27 15:27 ` [PATCH 0/2] virtio-spec: " Michael S. Tsirkin
2 siblings, 1 reply; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 13:43 UTC (permalink / raw)
To: virtualization; +Cc: Yan Vugenfirer, Dmitry Fleytman, Michael S. Tsirkin
From: Dmitry Fleytman <dfleytma@redhat.com>
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@redhat.com>
---
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
#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);
+
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] virtio-spec: dynamic network offloads configuration
2013-03-27 13:43 ` [PATCH 1/2] " Dmitry Fleytman
@ 2013-03-27 15:20 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-03-27 15:20 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
On Wed, Mar 27, 2013 at 03:43:17PM +0200, 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
> 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@redhat.com>
Looks sane to me. We'll be able to use this in Linux too.
Rusty?
> ---
> virtio-spec.lyx | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 143 insertions(+)
>
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 3d2f485..9e5125e 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -60,6 +60,7 @@
> \author -1930653948 "Amos Kong"
> \author -608949062 "Rusty Russell,,,"
> \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
> +\author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
> \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
> \author 1531152142 "Paolo Bonzini,,,"
> \author 1717892615 "Alexey Zaytsev,,,"
> @@ -4261,6 +4262,20 @@ VIRTIO_NET_F_GUEST_CSUM
> \end_inset
>
> (1) Guest handles packets with partial checksum
> +\change_inserted 460276516 1363712169
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 460276516 1363712334
> +VIRTIO_NET_F_CTRL_OFFLD
> +\begin_inset space ~
> +\end_inset
> +
> +(2) Control channel offloads reconfiguration support.
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Description
> @@ -5675,6 +5690,134 @@ virtqueue_pairs = 1
>
> \end_layout
>
> +\begin_layout Subsection*
> +
> +\change_inserted 460276516 1363765850
> +Offloads State Configuration
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363765861
> +If the VIRTIO_NET_F_CTRL_OFFLOADS feature is negotiated, the driver can
> + send control commands for dynamic offloads state configuration.
> +\end_layout
> +
> +\begin_layout Subsubsection*
> +
> +\change_inserted 460276516 1363765928
> +Setting offloads state
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363713225
> +\begin_inset listings
> +inline false
> +status open
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363765996
> +
> +u32 offloads;
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363765997
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766044
> +
> +#define VIRTIO_NET_OFFLOAD_GUEST_CSUM 1
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766051
> +
> +#define VIRTIO_NET_OFFLOAD_GUEST_TSO4 2
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766055
> +
> +#define VIRTIO_NET_OFFLOAD_GUEST_TSO6 4
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766064
> +
> +#define VIRTIO_NET_OFFLOAD_GUEST_ECN 8
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766035
> +
> +#define VIRTIO_NET_OFFLOAD_GUEST_UFO 16
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363766031
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363765865
> +
> +#define VIRTIO_NET_CTRL_OFFLOADS 5
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 460276516 1363765867
> +
> + #define VIRTIO_NET_CTRL_OFFLOADS_SET 0
> +\end_layout
> +
> +\end_inset
> +
> +
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363766082
> +The class VIRTIO_NET_CTRL_OFFLOADS has one command: VIRTIO_NET_CTRL_OFFLOADS_SET
> + applies the new offloads configuration.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363766435
> +u32 value passed as command data is a bitmask, bits set define offloads
> + to be enabled, bits cleared - offloads to be disabled.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363766757
> +There is a corresponding device feature for each offload.
> + Upon feature negotiation corresponding offload gets enabled to preserve
> + backward compartibility.
> +\end_layout
> +
> +\begin_layout Standard
> +
> +\change_inserted 460276516 1363766720
> +Corresponding feature must be negotiated at startup in order to allow dynamic
> + change of specific offload state.
> +\end_layout
> +
> \begin_layout Chapter*
> Appendix D: Block Device
> \end_layout
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] virtio-net: dynamic network offloads configuration
2013-03-27 13:43 ` [PATCH 2/2] virtio-net: " Dmitry Fleytman
@ 2013-03-27 15:26 ` Michael S. Tsirkin
2013-03-27 17:48 ` Dmitry Fleytman
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-03-27 15:26 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
On Wed, Mar 27, 2013 at 03:43:18PM +0200, 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
> 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@redhat.com>
> ---
> 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
>
> #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.
> +
> + 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] virtio-spec: dynamic network offloads configuration
2013-03-27 13:43 [PATCH 0/2] virtio-spec: dynamic network offloads configuration Dmitry Fleytman
2013-03-27 13:43 ` [PATCH 1/2] " Dmitry Fleytman
2013-03-27 13:43 ` [PATCH 2/2] virtio-net: " Dmitry Fleytman
@ 2013-03-27 15:27 ` Michael S. Tsirkin
2013-03-27 17:49 ` Dmitry Fleytman
2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-03-27 15:27 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
On Wed, Mar 27, 2013 at 03:43:16PM +0200, Dmitry Fleytman wrote:
> From: Dmitry Fleytman <dfleytma@redhat.com>
>
> One of recently introduced Windows features (RSC)
> requires network driver to be able to enable and disable
> HW LRO offload on the fly without device reinitialization.
>
> Current Virtio specification doesn't support this requirement.
> The solution proposed by following spec patch is to add
> a new control command for this purpose.
>
> The same solution may be used in Linux driver for ethtool interface
> implementation.
>
> Patch for QEMU mainline that implements this specification change
> attached as well for reference.
A minor note: sticking _GUEST_ in the command name
might be a good idea.
Sents some more minor comments per-patch.
> Dmitry Fleytman (1):
> virtio-net: dynamic network offloads configuration
>
> hw/virtio-net.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++---------
> hw/virtio-net.h | 19 ++++++++++
> 2 files changed, 113 insertions(+), 16 deletions(-)
>
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] virtio-net: dynamic network offloads configuration
2013-03-27 15:26 ` Michael S. Tsirkin
@ 2013-03-27 17:48 ` Dmitry Fleytman
2013-03-27 18:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 17:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 11216 bytes --]
Michael, thanks for review.
See my comments inline.
On Wed, Mar 27, 2013 at 5:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Mar 27, 2013 at 03:43:18PM +0200, 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
> > 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@redhat.com>
> > ---
> > 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:
[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
>
[-- Attachment #1.2: Type: text/html, Size: 15133 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] virtio-spec: dynamic network offloads configuration
2013-03-27 15:27 ` [PATCH 0/2] virtio-spec: " Michael S. Tsirkin
@ 2013-03-27 17:49 ` Dmitry Fleytman
0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 17:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 1218 bytes --]
On Wed, Mar 27, 2013 at 5:27 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Mar 27, 2013 at 03:43:16PM +0200, Dmitry Fleytman wrote:
> > From: Dmitry Fleytman <dfleytma@redhat.com>
> >
> > One of recently introduced Windows features (RSC)
> > requires network driver to be able to enable and disable
> > HW LRO offload on the fly without device reinitialization.
> >
> > Current Virtio specification doesn't support this requirement.
> > The solution proposed by following spec patch is to add
> > a new control command for this purpose.
> >
> > The same solution may be used in Linux driver for ethtool interface
> > implementation.
> >
> > Patch for QEMU mainline that implements this specification change
> > attached as well for reference.
>
> A minor note: sticking _GUEST_ in the command name
> might be a good idea.
> Sents some more minor comments per-patch.
>
>
[DF] Ok, will be fixed and resubmitted.
>
> > Dmitry Fleytman (1):
> > virtio-net: dynamic network offloads configuration
> >
> > hw/virtio-net.c | 110
> +++++++++++++++++++++++++++++++++++++++++++++++---------
> > hw/virtio-net.h | 19 ++++++++++
> > 2 files changed, 113 insertions(+), 16 deletions(-)
> >
> > --
> > 1.8.1.4
>
[-- Attachment #1.2: Type: text/html, Size: 2000 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] virtio-net: dynamic network offloads configuration
2013-03-27 17:48 ` Dmitry Fleytman
@ 2013-03-27 18:11 ` Michael S. Tsirkin
2013-03-27 20:00 ` Dmitry Fleytman
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2013-03-27 18:11 UTC (permalink / raw)
To: Dmitry Fleytman; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
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 <mst@redhat.com> wrote:
>
> On Wed, Mar 27, 2013 at 03:43:18PM +0200, 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
> > 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@redhat.com>
> > ---
> > 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.
> [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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] virtio-net: dynamic network offloads configuration
2013-03-27 18:11 ` Michael S. Tsirkin
@ 2013-03-27 20:00 ` Dmitry Fleytman
0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Fleytman @ 2013-03-27 20:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Yan Vugenfirer, Dmitry Fleytman, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 13411 bytes --]
On Wed, Mar 27, 2013 at 8:11 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com>
> wrote:
> >
> > On Wed, Mar 27, 2013 at 03:43:18PM +0200, 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
> > > 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@redhat.com>
> > > ---
> > > 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
> >
> >
>
[-- Attachment #1.2: Type: text/html, Size: 17230 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-03-27 20:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-27 13:43 [PATCH 0/2] virtio-spec: dynamic network offloads configuration Dmitry Fleytman
2013-03-27 13:43 ` [PATCH 1/2] " Dmitry Fleytman
2013-03-27 15:20 ` Michael S. Tsirkin
2013-03-27 13:43 ` [PATCH 2/2] virtio-net: " Dmitry Fleytman
2013-03-27 15:26 ` Michael S. Tsirkin
2013-03-27 17:48 ` Dmitry Fleytman
2013-03-27 18:11 ` Michael S. Tsirkin
2013-03-27 20:00 ` Dmitry Fleytman
2013-03-27 15:27 ` [PATCH 0/2] virtio-spec: " Michael S. Tsirkin
2013-03-27 17:49 ` Dmitry Fleytman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).