All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Updates to public/io/netif.h
@ 2015-10-19 13:53 Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id Paul Durrant
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

This series makes several modifications to netif.h in anticipation of
implementing NDIS RSS support in the Windows frontend driver.

Patch #1 documents the (sad) reality of the netif_rx_request/response id
field, which has been long overdue.

Patch #2 adds a definition of the NETRXF_gso_prefix flag which has been
present in the Linux variant of the header for several years

Patch #3 adds documentation for how negotiation of a hash algortithm to
be used on the frontend receive side should be done.

Patch #4 adds a new netif_extra_info type for passing hash values between
backend and frontend (or vice versa).

Patch #5 (new in v4) reduces comment duplication and fixes formatting.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id
  2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
@ 2015-10-19 13:53 ` Paul Durrant
  2015-10-19 16:00   ` Jan Beulich
  2015-10-19 13:53 ` [PATCH v4 2/5] public/io/netif.h: add definition of gso_prefix flag Paul Durrant
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Keir Fraser, Ian Campbell, Tim Deegan, Ian Jackson, Paul Durrant,
	Jan Beulich

The id field of the netif_rx_request_t abd netif_rx_response_t structures
is actually useless.

Because GSO metadata is passed from backend to frontend using
netif_extra_info segments, which do not carry information stating which
netif_rx_request_t was consumed to free up their slot, frontends assume
that the consumed request is the one that previously occupied that same
slot in the shared ring. Also, whilst theoretically possible for other
responses to be re-ordered, they never have been and that assumption has
always been baked into Linux xen-netfront.

Hence this patch documents that the request id and the response id must
be equal to the ring slot index modulo the ring size.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v4:
 - Add slightly more explanation in a comment
---
 xen/include/public/io/netif.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 5c31ae3..7ea9ea7 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -210,7 +210,7 @@
  * | id        | pad       | gref                  |
  * +-----+-----+-----+-----+-----+-----+-----+-----+
  *
- * id: request identifier, echoed in response.
+ * id: must be identical to ring slot index modulo ring size.
  * gref: reference to incoming granted frame.
  *
  * rx response (netif_rx_response_t)
@@ -221,11 +221,20 @@
  * | id        | offset    | flags     | status    |
  * +-----+-----+-----+-----+-----+-----+-----+-----+
  *
- * id: reflects id in receive request
+ * id: must be identical to request id (and hence, by implication, rx
+ *     responses must be located in the same ring slot as their
+ *     corresponding requests).
  * offset: offset in page of start of received packet
  * flags: NETRXF_*
  * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
  *
+ * NOTE: The reason that id must be identical to ring slot index
+ *       modulo ring size is because extra info segments (see below)
+ *       carry no indication of the netif_rx_request_t that was
+ *       consumed to make their slot available. The only way a
+ *       frontend can determine which netif_rx_request_t was consumed
+ *       is using the id -> slot identity relation.
+ *
  * Extra Info
  * ==========
  *
@@ -373,7 +382,7 @@ struct netif_tx_response {
 typedef struct netif_tx_response netif_tx_response_t;
 
 struct netif_rx_request {
-    uint16_t    id;        /* Echoed in response message.        */
+    uint16_t    id;
     uint16_t    pad;
     grant_ref_t gref;      /* Reference to incoming granted frame */
 };
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 2/5] public/io/netif.h: add definition of gso_prefix flag
  2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id Paul Durrant
@ 2015-10-19 13:53 ` Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 3/5] public/io/netif.h: add documentation for hash negotiation and mapping Paul Durrant
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Keir Fraser, Ian Campbell, Tim Deegan, Ian Jackson, Paul Durrant,
	Jan Beulich

This flag is defined here only for compatibility with the Linux variant of
this header. The feature has never been documented and should be
considered deprecated.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/io/netif.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 7ea9ea7..ffd11f9 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -404,6 +404,10 @@ typedef struct netif_rx_request netif_rx_request_t;
 #define _NETRXF_extra_info     (3)
 #define  NETRXF_extra_info     (1U<<_NETRXF_extra_info)
 
+/* Packet has GSO prefix. Deprecated but included for compatibility */
+#define _NETRXF_gso_prefix     (4)
+#define  NETRXF_gso_prefix     (1U<<_NETRXF_gso_prefix)
+
 struct netif_rx_response {
     uint16_t id;
     uint16_t offset;       /* Offset in page of start of received packet  */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 3/5] public/io/netif.h: add documentation for hash negotiation and mapping
  2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 2/5] public/io/netif.h: add definition of gso_prefix flag Paul Durrant
@ 2015-10-19 13:53 ` Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 4/5] public/io/netif.h: add extra info slots for passing hash values Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 5/5] public/io/netif.h: tidy up and remove duplicate comments Paul Durrant
  4 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Keir Fraser, Ian Campbell, Tim Deegan, Ian Jackson, Paul Durrant,
	Jan Beulich

This patch specifies the xenstore keys that should be used by frontends
and backends to negotiate a particular hash algorithm and queue mapping
to be used for mult-queue packet steering on the guest receive side.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v4:
 - Fix spelling of 'capabilities'
---
 xen/include/public/io/netif.h | 111 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 5 deletions(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index ffd11f9..7ea6a43 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -114,11 +114,112 @@
  * error. This includes scenarios where more (or fewer) queues were
  * requested than the frontend provided details for.
  *
- * Mapping of packets to queues is considered to be a function of the
- * transmitting system (backend or frontend) and is not negotiated
- * between the two. Guests are free to transmit packets on any queue
- * they choose, provided it has been set up correctly. Guests must be
- * prepared to receive packets on any queue they have requested be set up.
+ * Unless a hash algorithm or mapping of packet hash to queues has been
+ * negotiated (see below), queue selection is considered to be a function of
+ * the transmitting system (backend or frontend) and either end is free to
+ * transmit packets on any queue, provided it has been set up correctly.
+ * Guests must therefore be prepared to receive packets on any queue they
+ * have requested be set up.
+ */
+
+/*
+ * Hash negotiation (only applicable if using multiple queues):
+ *
+ * A backend can advertise a set of hash algorithms that it can perform by
+ * naming it in a space separated list in the "multi-queue-hash-list"
+ * xenstore key. For example, if the backend supports the 'foo' and 'bar'
+ * algorithms it would set:
+ *
+ * /local/domain/X/backend/vif/Y/Z/multi-queue-hash-list = "foo bar"
+ *
+ * Additionally, in supporting a particular algorithm, it may be necessary
+ * for the backend to specify the capabilities of its implementation of
+ * that algorithm, e.g. what sections of packet header it can hash.
+ * To do that it can set algorithm-specific keys under a parent capabilities
+ * key. For example, if the 'bar' algorithm implementation in the backend
+ * is capable of hashing over an IP version 4 header and a TCP header, the
+ * backend might set:
+ *
+ * /local/domain/X/backend/vif/Y/Z/multi-queue-hash-caps-bar/types = "ipv4+tcp"
+ *
+ * The backend should set all such keys before it moves into the initwait
+ * state.
+ *
+ * The frontend can select a hash algorithm at any time after it moves into
+ * the connected state by setting the "multi-queue-hash" key. The backend
+ * must therefore watch this key and be prepared to change hash algorithms
+ * at any time whilst in the connected state. So, for example, if the
+ * frontend wants 'foo' hashing, it should set:
+ *
+ * /local/domain/Y/device/vif/Z/multi-queue-hash = "foo"
+ *
+ * Additionally it may set parameters for that algorithm by setting
+ * algorithm-specific keys under a parent parameters key. For example, if
+ * the 'foo' algorithm implementation in the backend is capable of hashing
+ * over an IP version 4 header, a TCP header or both but the frontend only
+ * wants it to hash over only the IP version 4 header then it might set:
+ *
+ * /local/domain/Y/device/vif/Z/multi-queue-hash-params-foo/types = "ipv4"
+ *
+ * The backend must also watch the parameters key as the frontend may
+ * change the parameters at any time whilst in the connected state.
+ *
+ * (Capabilities and parameters documentation for specific algorithms is
+ * below).
+ *
+ * TOEPLITZ:
+ *
+ * If the backend supports Toeplitz hashing then it should include
+ * the algorithm name 'toeplitz' in its "multi-queue-hash-list" key.
+ * It should also advertise the following capabilities:
+ *
+ * types: a space separated list containing any or all of 'ipv4', 'tcpv4',
+ *        'ipv6', 'tcpv6', indicating over which headers the hash algorithm
+ *        is capable of being performed.
+ *
+ * max-key-length: an integer value indicating the maximum key length (in
+ *                 octets) that the frontend may supply.
+ *
+ * Upon selecting this algorithm, the frontend may supply the following
+ * parameters.
+ *
+ * types: a space separated list containing none, any or all of the type
+ *        names included in the types list in the capabilities.
+ *        When the backend encounters a packet type not in this list it
+ *        will assign a hash value of 0.
+ *
+ * key: a ':' separated list of octets (up to the maximum length specified
+ *      in the capabilities) expressed in hexadecimal indicating the key
+ *      that should be used in the hash calculation.
+ *
+ * For more information on Toeplitz hash calculation see:
+ *
+ * https://msdn.microsoft.com/en-us/library/windows/hardware/ff570725.aspx
+ */
+
+/*
+ * Hash mapping (only applicable if using multiple queues):
+ *
+ * If the backend is not capable, or no mapping is specified by the frontend
+ * then it is assumed that the hash -> queue mapping is done by simple
+ * modular arithmetic.
+ *
+ * To advertise that it is capable of accepting a specific mapping from the
+ * frontend the backend should set the "multi-queue-max-hash-mapping-length"
+ * key to a non-zero value. The frontend may then specify a mapping (up to
+ * the maximum specified length) as a ',' separated list of decimal queue
+ * numbers in the "multi-queue-hash-mapping" key.
+ *
+ * The backend should parse this list into an array and perform the mapping
+ * as follows:
+ *
+ * queue = mapping[hash % length-of-list]
+ *
+ * If any of the queue values specified in the list is not connected then
+ * the backend is free to choose a connected queue arbitrarily.
+ *
+ * The backend must be prepared to handle updates the mapping list at any
+ * time whilst in the connected state.
  */
 
 /*
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 4/5] public/io/netif.h: add extra info slots for passing hash values
  2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
                   ` (2 preceding siblings ...)
  2015-10-19 13:53 ` [PATCH v4 3/5] public/io/netif.h: add documentation for hash negotiation and mapping Paul Durrant
@ 2015-10-19 13:53 ` Paul Durrant
  2015-10-19 13:53 ` [PATCH v4 5/5] public/io/netif.h: tidy up and remove duplicate comments Paul Durrant
  4 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Keir Fraser, Ian Campbell, Tim Deegan, Ian Jackson, Paul Durrant,
	Jan Beulich

To properly support NDIS RSS, the Windows frontend PV driver needs the
Toeplitz hash value calculated by the backend (otherwise it would have to
duplicate the calculation).

This patch adds documentation for "feature-hash" and a definition of a
new XEN_NETIF_EXTRA_TYPE_HASH extra info segment.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v4:
 - Fix typo
---
 xen/include/public/io/netif.h | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 7ea6a43..4fe9f25 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -252,6 +252,12 @@
  */
 
 /*
+ * "feature-hash" advertises the capability to accept extra info slots of
+ * type XEN_NETIF_EXTRA_TYPE_HASH. They will not be sent by either end
+ * unless the other end advertises this feature.
+ */
+
+/*
  * This is the 'wire' format for packets:
  *  Request 1: netif_tx_request_t -- NETTXF_* (any flags)
  * [Request 2: netif_extra_info_t] (only if request 1 has NETTXF_extra_info)
@@ -385,6 +391,18 @@
  * type: Must be XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}
  * flags: XEN_NETIF_EXTRA_FLAG_*
  * addr: address to add/remove
+ *
+ * XEN_NETIF_EXTRA_TYPE_HASH:
+ *
+ *    0     1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |type |flags|htype| pad |LSB ---- value ---- MSB|
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * type: Must be XEN_NETIF_EXTRA_TYPE_HASH
+ * flags: XEN_NETIF_EXTRA_FLAG_*
+ * htype: XEN_NETIF_HASH_TYPE_*
+ * value: Hash value
  */
 
 /* Protocol checksum field is blank in the packet (hardware offload)? */
@@ -414,11 +432,12 @@ struct netif_tx_request {
 typedef struct netif_tx_request netif_tx_request_t;
 
 /* Types of netif_extra_info descriptors. */
-#define XEN_NETIF_EXTRA_TYPE_NONE      (0)  /* Never used - invalid */
-#define XEN_NETIF_EXTRA_TYPE_GSO       (1)  /* u.gso */
-#define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
-#define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
-#define XEN_NETIF_EXTRA_TYPE_MAX       (4)
+#define XEN_NETIF_EXTRA_TYPE_NONE       (0) /* Never used - invalid */
+#define XEN_NETIF_EXTRA_TYPE_GSO        (1) /* u.gso */
+#define XEN_NETIF_EXTRA_TYPE_MCAST_ADD  (2) /* u.mcast */
+#define XEN_NETIF_EXTRA_TYPE_MCAST_DEL  (3) /* u.mcast */
+#define XEN_NETIF_EXTRA_TYPE_HASH       (4) /* u.hash */
+#define XEN_NETIF_EXTRA_TYPE_MAX        (5)
 
 /* netif_extra_info_t flags. */
 #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
@@ -429,6 +448,13 @@ typedef struct netif_tx_request netif_tx_request_t;
 #define XEN_NETIF_GSO_TYPE_TCPV4        (1)
 #define XEN_NETIF_GSO_TYPE_TCPV6        (2)
 
+/* Hash types */
+#define XEN_NETIF_HASH_TYPE_NONE        (0)
+#define XEN_NETIF_HASH_TYPE_TCPV4       (1)
+#define XEN_NETIF_HASH_TYPE_IPV4        (2)
+#define XEN_NETIF_HASH_TYPE_TCPV6       (3)
+#define XEN_NETIF_HASH_TYPE_IPV6        (4)
+
 /*
  * This structure needs to fit within both netif_tx_request_t and
  * netif_rx_response_t for compatibility.
@@ -471,6 +497,12 @@ struct netif_extra_info {
             uint8_t addr[6]; /* Address to add/remove. */
         } mcast;
 
+        struct {
+            uint8_t type;
+            uint8_t pad;
+            uint8_t value[4];
+        } hash;
+
         uint16_t pad[3];
     } u;
 };
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v4 5/5] public/io/netif.h: tidy up and remove duplicate comments
  2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
                   ` (3 preceding siblings ...)
  2015-10-19 13:53 ` [PATCH v4 4/5] public/io/netif.h: add extra info slots for passing hash values Paul Durrant
@ 2015-10-19 13:53 ` Paul Durrant
  4 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-19 13:53 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

Now that requests and response types and extra info segments are
documented in block comments, we can get rid of the inline comments
in the structures. This has the happy side-effect of making the Linux
checkpatch.pl script make fewer complaints after import.

This patch also fixes a small whitespace issue in the initial boiler-
plate comment.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 xen/include/public/io/netif.h | 81 +++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 49 deletions(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 4fe9f25..01d9549 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -1,8 +1,8 @@
 /******************************************************************************
  * netif.h
- * 
+ *
  * Unified network-device I/O interface for Xen guest OSes.
- * 
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to
  * deal in the Software without restriction, including without limitation the
@@ -260,8 +260,10 @@
 /*
  * This is the 'wire' format for packets:
  *  Request 1: netif_tx_request_t -- NETTXF_* (any flags)
- * [Request 2: netif_extra_info_t] (only if request 1 has NETTXF_extra_info)
- * [Request 3: netif_extra_info_t] (only if request 2 has XEN_NETIF_EXTRA_MORE)
+ * [Request 2: netif_extra_info_t] (only if request 1 has
+ *                                  NETTXF_extra_info)
+ * [Request 3: netif_extra_info_t] (only if request 2 has
+ *                                  XEN_NETIF_EXTRA_MORE)
  *  Request 4: netif_tx_request_t -- NETTXF_more_data
  *  Request 5: netif_tx_request_t -- NETTXF_more_data
  *  ...
@@ -358,7 +360,7 @@
  *
  *    0     1     2     3     4     5     6     7  octet
  * +-----+-----+-----+-----+-----+-----+-----+-----+
- * |type |flags| type specfic data                 |
+ * |type |flags| type specific data                |
  * +-----+-----+-----+-----+-----+-----+-----+-----+
  * | padding for tx        |
  * +-----+-----+-----+-----+
@@ -366,7 +368,8 @@
  * type: XEN_NETIF_EXTRA_TYPE_*
  * flags: XEN_NETIF_EXTRA_FLAG_*
  * padding for tx: present only in the tx case due to 8 octet limit
- *     from rx case. Not shown in type specific entries below.
+ *                 from rx case. Not shown in type specific entries
+ *                 below.
  *
  * XEN_NETIF_EXTRA_TYPE_GSO:
  *
@@ -377,9 +380,14 @@
  *
  * type: Must be XEN_NETIF_EXTRA_TYPE_GSO
  * flags: XEN_NETIF_EXTRA_FLAG_*
- * size: Maximum payload size of each segment.
- * type: XEN_NETIF_GSO_TYPE_*
- * features: EN_NETIF_GSO_FEAT_*
+ * size: Maximum payload size of each segment. For example,
+ *       for TCP this is just the path MSS.
+ * type: XEN_NETIF_GSO_TYPE_*: This determines the protocol of
+ *       the packet and any extra features required to segment the
+ *       packet properly.
+ * features: EN_NETIF_GSO_FEAT_*: This specifies any extra GSO
+ *           features required to process this packet, such as ECN
+ *           support for TCPv4.
  *
  * XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}:
  *
@@ -423,11 +431,11 @@
 
 #define XEN_NETIF_MAX_TX_SIZE 0xFFFF
 struct netif_tx_request {
-    grant_ref_t gref;      /* Reference to buffer page */
-    uint16_t offset;       /* Offset within buffer page */
-    uint16_t flags;        /* NETTXF_* */
-    uint16_t id;           /* Echoed in response message. */
-    uint16_t size;         /* Packet size in bytes.       */
+    grant_ref_t gref;
+    uint16_t offset;
+    uint16_t flags;
+    uint16_t id;
+    uint16_t size;
 };
 typedef struct netif_tx_request netif_tx_request_t;
 
@@ -460,43 +468,18 @@ typedef struct netif_tx_request netif_tx_request_t;
  * netif_rx_response_t for compatibility.
  */
 struct netif_extra_info {
-    uint8_t type;  /* XEN_NETIF_EXTRA_TYPE_* */
-    uint8_t flags; /* XEN_NETIF_EXTRA_FLAG_* */
-
+    uint8_t type;
+    uint8_t flags;
     union {
-        /*
-         * XEN_NETIF_EXTRA_TYPE_GSO:
-         */
         struct {
-            /*
-             * Maximum payload size of each segment. For example, for TCP this
-             * is just the path MSS.
-             */
             uint16_t size;
-
-            /*
-             * GSO type. This determines the protocol of the packet and any
-             * extra features required to segment the packet properly.
-             */
-            uint8_t type; /* XEN_NETIF_GSO_TYPE_* */
-
-            /* Future expansion. */
+            uint8_t type;
             uint8_t pad;
-
-            /*
-             * GSO features. This specifies any extra GSO features required
-             * to process this packet, such as ECN support for TCPv4.
-             */
-            uint16_t features; /* XEN_NETIF_GSO_FEAT_* */
+            uint16_t features;
         } gso;
-
-        /*
-         * XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}:
-         */
         struct {
-            uint8_t addr[6]; /* Address to add/remove. */
+            uint8_t addr[6];
         } mcast;
-
         struct {
             uint8_t type;
             uint8_t pad;
@@ -510,14 +493,14 @@ typedef struct netif_extra_info netif_extra_info_t;
 
 struct netif_tx_response {
     uint16_t id;
-    int16_t  status;       /* NETIF_RSP_* */
+    int16_t  status;
 };
 typedef struct netif_tx_response netif_tx_response_t;
 
 struct netif_rx_request {
     uint16_t    id;
     uint16_t    pad;
-    grant_ref_t gref;      /* Reference to incoming granted frame */
+    grant_ref_t gref;
 };
 typedef struct netif_rx_request netif_rx_request_t;
 
@@ -543,9 +526,9 @@ typedef struct netif_rx_request netif_rx_request_t;
 
 struct netif_rx_response {
     uint16_t id;
-    uint16_t offset;       /* Offset in page of start of received packet  */
-    uint16_t flags;        /* NETRXF_* */
-    int16_t  status;       /* -ve: NETIF_RSP_* ; +ve: Rx'ed pkt size. */
+    uint16_t offset;
+    uint16_t flags;
+    int16_t  status;
 };
 typedef struct netif_rx_response netif_rx_response_t;
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id
  2015-10-19 13:53 ` [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id Paul Durrant
@ 2015-10-19 16:00   ` Jan Beulich
  2015-10-20  9:34     ` Paul Durrant
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2015-10-19 16:00 UTC (permalink / raw)
  To: Paul Durrant
  Cc: xen-devel, Keir Fraser, Ian Jackson, Ian Campbell, Tim Deegan

>>> On 19.10.15 at 15:53, <paul.durrant@citrix.com> wrote:
> The id field of the netif_rx_request_t abd netif_rx_response_t structures
> is actually useless.
> 
> Because GSO metadata is passed from backend to frontend using
> netif_extra_info segments, which do not carry information stating which
> netif_rx_request_t was consumed to free up their slot, frontends assume
> that the consumed request is the one that previously occupied that same
> slot in the shared ring. Also, whilst theoretically possible for other
> responses to be re-ordered, they never have been and that assumption has
> always been baked into Linux xen-netfront.

Is all this true even for frontends not supporting GSO and other
"extra" stuff?

Jan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id
  2015-10-19 16:00   ` Jan Beulich
@ 2015-10-20  9:34     ` Paul Durrant
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-20  9:34 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Ian Jackson, Tim (Xen.org), Keir (Xen.org), Ian Campbell, xen-devel

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 October 2015 17:01
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; xen-devel@lists.xenproject.org; Keir
> (Xen.org); Tim (Xen.org)
> Subject: Re: [PATCH v4 1/5] public/io/netif.h: document the reality of
> netif_rx_request/reponse id
> 
> >>> On 19.10.15 at 15:53, <paul.durrant@citrix.com> wrote:
> > The id field of the netif_rx_request_t abd netif_rx_response_t structures
> > is actually useless.
> >
> > Because GSO metadata is passed from backend to frontend using
> > netif_extra_info segments, which do not carry information stating which
> > netif_rx_request_t was consumed to free up their slot, frontends assume
> > that the consumed request is the one that previously occupied that same
> > slot in the shared ring. Also, whilst theoretically possible for other
> > responses to be re-ordered, they never have been and that assumption
> has
> > always been baked into Linux xen-netfront.
> 
> Is all this true even for frontends not supporting GSO and other
> "extra" stuff?
> 

Maybe, but backends need to be compatible with Linux netfront and regardless of whether it has enabled GSO on the receive side, it makes no use of response id. However, looking again, maybe the restriction can be relaxed a little.

When netfront posts an rx request it sets the request id to ring slot modulo ring size but, when processing responses, it simply looks up the context it saved (e.g. grant ref) based on request id by assuming the identity relation between slot number and id.
So, because it does not verify the identity relation, it is sufficient to simply make sure that responses appear in the same ring slot as their corresponding request.
So, I think it is sufficient to say response id must match request id (for those frontends, like the Windows  one, that do use it) and responses must appear in the same slot as requests (to keep compatibility with Linux netfront). I'll adjust the patch accordingly.

  Paul

> Jan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 0/4] Updates to public/io/netif.h
@ 2015-10-20 12:35 Paul Durrant
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Durrant @ 2015-10-20 12:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

This series makes several modifications to netif.h in anticipation of
implementing NDIS RSS support in the Windows frontend driver.

Patch #1 documents the (sad) reality of the netif_rx_request/response
protocol, which has been long overdue.

Patch #2 adds a definition of the NETRXF_gso_prefix flag which has been
present in the Linux variant of the header for several years

Patch #3 adds documentation for how negotiation of a hash algortithm to
be used on the frontend receive side should be done.

Patch #4 adds a new netif_extra_info type for passing hash values between
backend and frontend (or vice versa).

Patch #5 (new in v4) reduces comment duplication and fixes formatting.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-10-20 12:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-19 13:53 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant
2015-10-19 13:53 ` [PATCH v4 1/5] public/io/netif.h: document the reality of netif_rx_request/reponse id Paul Durrant
2015-10-19 16:00   ` Jan Beulich
2015-10-20  9:34     ` Paul Durrant
2015-10-19 13:53 ` [PATCH v4 2/5] public/io/netif.h: add definition of gso_prefix flag Paul Durrant
2015-10-19 13:53 ` [PATCH v4 3/5] public/io/netif.h: add documentation for hash negotiation and mapping Paul Durrant
2015-10-19 13:53 ` [PATCH v4 4/5] public/io/netif.h: add extra info slots for passing hash values Paul Durrant
2015-10-19 13:53 ` [PATCH v4 5/5] public/io/netif.h: tidy up and remove duplicate comments Paul Durrant
2015-10-20 12:35 [PATCH v4 0/4] Updates to public/io/netif.h Paul Durrant

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.