ofp-util: Implement OpenFlow 1.1 packet-in message.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Reviewed-by: Simon Horman <horms@verge.net.au>

diff --git a/DESIGN b/DESIGN
index d36b025..4d654d0 100644 (file)
--- a/DESIGN
+++ b/DESIGN
@@ -266,6 +266,53 @@ OpenFlow 1.3 makes these changes:
 The table for 1.3 is the same as the one shown above for 1.2.
 
 
 The table for 1.3 is the same as the one shown above for 1.2.
 
 

+OFPT_PACKET_IN
+==============
+
+The OpenFlow 1.1 specification for OFPT_PACKET_IN is confusing.  The
+definition in OF1.1 openflow.h is[*]:
+
+  /* Packet received on port (datapath -> controller). */
+  struct ofp_packet_in {
+      struct ofp_header header;
+      uint32_t buffer_id;     /* ID assigned by datapath. */
+      uint32_t in_port;       /* Port on which frame was received. */
+      uint32_t in_phy_port;   /* Physical Port on which frame was received. */
+      uint16_t total_len;     /* Full length of frame. */
+      uint8_t reason;         /* Reason packet is being sent (one of OFPR_*) */
+      uint8_t table_id;       /* ID of the table that was looked up */
+      uint8_t data[0];        /* Ethernet frame, halfway through 32-bit word,
+                                 so the IP header is 32-bit aligned.  The
+                                 amount of data is inferred from the length
+                                 field in the header.  Because of padding,
+                                 offsetof(struct ofp_packet_in, data) ==
+                                 sizeof(struct ofp_packet_in) - 2. */
+  };
+  OFP_ASSERT(sizeof(struct ofp_packet_in) == 24);
+
+The confusing part is the comment on the data[] member.  This comment
+is a leftover from OF1.0 openflow.h, in which the comment was correct:
+sizeof(struct ofp_packet_in) is 20 in OF1.0 and offsetof(struct
+ofp_packet_in, data) is 18.  When OF1.1 was written, the structure
+members were changed but the comment was carelessly not updated, and
+the comment became wrong: sizeof(struct ofp_packet_in) and
+offsetof(struct ofp_packet_in, data) are both 24 in OF1.1.
+
+That leaves the question of how to implement ofp_packet_in in OF1.1.
+The OpenFlow reference implementation for OF1.1 does not include any
+padding, that is, the first byte of the encapsulated frame immediately
+follows the 'table_id' member without a gap.  Open vSwitch therefore
+implements it the same way for compatibility.
+
+For an earlier discussion, please see the thread archived at:
+https://mailman.stanford.edu/pipermail/openflow-discuss/2011-August/002604.html
+
+[*] The quoted definition is directly from OF1.1.  Definitions used
+    inside OVS omit the 8-byte ofp_header members, so the sizes in
+    this discussion are 8 bytes larger than those declared in OVS
+    header files.
+
+

 VLAN Matching
 =============
 
 VLAN Matching
 =============
 

diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index 8684d5e..eaf2ee9 100644 (file)
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -54,13 +54,6 @@ OpenFlow 1.1
 The list of remaining work items for OpenFlow 1.1 is below.  It is
 probably incomplete.
 
 The list of remaining work items for OpenFlow 1.1 is below.  It is
 probably incomplete.
 

-    * The new in_phy_port field in OFPT_PACKET_IN needs some kind of
-      implementation.  It has a sensible interpretation for tunnels
-      but in general the physical port is not in the datapath for OVS
-      so the value is not necessarily meaningful.  We might have to
-      just fix it as the same as in_port.
-      [required for OF1.1; optional for OF1.2+]
-

     * OFPT_TABLE_MOD message.  This is new in OF1.1, so we need to
       implement it.  It should be implemented so that the default OVS
       behavior does not change.
     * OFPT_TABLE_MOD message.  This is new in OF1.1, so we need to
       implement it.  It should be implemented so that the default OVS
       behavior does not change.

diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index 9015109..92dd3c5 100644 (file)
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -741,12 +741,7 @@ struct ofp11_packet_in {
     ovs_be16 total_len;     /* Full length of frame. */
     uint8_t reason;         /* Reason packet is being sent (one of OFPR_*) */
     uint8_t table_id;       /* ID of the table that was looked up */
     ovs_be16 total_len;     /* Full length of frame. */
     uint8_t reason;         /* Reason packet is being sent (one of OFPR_*) */
     uint8_t table_id;       /* ID of the table that was looked up */

-    /* uint8_t data[0];        Ethernet frame, halfway through 32-bit word,
-                               so the IP header is 32-bit aligned. The
-                               amount of data is inferred from the length
-                               field in the header. Because of padding,
-                               offsetof(struct ofp_packet_in, data) ==
-                               sizeof(struct ofp_packet_in) - 2. */
+    /* Followed by Ethernet frame. */

 };
 OFP_ASSERT(sizeof(struct ofp11_packet_in) == 16);
 
 };
 OFP_ASSERT(sizeof(struct ofp11_packet_in) == 16);
 

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index b4723b8..7fc4c7c 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -3196,6 +3196,23 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
         pin->reason = opi->reason;
         pin->buffer_id = ntohl(opi->buffer_id);
         pin->total_len = ntohs(opi->total_len);
         pin->reason = opi->reason;
         pin->buffer_id = ntohl(opi->buffer_id);
         pin->total_len = ntohs(opi->total_len);

+    } else if (raw == OFPRAW_OFPT11_PACKET_IN) {
+        const struct ofp11_packet_in *opi;
+        enum ofperr error;
+
+        opi = ofpbuf_pull(&b, sizeof *opi);
+
+        pin->packet = b.data;
+        pin->packet_len = b.size;
+
+        pin->buffer_id = ntohl(opi->buffer_id);
+        error = ofputil_port_from_ofp11(opi->in_port, &pin->fmd.in_port);
+        if (error) {
+            return error;
+        }
+        pin->total_len = ntohs(opi->total_len);
+        pin->reason = opi->reason;
+        pin->table_id = opi->table_id;

     } else if (raw == OFPRAW_NXT_PACKET_IN) {
         const struct nx_packet_in *npi;
         struct match match;
     } else if (raw == OFPRAW_NXT_PACKET_IN) {
         const struct nx_packet_in *npi;
         struct match match;


@@ -3309,6 +3326,27 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin)
     return packet;
 }
 
     return packet;
 }
 

+static struct ofpbuf *
+ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
+{
+    struct ofp11_packet_in *opi;
+    struct ofpbuf *packet;
+
+    packet = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
+                              htonl(0), pin->packet_len);
+    opi = ofpbuf_put_zeros(packet, sizeof *opi);
+    opi->buffer_id = htonl(pin->buffer_id);
+    opi->in_port = ofputil_port_to_ofp11(pin->fmd.in_port);
+    opi->in_phy_port = opi->in_port;
+    opi->total_len = htons(pin->total_len);
+    opi->reason = pin->reason;
+    opi->table_id = pin->table_id;
+
+    ofpbuf_put(packet, pin->packet, pin->packet_len);
+
+    return packet;
+}
+

 static struct ofpbuf *
 ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
                                enum ofputil_protocol protocol)
 static struct ofpbuf *
 ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
                                enum ofputil_protocol protocol)


@@ -3373,7 +3411,8 @@ ofputil_encode_packet_in(const struct ofputil_packet_in *pin,
         break;
 
     case OFPUTIL_P_OF11_STD:
         break;
 
     case OFPUTIL_P_OF11_STD:

-        NOT_REACHED();
+        packet = ofputil_encode_ofp11_packet_in(pin);
+        break;

 
     case OFPUTIL_P_OF12_OXM:
     case OFPUTIL_P_OF13_OXM:
 
     case OFPUTIL_P_OF12_OXM:
     case OFPUTIL_P_OF13_OXM:

diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 82e8c3d..2f09c1b 100644 (file)
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -469,6 +469,21 @@ tcp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:0
 ])
 AT_CLEANUP
 
 ])
 AT_CLEANUP
 

+AT_SETUP([OFPT_PACKET_IN - OF1.1])
+AT_KEYWORDS([ofp-print])
+AT_CHECK([ovs-ofctl ofp-print "\
+02 0a 00 54 00 00 00 00 00 00 01 11 00 00 00 03 \
+00 00 00 03 00 3c 00 00 \
+50 54 00 00 00 06 50 54 00 00 00 05 08 00 \
+45 00 00 28 bd 12 00 00 40 06 3c 6a c0 a8 00 01 \
+c0 a8 00 02 27 2f 00 00 78 50 cc 5b 57 af 42 1e \
+50 02 02 00 26 e8 00 00 00 00 00 00 00 00 \
+"], [0], [dnl
+OFPT_PACKET_IN (OF1.1) (xid=0x0): total_len=60 in_port=3 (via no_match) data_len=60 buffer=0x00000111
+tcp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=10031,tp_dst=0,tcp_flags=0x002 tcp_csum:26e8
+])
+AT_CLEANUP
+

 AT_SETUP([OFPT_PACKET_IN - OF1.2])
 AT_KEYWORDS([ofp-print])
 AT_CHECK([ovs-ofctl ofp-print "\
 AT_SETUP([OFPT_PACKET_IN - OF1.2])
 AT_KEYWORDS([ofp-print])
 AT_CHECK([ovs-ofctl ofp-print "\

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 27b6b34..be7387d 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -1757,6 +1757,39 @@ OFPT_BARRIER_REPLY (OF1.2):
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 

+dnl This test checks that OFPT_PACKET_OUT accepts both OFPP_NONE (as
+dnl specified by OpenFlow 1.1) and OFPP_CONTROLLER (used by some
+dnl controllers despite the spec) as meaning a packet that was generated
+dnl by the controller.
+AT_SETUP([ofproto - packet-out from controller (OpenFlow 1.1)])
+OVS_VSWITCHD_START
+
+# Start a monitor listening for packet-ins.
+AT_CHECK([ovs-ofctl -O OpenFlow11 monitor br0 --detach --no-chdir --pidfile])
+ovs-appctl -t ovs-ofctl ofctl/send 0209000c0123456700000080
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
+AT_CAPTURE_FILE([monitor.log])
+
+# Send some packet-outs with OFPP_NONE and OFPP_CONTROLLER (65533) as in_port.
+AT_CHECK([ovs-ofctl -O OpenFlow11 packet-out br0 none controller '0001020304050010203040501234'])
+AT_CHECK([ovs-ofctl -O OpenFlow11 packet-out br0 4294967293 controller '0001020304050010203040505678'])
+
+# Stop the monitor and check its output.
+ovs-appctl -t ovs-ofctl ofctl/barrier
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
+OFPT_PACKET_IN (OF1.1): total_len=14 in_port=ANY (via action) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
+OFPT_PACKET_IN (OF1.1): total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
+metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x5678
+OFPT_BARRIER_REPLY (OF1.1):
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+

 dnl This test checks that metadata is encoded in packet_in structures,
 dnl supported by NXAST.
 AT_SETUP([ofproto - packet-out with metadata (NXM)])
 dnl This test checks that metadata is encoded in packet_in structures,
 dnl supported by NXAST.
 AT_SETUP([ofproto - packet-out with metadata (NXM)])