[PATCH v6 0/3] updates to public/io/netif.h

[PATCH v6 0/3] updates to public/io/netif.h

[Paul Durrant]

v5 of this series included updates to support NDIS RSS for Windows
frontends. Since review concluded that some more fundamental changes
were needed in the protocol, v6 is just the surrounding cleanup patches
which are still relevant.
Another series will be posted for RSS related infrastructure.

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 reduces comment duplication and fixes formatting.

[PATCH v6 1/3] public/io/netif.h: document the reality of netif_rx_request/reponse

[Paul Durrant]

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 must
assume some form of identity relation between ring slot and request.
Hence, so that it is able to use GSO metadata, Linux netfront simply
assumes rx responses appear in the same ring slot as their corresponding
request.

This patch documents the assumption made by Linux netfront and the
necessity of the assumption (to support GSO) so that backends are coded
to be compatible.

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

v5:
 - Re-word the comments since the restriction is not actually on
   the rx request/response id field, but the slots used by requests
   and responses.
---
 xen/include/public/io/netif.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 5c31ae3..04d8026 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -226,15 +226,29 @@
  * flags: NETRXF_*
  * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
  *
+ * NOTE: Historically, to support GSO on the frontend receive side, Linux
+ *       netfront does not make use of the rx response id (because, as
+ *       described below, extra info structures overlay the id field).
+ *       Instead it assumes that responses always appear in the same ring
+ *       slot as their corresponding request. Thus, to maintain
+ *       compatibilty, backends must make sure this is the case.
+ *
  * Extra Info
  * ==========
  *
- * Can be present if initial request has NET{T,R}XF_extra_info, or
- * previous extra request has XEN_NETIF_EXTRA_MORE.
+ * Can be present if initial request or response has NET{T,R}XF_extra_info,
+ * or previous extra request has XEN_NETIF_EXTRA_MORE.
  *
  * The struct therefore needs to fit into either a tx or rx slot and
  * is therefore limited to 8 octets.
  *
+ * NOTE: Because extra info data overlays the usual request/response
+ *       structures, there is no id information in the opposite direction.
+ *       So, if an extra info overlays an rx response the frontend can
+ *       assume that it is in the same ring slot as the request that was
+ *       consumed to make the slot available, and the backend must ensure
+ *       this assumption is true.
+ *
  * extra info (netif_extra_info_t)
  * -------------------------------
  *
-- 
2.1.4

[PATCH v6 2/3] public/io/netif.h: add definition of gso_prefix flag

[Paul Durrant]

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 04d8026..f62a0c8 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -409,6 +409,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

[PATCH v6 3/3] public/io/netif.h: tidy up and remove duplicate comments

[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, and a typo in one of the ascii-art diagrams.

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 | 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 f62a0c8..e103cf3 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
@@ -153,8 +153,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
  *  ...
@@ -256,7 +258,7 @@
  *
  *    0     1     2     3     4     5     6     7  octet
  * +-----+-----+-----+-----+-----+-----+-----+-----+
- * |type |flags| type specfic data                 |
+ * |type |flags| type specific data                |
  * +-----+-----+-----+-----+-----+-----+-----+-----+
  * | padding for tx        |
  * +-----+-----+-----+-----+
@@ -264,7 +266,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:
  *
@@ -275,9 +278,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}:
  *
@@ -309,11 +317,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;
 
@@ -338,43 +346,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;
-
         uint16_t pad[3];
     } u;
 };
@@ -382,14 +365,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;        /* Echoed in response message.        */
     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;
 
@@ -415,9 +398,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

Re: [PATCH v6 0/3] updates to public/io/netif.h

[Ian Campbell]

On Mon, 2015-11-16 at 15:43 +0000, Paul Durrant wrote:
> v5 of this series included updates to support NDIS RSS for Windows
> frontends. Since review concluded that some more fundamental changes
> were needed in the protocol, v6 is just the surrounding cleanup patches
> which are still relevant.
> Another series will be posted for RSS related infrastructure.

LGTM: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.