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.haa...@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 <el...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
