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 9B1B1A04B5 for ; Fri, 6 Nov 2020 18:41:44 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8716C2E8B; Fri, 6 Nov 2020 18:41:40 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 224ED29C6; Fri, 6 Nov 2020 18:41:35 +0100 (CET) IronPort-SDR: /ZpH5eZdOo1Y2DMMw4N31wyblfmocmguNQhpiJ92jzEuNKroqQgmJvtgnns+A6MILEuA0I6Fv3 Gqf7EmeQXOmA== X-IronPort-AV: E=McAfee;i="6000,8403,9797"; a="187504278" X-IronPort-AV: E=Sophos;i="5.77,457,1596524400"; d="scan'208";a="187504278" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Nov 2020 09:41:34 -0800 IronPort-SDR: v/krduuqUGyLDKY/M7rqaMV0Ml39Bfkz0/GuTLEwYVrCiH3yQCujZj12Dqu3pXwkGsjngfdpS+ 84pGLsRZSZWw== X-IronPort-AV: E=Sophos;i="5.77,457,1596524400"; d="scan'208";a="539921968" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.228.45]) ([10.213.228.45]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Nov 2020 09:41:32 -0800 To: Bing Zhao , Slava Ovsiienko , Matan Azrad Cc: "dev@dpdk.org" , Ori Kam , Raslan Darawsheh , "stable@dpdk.org" References: <1604382118-336293-1-git-send-email-bingz@nvidia.com> From: Ferruh Yigit Message-ID: <7b10f95c-bd9f-afd9-8925-73552fb37c09@intel.com> Date: Fri, 6 Nov 2020 17:41:28 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/mlx5: fix the eCPRI common header endianness X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On 11/6/2020 2:10 PM, Bing Zhao wrote: > Hi Ferruh, > > Thanks for your review and comments, PSB. > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Friday, November 6, 2020 7:20 PM >> To: Bing Zhao ; Slava Ovsiienko >> ; Matan Azrad >> Cc: dev@dpdk.org; Ori Kam ; Raslan Darawsheh >> ; stable@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix the eCPRI common >> header endianness >> >> External email: Use caution opening links or attachments >> >> >> On 11/3/2020 5:41 AM, Bing Zhao wrote: >>> The input header of a RTE flow item is with network byte order. In >> the >>> host with little endian, the bit field order are the same as the >> byte >>> order. >>> When checking the an eCPRI message type, the wrong field will be >>> selected. Right now, since the whole u32 is being checked and for >> all >>> types, the following implementation is unique. There is no >> functional >>> risk but it is still an error to fix. >> > >> >> Isn't the 'ecpri_v' filled by application (CPU), why there is an >> assumption that it is big endian? >> > > Yes, this is filled by the application SW. I checked the current code of other headers and current implementation. > 1. In the testpmd flow example, all the headers of network stack like IPv4 are translated into to be in BE, "ARGS_ENTRY_HTON" > 2. All fields are with "rte_be*_t"type, even though only a typedef, it should be considered in BE. > 3. RTE flow will just pass the flow items pointer to the PMD drivers, so in the PMD part, the headers should be treated as in BE. > So, to my understanding, this is not an assumption but some constraint. > Correct me if my understanding is wrong. I missed in 'cmdline_flow.c' big endian conversion done via 'arg->hton' check, so yes PMD getting big endian values makes sense, thanks for clarification. > >> And even if it is big endian, it should be broken previously right? >> Since it was checking wrong field as 'type' as you said, why there >> were no functional risk? > > In the PMD driver, the first u32 *value containing this type is already used for matching. And a checking is used for the following "sub-headers" > matching. Indeed, the first u32 *mask is used to confirm if the following sub-header need to be matched. > Since different types will lead to different variants of the packets, the switch-of-type is used. > But all the 3 types supported in PMD now almost have the same results (part of the second u32, remaining will be all 0s). > So even if the type of the value is always "0" by mistake, the cases are the same and it still works by coincidence. if 'type' is '0' I see it works, but what if the 'type' is something other than values covered in 'switch', than it may fail matching 'sub-headers' and I guess that can happen based on value of 'size' field. Anyway, since you are already fixing it, will you be OK if I drop last two sentences from the commit log and proceed?