Ben Pfaff <b...@ovn.org> writes:
> On Thu, Dec 05, 2019 at 12:43:31PM -0800, William Tu wrote:
>> GTP, GPRS Tunneling Protocol, is a group of IP-based communications
>> protocols used to carry general packet radio service (GPRS) within
>> GSM, UMTS and LTE networks. GTP protocol has two parts: Signalling
>> (GTP-Control, GTP-C) and User data (GTP-User, GTP-U). GTP-C is used
>> for setting up GTP-U protocol, which is an IP-in-UDP tunneling
>> protocol. Usually GTP is used in connecting between base station for
>> radio, Serving Gateway (S-GW), and PDN Gateway (P-GW).
>>
>> This patch implements GTP-U protocol for userspace datapath,
>> supporting only required header fields and G-PDU message type.
>> See spec in:
>> https://tools.ietf.org/html/draft-hmm-dmm-5g-uplane-analysis-00
>>
>> Signed-off-by: Yi Yang <yangy...@inspur.com>
>> Co-authored-by: Yi Yang <yangy...@inspur.com>
>> Signed-off-by: William Tu <u9012...@gmail.com>
>
> Thanks a lot! I made another pass and noticed a few more things.
>
> For some "flags" fields, where it's important to understand the flags,
> we break out the flags for user comprehension. For example, we do this
> with TCP flags. Do you have an idea whether the GTP-U flags are
> important enough for this? If so, then we'd need some additional
> changes for parsing and formatting GTP-U flags. If not, I suggest
> changing formatting for them in meta-flow.h from "decimal" to
> "hexadecimal" because it's easier to understand bit-mappings from hex to
> binary in your head (my suggested patch does that).
CC'd Fouad for this part, too.
My understanding is that the GTP-U flags don't contain session state
information, but are really per-pdu 'special' settings. For example,
there's no such idea as 'control' with gtp-u (that's all done via gtp-c)
so there isn't much to select on. Some of the well-known extension
headers might have value as selectors, though (I'm thinking the RAN
container ID extensions, maybe the UDP Port for error path).
I agree with the hex data change.
> I think that flow_get_metadata() should copy the new fields.
>
> These new fields are marked writable. Is it actually safe to let the
> user change the flags field? It seems to me that all of the bits here
> are either fixed or indicate whether some following field is present, so
> that changing it could break the interpretation of the packet. Maybe
> the flags should be read-only.
I can make an argument for the extension bit to be writable only if
there's a corresponding method to push or pop extensions. I'm not sure
which extensions the SDN should ever push in (maybe UDP Port for error
path) vs. the actual RAN application.
> I think that format_flow_tunnel() in lib/match.c should format the new
> fields.
>
> The use of "? :" as in netdev_gtpu_build_header() is a GCC extension. I
> think that MSVC will reject it.
>
> Doesn't tun_key_to_attr() need to support the new fields?
>
> I'm pretty suspicious of the treatment of PT_GTPU_MSG packets. I don't
> know what can be done with them. I bet they have not been tested.
>
> I'm appending a few more minor suggestions.
>
> Thanks again!
>
> Ben
>
> -8<--------------------------cut here-------------------------->8--
>
> diff --git a/Documentation/faq/configuration.rst
> b/Documentation/faq/configuration.rst
> index a9ac9354d0dc..4a98740c5d4d 100644
> --- a/Documentation/faq/configuration.rst
> +++ b/Documentation/faq/configuration.rst
> @@ -227,9 +227,9 @@ Q: Does Open vSwitch support IPv6 GRE?
>
> Q: Does Open vSwitch support GTP-U?
>
> - A: Yes. GTP-U (GPRS Tunnelling Protocol User Plane (GTPv1-U))
> - is supported in userspace datapath. TEID is set by using tunnel
> - key field.
> + A: Yes. Starting with version 2.13, the Open vSwitch userspace
> + datapath supports GTP-U (GPRS Tunnelling Protocol User Plane
> + (GTPv1-U)). TEID is set by using tunnel key field.
>
> ::
>
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index d57a36468e19..b3457f231f9a 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -512,7 +512,7 @@ enum OVS_PACKED_ENUM mf_field_id {
> *
> * Type: u8.
> * Maskable: bitwise.
> - * Formatting: decimal.
> + * Formatting: hexadecimal.
> * Prerequisites: none.
> * Access: read/write.
> * NXM: none.
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 7ae554c87964..bbce5e1f55cb 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -738,7 +738,7 @@ netdev_gtpu_pop_header(struct dp_packet *packet)
>
> tnl->gtpu_flags = gtph->md.flags;
> tnl->gtpu_msgtype = gtph->md.msgtype;
> - tnl->tun_id = htonll(ntohl(get_16aligned_be32(>ph->teid)));
> + tnl->tun_id = be32_to_be64(get_16aligned_be32(>ph->teid));
>
> if (tnl->gtpu_msgtype == GTPU_MSGTYPE_GPDU) {
> struct ip_header *ip;
> diff --git a/lib/packets.h b/lib/packets.h
> index 6a4d3fb87392..405ea9325c21 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1498,12 +1498,14 @@ struct gtpu_metadata {
> uint8_t flags;
> uint8_t msgtype;
> };
> +BUILD_ASSERT_DECL(sizeof(struct gtpu_metadata) == 2);
>
> struct gtpuhdr {
> struct gtpu_metadata md;
> ovs_be16 len;
> ovs_16aligned_be32 teid;
> };
> +BUILD_ASSERT_DECL(sizeof(struct gtpuhdr) == 8);
>
> /* VXLAN protocol header */
> struct vxlanhdr {
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
