From 4d197ebbf6b6e8b26bb836b39c53c4101a0f2e14 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 4 Dec 2013 08:17:50 -0800 Subject: [PATCH] ofp-util: Implement OpenFlow 1.1 packet-in message. Signed-off-by: Ben Pfaff Reviewed-by: Simon Horman --- DESIGN | 47 +++++++++++++++++++++++++++++++++ OPENFLOW-1.1+ | 7 ----- include/openflow/openflow-1.1.h | 7 +---- lib/ofp-util.c | 41 +++++++++++++++++++++++++++- tests/ofp-print.at | 15 +++++++++++ tests/ofproto.at | 33 +++++++++++++++++++++++ 6 files changed, 136 insertions(+), 14 deletions(-) diff --git a/DESIGN b/DESIGN index d36b02507..4d654d01d 100644 --- 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. +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 ============= diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+ index 8684d5e42..eaf2ee99e 100644 --- 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 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. diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h index 90151099a..92dd3c5e5 100644 --- 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 */ - /* 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); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index b4723b804..7fc4c7c4b 100644 --- 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); + } 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; @@ -3309,6 +3326,27 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin) 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) @@ -3373,7 +3411,8 @@ ofputil_encode_packet_in(const struct ofputil_packet_in *pin, 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: diff --git a/tests/ofp-print.at b/tests/ofp-print.at index 82e8c3d17..2f09c1b8b 100644 --- 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_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 "\ diff --git a/tests/ofproto.at b/tests/ofproto.at index 27b6b34c4..be7387d33 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -1757,6 +1757,39 @@ OFPT_BARRIER_REPLY (OF1.2): 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)]) -- 2.43.0