On Wed, Apr 20, 2016 at 6:10 PM, Li, Johnson <johnson...@intel.com> wrote:
>> -----Original Message-----
>> From: Jesse Gross [mailto:je...@kernel.org]
>> Sent: Thursday, April 21, 2016 4:23 AM
>> To: Li, Johnson <johnson...@intel.com>
>> Cc: ovs dev <dev@openvswitch.org>
>> Subject: Re: [ovs-dev] [PATCH v2] Add VxLAN-GBP support for user space
>> data path
>> > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
>> > e398562..0ac449e 100644
>> > --- a/lib/netdev-vport.c
>> > +++ b/lib/netdev-vport.c
>> > @@ -1297,10 +1298,18 @@ netdev_vxlan_pop_header(struct dp_packet
>> *packet)
>> > return EINVAL;
>> > }
>> >
>> > - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) ||
>> > - (get_16aligned_be32(&vxh->vx_vni) & htonl(0xff))) {
>> > - VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x vni=%#x\n",
>> > - ntohl(get_16aligned_be32(&vxh->vx_flags)),
>> > + vxh_flags = get_16aligned_be32(&vxh->vx_flags);
>> > + if (vxh_flags & htonl(VXLAN_HF_GBP)) {
>>
>> This check needs to be conditional on GBP actually being enabled in some
>> form. Otherwise, you could misinterpret a different extension that uses
>> overlapping bit combinations.
> From the spec at
> https://tools.ietf.org/html/draft-smith-vxlan-group-policy-01#section-2.1
> it says " Bit 0 of the initial word is defined as the G (Group Based Policy
> Extension) bit".
> If the G bit is set, it indicates that the GBP ID is carried. Then I can
> conclude that if
> The header is VxLAN header(Special UDP destination port is detected), and the
> bit 0
> Is set, then the header should be VxLAN-GBP header.
This draft is an extension to VXLAN and so you can only follow it if
you know that that extension is being used. There are other proposals
with conflicting definitions.
>> > + tnl->gbp_id = htons(ntohl(vxh_flags) & VXLAN_GBP_ID_MASK);
>>
>> You can apply the byteswap to the mask instead of doing it twice on the flags
>> - AND operations can be done in any byte order.
>>
>> > ovs_mutex_unlock(&dev->mutex);
>> > diff --git a/lib/packets.h b/lib/packets.h index 8139a6b..c78b053
>> > 100644
>> > --- a/lib/packets.h
>> > +++ b/lib/packets.h
>> > @@ -1003,6 +1003,11 @@ struct vxlanhdr {
>> >
>> > #define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required
>> > value. */
>>
>> This is itself representing a flag in the VXLAN header, can you rename it to
>> be
>> consistent with the other header flags?
> This just indicates that the VNI is set and valid for the VxLAN [-GB P|-GPE]
> header.
Yes, please define it that way.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
