| Message ID | 1364902740-24948-2-git-send-email-dmitry@daynix.com |
|---|---|
| State | New |
| Headers | show |
Dmitry Fleytman <dmitry@daynix.com> writes: > 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_GUEST_OFFLOADS. > > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com> (BTW, I like to be CC'd on these things directly, so I don't miss them) The idea is fine. But I dislike the duplication of constants: let's just use the feature bits directly: #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ partial csum */ #define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 in. */ #define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 in. */ #define VIRTIO_NET_F_GUEST_ECN 9 /* Guest can handle TSO[6] w/ ECN in. */ #define VIRTIO_NET_F_GUEST_UFO 10 /* Guest can handle UFO in. */ You want this, because you have to test against them anyway before trying to re-enable them. And secondly, it'll be much clearer if you don't say "change" but "disable and re-enable", which is what's actually allowed. Thanks, Rusty.
On Wed, Apr 03, 2013 at 11:50:19AM +1030, Rusty Russell wrote: > Dmitry Fleytman <dmitry@daynix.com> writes: > > 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_GUEST_OFFLOADS. > > > > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com> > > (BTW, I like to be CC'd on these things directly, so I don't miss them) > > The idea is fine. > > But I dislike the duplication of constants: let's just use the feature > bits directly: > > #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ partial csum */ > #define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 in. */ > #define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 in. */ > #define VIRTIO_NET_F_GUEST_ECN 9 /* Guest can handle TSO[6] w/ ECN in. */ > #define VIRTIO_NET_F_GUEST_UFO 10 /* Guest can handle UFO in. */ > > You want this, because you have to test against them anyway before > trying to re-enable them. > > And secondly, it'll be much clearer if you don't say "change" but > "disable and re-enable", which is what's actually allowed. > > Thanks, > Rusty. Okay and let's make command it bigger, say 64 bit then, in case we add lots of feature bits in the future?
Ok, sending new patches... On Wed, Apr 3, 2013 at 2:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Apr 03, 2013 at 11:50:19AM +1030, Rusty Russell wrote: > > Dmitry Fleytman <dmitry@daynix.com> writes: > > > 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_GUEST_OFFLOADS. > > > > > > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com> > > > > (BTW, I like to be CC'd on these things directly, so I don't miss them) > > > > The idea is fine. > > > > But I dislike the duplication of constants: let's just use the feature > > bits directly: > > > > #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ > partial csum */ > > #define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 > in. */ > > #define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 > in. */ > > #define VIRTIO_NET_F_GUEST_ECN 9 /* Guest can handle TSO[6] > w/ ECN in. */ > > #define VIRTIO_NET_F_GUEST_UFO 10 /* Guest can handle UFO > in. */ > > > > You want this, because you have to test against them anyway before > > trying to re-enable them. > > > > And secondly, it'll be much clearer if you don't say "change" but > > "disable and re-enable", which is what's actually allowed. > > > > Thanks, > > Rusty. > > Okay and let's make command it bigger, say 64 bit then, > in case we add lots of feature bits in the future? > > >
diff --git a/virtio-spec.lyx b/virtio-spec.lyx index 3d2f485..fdba814 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_GUEST_OFFLOADS +\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_GUEST_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_GUEST_OFFLOADS 5 +\end_layout + +\begin_layout Plain Layout + +\change_inserted 460276516 1363765867 + + #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET 0 +\end_layout + +\end_inset + + +\end_layout + +\begin_layout Standard + +\change_inserted 460276516 1363766082 +The class VIRTIO_NET_CTRL_GUEST_OFFLOADS has one command: VIRTIO_NET_CTRL_GUEST_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