On Thu, Dec 05, 2019 at 08:53:35PM -0800, Ben Pfaff wrote:
> 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>
>
Hi Ben,
Thanks for the review.
> 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).
agree. hexadecimal makes more sense.
>
> I think that flow_get_metadata() should copy the new fields.
Yes, now I understand FLOW_WC_SEQ is to remind you to add code there.
>
> 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.
Yes, make sense.
I think both fields tun_gtpu_flags,tun_gtpu_msgtype can be read-only.
>
> I think that format_flow_tunnel() in lib/match.c should format the new
> fields.
good catch, thanks
>
> The use of "? :" as in netdev_gtpu_build_header() is a GCC extension. I
> think that MSVC will reject it.
>
OK, will fix it
> Doesn't tun_key_to_attr() need to support the new fields?
good catch, thanks
>
> 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 didn't test it.
Here we only support GTP-U msgtype=255 (G-PDU).
For other GTP-U messages, (ex: echo req/response), users should use
OpenFlow rules to match it and send to their application.
Regards,
William
> 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
