| Message ID | 20210711051514.14634-3-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] netdev-offload-dpdk: Fix IPv6 rewrite cast-align warning | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
> -----Original Message----- > From: Eli Britstein <elibr@nvidia.com> > Sent: Sunday, July 11, 2021 6:15 AM > To: dev@openvswitch.org; Ilya Maximets <i.maximets@ovn.org>; Van Haaren, > Harry <harry.van.haaren@intel.com> > Cc: Gaetan Rivet <gaetanr@nvidia.com>; Majd Dibbiny <majd@nvidia.com>; Eli > Britstein <elibr@nvidia.com> > Subject: [PATCH 2/2] netdev-offload-dpdk: Fix vxlan vni cast-align warnings > > Compiling with -Werror and -Wcast-align has errors like: > > lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': > lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment > of target type [-Werror=cast-align] > 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > | ^ > > Fix them. > > Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> > Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap > attribute.") > Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") > Signed-off-by: Eli Britstein <elibr@nvidia.com> Thanks Eli, tested compilation, and the cast-align issue is resolved for master branch with these the patches in this series. I cannot test functionality here, so just compile tested. Series-Tested-by: Harry van Haaren <harry.van.haaren@intel.com>
On 7/11/21 7:15 AM, Eli Britstein wrote: > Compiling with -Werror and -Wcast-align has errors like: > > lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': > lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment > of target type [-Werror=cast-align] > 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > | ^ > > Fix them. > > Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> > Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") > Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") > Signed-off-by: Eli Britstein <elibr@nvidia.com> > --- > lib/netdev-offload-dpdk.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index a24f92782..e4b19ae40 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, > } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { > const struct rte_flow_item_vxlan *vxlan_spec = item->spec; > const struct rte_flow_item_vxlan *vxlan_mask = item->mask; > + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % > + sizeof(ovs_be32) == 0); > > ds_put_cstr(s, "vxlan "); > if (vxlan_spec) { > @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, > vxlan_mask = &rte_flow_item_vxlan_mask; > } > DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, > - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, > - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); > + ntohl(*ALIGNED_CAST(ovs_be32 *, > + vxlan_spec->vni)) >> 8, > + ntohl(*ALIGNED_CAST(ovs_be32 *, > + vxlan_mask->vni)) >> 8); > } > ds_put_cstr(s, "/ "); > } else { > @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) > ds_put_format(s, "set vxlan ip-version %s ", > ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); > if (vxlan) { > + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % > + sizeof(ovs_be32) == 0); > ds_put_format(s, "vni %"PRIu32" ", > - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); > + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); > } > if (udp) { > ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", > @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, > vx_spec = xzalloc(sizeof *vx_spec); > vx_mask = xzalloc(sizeof *vx_mask); > > - put_unaligned_be32((ovs_be32 *) vx_spec->vni, > + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % > + sizeof(ovs_be32) == 0); > + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), > htonl(ntohll(match->flow.tunnel.tun_id) << 8)); > - put_unaligned_be32((ovs_be32 *) vx_mask->vni, > + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), > htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); > > consumed_masks->tunnel.tun_id = 0; > Same concerns here about the build time assertion as in the patch #1. It also seems redundant to use put_unaligned_* functions and have a build assertion at the same time. Suggesting to just use put/get_unaligned_* in all cases and remove build time assertions. Best regards, Ilya Maximets.
On 7/22/2021 3:28 PM, Ilya Maximets wrote: > External email: Use caution opening links or attachments > > > On 7/11/21 7:15 AM, Eli Britstein wrote: >> Compiling with -Werror and -Wcast-align has errors like: >> >> lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': >> lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment >> of target type [-Werror=cast-align] >> 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >> | ^ >> >> Fix them. >> >> Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> >> Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") >> Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> --- >> lib/netdev-offload-dpdk.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index a24f92782..e4b19ae40 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, >> } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { >> const struct rte_flow_item_vxlan *vxlan_spec = item->spec; >> const struct rte_flow_item_vxlan *vxlan_mask = item->mask; >> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >> + sizeof(ovs_be32) == 0); >> >> ds_put_cstr(s, "vxlan "); >> if (vxlan_spec) { >> @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, >> vxlan_mask = &rte_flow_item_vxlan_mask; >> } >> DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, >> - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >> - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); >> + ntohl(*ALIGNED_CAST(ovs_be32 *, >> + vxlan_spec->vni)) >> 8, >> + ntohl(*ALIGNED_CAST(ovs_be32 *, >> + vxlan_mask->vni)) >> 8); >> } >> ds_put_cstr(s, "/ "); >> } else { >> @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) >> ds_put_format(s, "set vxlan ip-version %s ", >> ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); >> if (vxlan) { >> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >> + sizeof(ovs_be32) == 0); >> ds_put_format(s, "vni %"PRIu32" ", >> - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); >> + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); >> } >> if (udp) { >> ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", >> @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, >> vx_spec = xzalloc(sizeof *vx_spec); >> vx_mask = xzalloc(sizeof *vx_mask); >> >> - put_unaligned_be32((ovs_be32 *) vx_spec->vni, >> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >> + sizeof(ovs_be32) == 0); >> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), >> htonl(ntohll(match->flow.tunnel.tun_id) << 8)); >> - put_unaligned_be32((ovs_be32 *) vx_mask->vni, >> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), >> htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); >> >> consumed_masks->tunnel.tun_id = 0; >> > Same concerns here about the build time assertion as in the patch #1. > It also seems redundant to use put_unaligned_* functions and have a > build assertion at the same time. > > Suggesting to just use put/get_unaligned_* in all cases and remove > build time assertions. The code before this patch just uses (for example) put_unaligned_be32, which its 1st argument is (ovs_be32 *). vni in struct rte_flow_item_vxlan in dpdk is uint8_t vni[3]; /**< VXLAN identifier. */ I use ALIGNED_CAST to mute the warning, and the assert to make sure the alignment is correct. I don't understand your suggestion here, unless you suggest to use memcpy as suggested in patch#1. > > Best regards, Ilya Maximets.
On 7/22/21 3:00 PM, Eli Britstein wrote: > > On 7/22/2021 3:28 PM, Ilya Maximets wrote: >> External email: Use caution opening links or attachments >> >> >> On 7/11/21 7:15 AM, Eli Britstein wrote: >>> Compiling with -Werror and -Wcast-align has errors like: >>> >>> lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': >>> lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment >>> of target type [-Werror=cast-align] >>> 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>> | ^ >>> >>> Fix them. >>> >>> Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> >>> Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") >>> Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") >>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>> --- >>> lib/netdev-offload-dpdk.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>> index a24f92782..e4b19ae40 100644 >>> --- a/lib/netdev-offload-dpdk.c >>> +++ b/lib/netdev-offload-dpdk.c >>> @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, >>> } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { >>> const struct rte_flow_item_vxlan *vxlan_spec = item->spec; >>> const struct rte_flow_item_vxlan *vxlan_mask = item->mask; >>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>> + sizeof(ovs_be32) == 0); >>> >>> ds_put_cstr(s, "vxlan "); >>> if (vxlan_spec) { >>> @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, >>> vxlan_mask = &rte_flow_item_vxlan_mask; >>> } >>> DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, >>> - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>> - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); >>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>> + vxlan_spec->vni)) >> 8, >>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>> + vxlan_mask->vni)) >> 8); >>> } >>> ds_put_cstr(s, "/ "); >>> } else { >>> @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) >>> ds_put_format(s, "set vxlan ip-version %s ", >>> ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); >>> if (vxlan) { >>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>> + sizeof(ovs_be32) == 0); >>> ds_put_format(s, "vni %"PRIu32" ", >>> - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); >>> + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); >>> } >>> if (udp) { >>> ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", >>> @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, >>> vx_spec = xzalloc(sizeof *vx_spec); >>> vx_mask = xzalloc(sizeof *vx_mask); >>> >>> - put_unaligned_be32((ovs_be32 *) vx_spec->vni, >>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>> + sizeof(ovs_be32) == 0); >>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), >>> htonl(ntohll(match->flow.tunnel.tun_id) << 8)); >>> - put_unaligned_be32((ovs_be32 *) vx_mask->vni, >>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), >>> htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); >>> >>> consumed_masks->tunnel.tun_id = 0; >>> >> Same concerns here about the build time assertion as in the patch #1. >> It also seems redundant to use put_unaligned_* functions and have a >> build assertion at the same time. >> >> Suggesting to just use put/get_unaligned_* in all cases and remove >> build time assertions. > > The code before this patch just uses (for example) put_unaligned_be32, which its 1st argument is (ovs_be32 *). > > vni in struct rte_flow_item_vxlan in dpdk is uint8_t vni[3]; /**< VXLAN identifier. */ > > I use ALIGNED_CAST to mute the warning, and the assert to make sure the alignment is correct. > > I don't understand your suggestion here, unless you suggest to use memcpy as suggested in patch#1. put_unaligned_be32 is an alias for put_unaligned_u32 that is implemented like this: 116 static inline void put_unaligned_u32(uint32_t *p_, uint32_t x_) 117 { 118 uint8_t *p = (uint8_t *) p_; 119 uint32_t x = ntohl(x_); 120 121 p[0] = x >> 24; 122 p[1] = x >> 16; 123 p[2] = x >> 8; 124 p[3] = x; 125 } or by the equivalent function provided by compiler. The memory copy performed byte-by-byte, hence independent form the original alignment of the memory. So, you may add ALIGNED_CAST to silence the warning, but we don't need the build assertion, because put_unaligned_* functions requires 1 byte alignment which is always there. Best regards, Ilya Maximets.
On 7/22/2021 4:10 PM, Ilya Maximets wrote: > External email: Use caution opening links or attachments > > > On 7/22/21 3:00 PM, Eli Britstein wrote: >> On 7/22/2021 3:28 PM, Ilya Maximets wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On 7/11/21 7:15 AM, Eli Britstein wrote: >>>> Compiling with -Werror and -Wcast-align has errors like: >>>> >>>> lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': >>>> lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment >>>> of target type [-Werror=cast-align] >>>> 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>>> | ^ >>>> >>>> Fix them. >>>> >>>> Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> >>>> Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") >>>> Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") >>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>> --- >>>> lib/netdev-offload-dpdk.c | 18 +++++++++++++----- >>>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>> index a24f92782..e4b19ae40 100644 >>>> --- a/lib/netdev-offload-dpdk.c >>>> +++ b/lib/netdev-offload-dpdk.c >>>> @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, >>>> } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { >>>> const struct rte_flow_item_vxlan *vxlan_spec = item->spec; >>>> const struct rte_flow_item_vxlan *vxlan_mask = item->mask; >>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>> + sizeof(ovs_be32) == 0); >>>> >>>> ds_put_cstr(s, "vxlan "); >>>> if (vxlan_spec) { >>>> @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, >>>> vxlan_mask = &rte_flow_item_vxlan_mask; >>>> } >>>> DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, >>>> - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>>> - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); >>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>>> + vxlan_spec->vni)) >> 8, >>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>>> + vxlan_mask->vni)) >> 8); >>>> } >>>> ds_put_cstr(s, "/ "); >>>> } else { >>>> @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) >>>> ds_put_format(s, "set vxlan ip-version %s ", >>>> ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); >>>> if (vxlan) { >>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>> + sizeof(ovs_be32) == 0); >>>> ds_put_format(s, "vni %"PRIu32" ", >>>> - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); >>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); >>>> } >>>> if (udp) { >>>> ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", >>>> @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, >>>> vx_spec = xzalloc(sizeof *vx_spec); >>>> vx_mask = xzalloc(sizeof *vx_mask); >>>> >>>> - put_unaligned_be32((ovs_be32 *) vx_spec->vni, >>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>> + sizeof(ovs_be32) == 0); >>>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), >>>> htonl(ntohll(match->flow.tunnel.tun_id) << 8)); >>>> - put_unaligned_be32((ovs_be32 *) vx_mask->vni, >>>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), >>>> htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); >>>> >>>> consumed_masks->tunnel.tun_id = 0; >>>> >>> Same concerns here about the build time assertion as in the patch #1. >>> It also seems redundant to use put_unaligned_* functions and have a >>> build assertion at the same time. >>> >>> Suggesting to just use put/get_unaligned_* in all cases and remove >>> build time assertions. >> The code before this patch just uses (for example) put_unaligned_be32, which its 1st argument is (ovs_be32 *). >> >> vni in struct rte_flow_item_vxlan in dpdk is uint8_t vni[3]; /**< VXLAN identifier. */ >> >> I use ALIGNED_CAST to mute the warning, and the assert to make sure the alignment is correct. >> >> I don't understand your suggestion here, unless you suggest to use memcpy as suggested in patch#1. > put_unaligned_be32 is an alias for put_unaligned_u32 that > is implemented like this: > > 116 static inline void put_unaligned_u32(uint32_t *p_, uint32_t x_) > 117 { > 118 uint8_t *p = (uint8_t *) p_; > 119 uint32_t x = ntohl(x_); > 120 > 121 p[0] = x >> 24; > 122 p[1] = x >> 16; > 123 p[2] = x >> 8; > 124 p[3] = x; > 125 } > > or by the equivalent function provided by compiler. > > The memory copy performed byte-by-byte, hence independent form the > original alignment of the memory. So, you may add ALIGNED_CAST to > silence the warning, but we don't need the build assertion, because > put_unaligned_* functions requires 1 byte alignment which is always > there. Removing the asserts implies knowledge of the internal implementation of put_unaligned_u32 by another file. > > Best regards, Ilya Maximets.
On 7/22/21 3:14 PM, Eli Britstein wrote: > > On 7/22/2021 4:10 PM, Ilya Maximets wrote: >> External email: Use caution opening links or attachments >> >> >> On 7/22/21 3:00 PM, Eli Britstein wrote: >>> On 7/22/2021 3:28 PM, Ilya Maximets wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> On 7/11/21 7:15 AM, Eli Britstein wrote: >>>>> Compiling with -Werror and -Wcast-align has errors like: >>>>> >>>>> lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': >>>>> lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment >>>>> of target type [-Werror=cast-align] >>>>> 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>>>> | ^ >>>>> >>>>> Fix them. >>>>> >>>>> Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> >>>>> Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") >>>>> Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") >>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>>> --- >>>>> lib/netdev-offload-dpdk.c | 18 +++++++++++++----- >>>>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>> index a24f92782..e4b19ae40 100644 >>>>> --- a/lib/netdev-offload-dpdk.c >>>>> +++ b/lib/netdev-offload-dpdk.c >>>>> @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, >>>>> } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { >>>>> const struct rte_flow_item_vxlan *vxlan_spec = item->spec; >>>>> const struct rte_flow_item_vxlan *vxlan_mask = item->mask; >>>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>>> + sizeof(ovs_be32) == 0); >>>>> >>>>> ds_put_cstr(s, "vxlan "); >>>>> if (vxlan_spec) { >>>>> @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, >>>>> vxlan_mask = &rte_flow_item_vxlan_mask; >>>>> } >>>>> DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, >>>>> - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, >>>>> - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); >>>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>>>> + vxlan_spec->vni)) >> 8, >>>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, >>>>> + vxlan_mask->vni)) >> 8); >>>>> } >>>>> ds_put_cstr(s, "/ "); >>>>> } else { >>>>> @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) >>>>> ds_put_format(s, "set vxlan ip-version %s ", >>>>> ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); >>>>> if (vxlan) { >>>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>>> + sizeof(ovs_be32) == 0); >>>>> ds_put_format(s, "vni %"PRIu32" ", >>>>> - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); >>>>> + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); >>>>> } >>>>> if (udp) { >>>>> ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", >>>>> @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, >>>>> vx_spec = xzalloc(sizeof *vx_spec); >>>>> vx_mask = xzalloc(sizeof *vx_mask); >>>>> >>>>> - put_unaligned_be32((ovs_be32 *) vx_spec->vni, >>>>> + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % >>>>> + sizeof(ovs_be32) == 0); >>>>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), >>>>> htonl(ntohll(match->flow.tunnel.tun_id) << 8)); >>>>> - put_unaligned_be32((ovs_be32 *) vx_mask->vni, >>>>> + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), >>>>> htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); >>>>> >>>>> consumed_masks->tunnel.tun_id = 0; >>>>> >>>> Same concerns here about the build time assertion as in the patch #1. >>>> It also seems redundant to use put_unaligned_* functions and have a >>>> build assertion at the same time. >>>> >>>> Suggesting to just use put/get_unaligned_* in all cases and remove >>>> build time assertions. >>> The code before this patch just uses (for example) put_unaligned_be32, which its 1st argument is (ovs_be32 *). >>> >>> vni in struct rte_flow_item_vxlan in dpdk is uint8_t vni[3]; /**< VXLAN identifier. */ >>> >>> I use ALIGNED_CAST to mute the warning, and the assert to make sure the alignment is correct. >>> >>> I don't understand your suggestion here, unless you suggest to use memcpy as suggested in patch#1. >> put_unaligned_be32 is an alias for put_unaligned_u32 that >> is implemented like this: >> >> 116 static inline void put_unaligned_u32(uint32_t *p_, uint32_t x_) >> 117 { >> 118 uint8_t *p = (uint8_t *) p_; >> 119 uint32_t x = ntohl(x_); >> 120 >> 121 p[0] = x >> 24; >> 122 p[1] = x >> 16; >> 123 p[2] = x >> 8; >> 124 p[3] = x; >> 125 } >> >> or by the equivalent function provided by compiler. >> >> The memory copy performed byte-by-byte, hence independent form the >> original alignment of the memory. So, you may add ALIGNED_CAST to >> silence the warning, but we don't need the build assertion, because >> put_unaligned_* functions requires 1 byte alignment which is always >> there. > Removing the asserts implies knowledge of the internal implementation of put_unaligned_u32 by another file. The main and only purpose of this function is to copy 32 bits regardless of their alignment: /* Stores 'x' at possibly misaligned address 'p'. */ There is no reason to use it otherwise. So, it's perfectly fine that the module that uses it knows what this function is supposed to do.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index a24f92782..e4b19ae40 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -375,6 +375,8 @@ dump_flow_pattern(struct ds *s, } else if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN) { const struct rte_flow_item_vxlan *vxlan_spec = item->spec; const struct rte_flow_item_vxlan *vxlan_mask = item->mask; + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % + sizeof(ovs_be32) == 0); ds_put_cstr(s, "vxlan "); if (vxlan_spec) { @@ -382,8 +384,10 @@ dump_flow_pattern(struct ds *s, vxlan_mask = &rte_flow_item_vxlan_mask; } DUMP_PATTERN_ITEM(vxlan_mask->vni, "vni", "%"PRIu32, - ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, - ntohl(*(ovs_be32 *) vxlan_mask->vni) >> 8); + ntohl(*ALIGNED_CAST(ovs_be32 *, + vxlan_spec->vni)) >> 8, + ntohl(*ALIGNED_CAST(ovs_be32 *, + vxlan_mask->vni)) >> 8); } ds_put_cstr(s, "/ "); } else { @@ -417,8 +421,10 @@ dump_vxlan_encap(struct ds *s, const struct rte_flow_item *items) ds_put_format(s, "set vxlan ip-version %s ", ipv4 ? "ipv4" : ipv6 ? "ipv6" : "ERR"); if (vxlan) { + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % + sizeof(ovs_be32) == 0); ds_put_format(s, "vni %"PRIu32" ", - ntohl(*(ovs_be32 *) vxlan->vni) >> 8); + ntohl(*ALIGNED_CAST(ovs_be32 *, vxlan->vni)) >> 8); } if (udp) { ds_put_format(s, "udp-src %"PRIu16" udp-dst %"PRIu16" ", @@ -1003,9 +1009,11 @@ parse_vxlan_match(struct flow_patterns *patterns, vx_spec = xzalloc(sizeof *vx_spec); vx_mask = xzalloc(sizeof *vx_mask); - put_unaligned_be32((ovs_be32 *) vx_spec->vni, + BUILD_ASSERT_DECL(offsetof(struct rte_flow_item_vxlan, vni) % + sizeof(ovs_be32) == 0); + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_spec->vni), htonl(ntohll(match->flow.tunnel.tun_id) << 8)); - put_unaligned_be32((ovs_be32 *) vx_mask->vni, + put_unaligned_be32(ALIGNED_CAST(ovs_be32 *, vx_mask->vni), htonl(ntohll(match->wc.masks.tunnel.tun_id) << 8)); consumed_masks->tunnel.tun_id = 0;
Compiling with -Werror and -Wcast-align has errors like: lib/netdev-offload-dpdk.c: In function 'dump_flow_pattern': lib/netdev-offload-dpdk.c:385:38: error: cast increases required alignment of target type [-Werror=cast-align] 385 | ntohl(*(ovs_be32 *) vxlan_spec->vni) >> 8, | ^ Fix them. Reported-by: Harry Van Haaren <harry.van.haaren@intel.com> Fixes: 4e432d6f8128 ("netdev-offload-dpdk: Support tnl/push using vxlan encap attribute.") Fixes: e098c2f966cb ("netdev-dpdk-offload: Add vxlan pattern matching function.") Signed-off-by: Eli Britstein <elibr@nvidia.com> --- lib/netdev-offload-dpdk.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)