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 1496CA04DD; Wed, 18 Nov 2020 18:44:54 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 16FF05AAE; Wed, 18 Nov 2020 18:44:52 +0100 (CET) Received: from mail-oi1-f193.google.com (mail-oi1-f193.google.com [209.85.167.193]) by dpdk.org (Postfix) with ESMTP id 0A495F90 for ; Wed, 18 Nov 2020 18:44:50 +0100 (CET) Received: by mail-oi1-f193.google.com with SMTP id s18so1960107oih.1 for ; Wed, 18 Nov 2020 09:44:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nQ15jpEjint9yRyVcj/yYBBQTMnM3pmz4qyiRHGBbbU=; b=h37MnOidcETv2HnVNWBOaKPSLAWLP2DG0EjJFwJa6zxvJ7wg9UQRLb0TufyjzAPSN9 Gn8XutSs20D0+t3WdJlHKOceeiL+ZaJGfL6R59lDFkQG4k7eBd1zpUM0JPjQS+J9WnCJ Xr+HOzCryvTJTZIg8umZn/hmaXF61tIIN+4pI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nQ15jpEjint9yRyVcj/yYBBQTMnM3pmz4qyiRHGBbbU=; b=aYTFrXw470ats/cK2K/sgJw/x/DsLNTXCO/dBrfScoZpjjUPvYM81ocHI26MPsecBC 6CPcKdfms3Dghltm7W4Mgr03oulkDt9xgNXL1aXHubNKKJgQXPxiju8sJB5gYmyyZAgu bfqzcSfd4Q3nZTe/9b6/k0HUJ9pMXD+BHKnLtyobXzqnUsbWIXrxhArwI2jUHQreqj6x /unqQqI4+YvMejF6VBlYI2vui0e7MbnTyh0tq6YXNo9vGDTs+Z+IXXl7Yh6NLxQOQfXZ mKCjT7V9TD0FSWSuMj5y6SRKYOobAiVdQmuTweU/JP3ThFpzH/KUK2RvIx8b5mcZOd3y 32LQ== X-Gm-Message-State: AOAM532CAK4q/E9Jn8mA6sITbZDLsCFFnOoKAxdvnpgsb9yTDCO3prUB zAys0ufnh3yOObGZ69KJ+oJloazhNpwrm8xjx0KU9Q== X-Google-Smtp-Source: ABdhPJw7Kj+OD/SMFOz30a3+21+tvTCkgNORrsJwq3ekFC/NMbHUbf6hUGtLnklH6z8SOvSxoZr7FuuEukQxI5fKfmc= X-Received: by 2002:aca:4d07:: with SMTP id a7mr164486oib.168.1605721487938; Wed, 18 Nov 2020 09:44:47 -0800 (PST) MIME-Version: 1.0 References: <141eb0140301108b1320ecfd93c89e48b02cd546.1605493464.git.jackmin@nvidia.com> <34f0ab63-ca71-85ea-2fc6-840b1c030ffb@oktetlabs.ru> In-Reply-To: <34f0ab63-ca71-85ea-2fc6-840b1c030ffb@oktetlabs.ru> From: Ajit Khaparde Date: Wed, 18 Nov 2020 09:44:31 -0800 Message-ID: To: Andrew Rybchenko Cc: Ferruh Yigit , Xiaoyu Min , Somnath Kotur , dpdk-dev , Xiaoyu Min Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 Tue, Nov 17, 2020 at 10:37 PM Andrew Rybchenko wrote: > > 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. I took a look at the change again. I had an impression that it is changing from specific protocol header to rte flow item, not the other way around. This is fine. Note that the commit is changing VXLAN and VLAN. But the log suggests only VXLAN. Other than that, Acked-by: Ajit Khaparde