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 <[email protected]> >>>>> 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 <[email protected]> >>>>> --- >>>>> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
