From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id D7F67A04E6; Wed, 18 Nov 2020 07:37:13 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A6E7C58C4; Wed, 18 Nov 2020 07:37:11 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by dpdk.org (Postfix) with ESMTP id 7C7F34C90 for ; Wed, 18 Nov 2020 07:37:09 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id E0C6E7F548; Wed, 18 Nov 2020 09:37:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru E0C6E7F548 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1605681427; bh=1+7WSyXHBi5vK9S6qQn7lp2J09GHF4we0aSEnIpYd2g=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=leY1BWDluBZK1LskQMqGMrcmqlRcc5FXBwxckxLNLDmIWKpFU+RQn10r36ly4jqkH yMmuiZIrUIL9644dpFRmKHftIqGWy+t/RmzkYJ8ZdHslcmdMC2YPmNsvy5XD+hcOLU JBI5rSkNpH27olro5AMttImtR7ixI8mmgRvFffV0= To: Ajit Khaparde , Ferruh Yigit Cc: Xiaoyu Min , Somnath Kotur , dpdk-dev , Xiaoyu Min References: <141eb0140301108b1320ecfd93c89e48b02cd546.1605493464.git.jackmin@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <34f0ab63-ca71-85ea-2fc6-840b1c030ffb@oktetlabs.ru> Date: Wed, 18 Nov 2020 09:37:07 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 11/18/20 3:34 AM, Ajit Khaparde wrote: > On Mon, Nov 16, 2020 at 8:13 AM Ferruh Yigit wrote: > >> On 11/16/2020 7:55 AM, Xiaoyu Min wrote: >>> From: Xiaoyu Min >>> >>> The rte_flow_item_eth and rte_flow_item_vlan items are refined. >>> The structs do not exactly represent the packet bits captured on the >>> wire anymore so should only copy real header instead of the whole struct. >>> >>> Replace the rte_flow_item_* with the existing corresponding rte_*_hdr. >>> >>> Fixes: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN >> items") >>> >>> Signed-off-by: Xiaoyu Min >> >> <...> >> >>> @@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct >> rte_flow_action *action_item, >>> BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n"); >>> return BNXT_TF_RC_ERROR; >>> } >>> - vxlan_size = sizeof(struct rte_flow_item_vxlan); >>> + vxlan_size = sizeof(struct rte_vxlan_hdr); >>> /* copy the vxlan details */ >>> memcpy(&vxlan_spec, item->spec, vxlan_size); >>> vxlan_spec.flags = 0x08; >>> >> >> 'vxlan_size' seems used both to copy rt_flow_item [1] and to header [2]. >> Also >> ''vxlan_size' is used to copy the 'vxlan_spec'. >> >> Since both "struct rte_flow_item_vxlan" & "struct rte_vxlan_hdr" size is >> same, >> this should work fine, but I guess it may be broken if sizes of those two >> structures changes in the future. >> >> [1] >> memcpy(&vxlan_spec, item->spec, vxlan_size); >> >> [2] >> ulp_encap_buffer_copy(buff, (const uint8_t *)&vxlan_spec, >> vxlan_size, ULP_BUFFER_ALIGN_8_BYTE); >> > Also, I feel rte_flow_item_vxlan is a control plane structure while > rte_vlan_hdr is more for dataplane. > It's better to keep them separate to allow refining them independently. I disagree with the point. It is typical duplication which should be fixed. VXLAN item should consist of rte_vxlan_hdr plus extra non header fields. In fact, similar thing should be done for Ethernet flow item spec. These items are used for VXLAN encap action and in current state of definitions it is tricky to use VXLAN and Ethernet items to construct header to be prepended. It is required to copy field by field since it is bad to rely on the fact that flow item starts from protocol header if it is not specified in corresponding C type definition.