From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f177.google.com (mail-wr0-f177.google.com [209.85.128.177]) by dpdk.org (Postfix) with ESMTP id 8574E1C7FE for ; Thu, 5 Apr 2018 16:39:34 +0200 (CEST) Received: by mail-wr0-f177.google.com with SMTP id d17so13439910wre.1 for ; Thu, 05 Apr 2018 07:39:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=+smcYX5XvSZCdo662T3hSOXx0fIU5ZWHk/+8R98k9lo=; b=evLrD4JmPSW26ylO0k1kYxpEQuKqtSy7YBHOzZeSlVAwo9NAIvrb7AvBg2UOlDxWkk FXYaI+3URwfIXoL6Op2V2NtO9Y6aLzBpVFViEz2IZSHSPuL9KxGToq7NoVitPDO94OCX l3cjhmhCmv4jbNfYDgXPTHCeB3Nui6Q+K4MP5wwksX2Lacxf+1mTsZ5af8eGgvU18sOT s/NIuxG1xy/q6rku59hSlLfjCaRxFsNJigh5Mz9BuvpexqqOTdKD2vGNDJXeHzgmIvep 7eEYJZaU7YVyWKVDaQP3YC1T5ON7O7IylIsXYZm4ZkgyaSJq5+2SRA5xu3Y+R+tMgYJa nLWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=+smcYX5XvSZCdo662T3hSOXx0fIU5ZWHk/+8R98k9lo=; b=TUx3eXYK9L5HYx3rycP+r/IJNvxC7+Fj0b7XgN4ktG/NfMDe33dl0vpXfOr7cYh9Nw /aowfZk4lduX1kDzwz1KUGku2PveRym+avB0a4uN8LkcWgpnfoFa4Sf+pVLQJHrq42x7 kMi8BD+yWXXQM2X3i23DFb5wDT6ESwr2GPQuXtBDvGzjrIWH2dk7gdaH0b6I4AFPU+3X AXiy4cy0/bD/I5oHSDbpCCnSHaJ7GHBBBhQuzZaxI0fCDmzDTdztcbN7cg3x0E12+9H/ NqH4ziXVIwnl0Nh0v1rUhqFyUrYg4LxbnWFiRj3Pr+wraiot35+B7JBzTftSCYK4izBQ /0dg== X-Gm-Message-State: AElRT7EUKNMPrFTX4cZwiXVvUvN3ckDJHnH78v0acAyJnFhzWKlOTyoY XlfZQwodWFXF4VMM0CU9QWLUNQ== X-Google-Smtp-Source: AIpwx4/R++DQKFSC8Ks9OUK+rIvTeaiTMpdQgzVqeKnw+N1ASC2LlevOZ+Hwt7I9ZYUNR6oK2rsmig== X-Received: by 10.223.158.6 with SMTP id u6mr16159499wre.142.1522939174267; Thu, 05 Apr 2018 07:39:34 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id t196sm6074611wme.35.2018.04.05.07.39.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Apr 2018 07:39:33 -0700 (PDT) Date: Thu, 5 Apr 2018 16:39:20 +0200 From: Adrien Mazarguil To: "Zhang, Qi Z" Cc: Thomas Monjalon , "Yigit, Ferruh" , "dev@dpdk.org" , "Lu, Wenzhuo" , "Wu, Jingjing" , Ajit Khaparde , Somnath Kotur , John Daley , Hyong Youb Kim , "Xing, Beilei" , "Ananyev, Konstantin" , Nelio Laranjeiro , Yongseok Koh , Jacek Siuda , Tomasz Duszynski , Dmitri Epshtein , Natalie Samsonov , Jianbo Liu , Andrew Rybchenko , Pascal Mazon Message-ID: <20180405143920.GM4957@6wind.com> References: <20180404150312.12304-1-adrien.mazarguil@6wind.com> <20180404150312.12304-12-adrien.mazarguil@6wind.com> <039ED4275CED7440929022BC67E7061153182FE4@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <039ED4275CED7440929022BC67E7061153182FE4@SHSMSX103.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v1 11/16] ethdev: refine TPID handling in flow API 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: , X-List-Received-Date: Thu, 05 Apr 2018 14:39:34 -0000 On Thu, Apr 05, 2018 at 02:20:34PM +0000, Zhang, Qi Z wrote: > Hi Adrien: > > > Hi PMD maintainers, while I'm pretty confident in these changes, I could not > > validate them with all devices. > > > > It would be great if you could apply this patch, run testpmd, create VLAN flow > > rules with/without inner EtherType as described and send matching traffic > > while making sure nothing was broken in the process. > > > > Thanks! > > --- > > diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c index > > 1b336df74..c6dd889ad 100644 > > --- a/drivers/net/i40e/i40e_flow.c > > +++ b/drivers/net/i40e/i40e_flow.c > > @@ -2490,24 +2490,36 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev > > *dev, > > "Invalid MAC_addr mask."); > > return -rte_errno; > > } > > + } > > + if (eth_spec && eth_mask && eth_mask->type) { > > + enum rte_flow_item_type next = (item + 1)->type; > > > > - if ((eth_mask->type & UINT16_MAX) == > > - UINT16_MAX) { > > - input_set |= I40E_INSET_LAST_ETHER_TYPE; > > - filter->input.flow.l2_flow.ether_type = > > - eth_spec->type; > > + if (eth_mask->type != RTE_BE16(0xffff)) { > > + rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Invalid type mask."); > > + return -rte_errno; > > } > > > > ether_type = rte_be_to_cpu_16(eth_spec->type); > > - if (ether_type == ETHER_TYPE_IPv4 || > > - ether_type == ETHER_TYPE_IPv6 || > > - ether_type == ETHER_TYPE_ARP || > > - ether_type == outer_tpid) { > > + > > + if ((next == RTE_FLOW_ITEM_TYPE_VLAN && > > + ether_type != outer_tpid) || > > + (next != RTE_FLOW_ITEM_TYPE_VLAN && > > + (ether_type == ETHER_TYPE_IPv4 || > > + ether_type == ETHER_TYPE_IPv6 || > > + ether_type == ETHER_TYPE_ARP || > > + ether_type == outer_tpid))) { > > rte_flow_error_set(error, EINVAL, > > RTE_FLOW_ERROR_TYPE_ITEM, > > item, > > "Unsupported ether_type."); > > return -rte_errno; > > + } else if (next != RTE_FLOW_ITEM_TYPE_VLAN) { > > + input_set |= I40E_INSET_LAST_ETHER_TYPE; > > + filter->input.flow.l2_flow.ether_type = > > + eth_spec->type; > > } > > } > > > > @@ -2518,6 +2530,14 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev > > *dev, > > case RTE_FLOW_ITEM_TYPE_VLAN: > > vlan_spec = item->spec; > > vlan_mask = item->mask; > > + > > + if (input_set & I40E_INSET_LAST_ETHER_TYPE) { > > + rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ITEM, > > + item, > > + "Unsupported outer TPID."); > > + return -rte_errno; > > + } > > Please correct me I'm wrong, now I40E_INSET_LAST_ETHER_TYPE will only be set when next != RTE_FLOW_ITEM_TYPE_VLAN > while, RTE_FLOW_ITEM_TYPE_VLAN will only be the next to RTE_FLOW_ITEM_TYPE_ETH for fdir, so above check seems unnecessary ? You're right, it's unnecessary. I can remove this change or leave it as a safety measure for future contributions, since when parsing a VLAN item one is not necessarily aware of what happened in previous iterations. How about an assertion check for debugging purposes instead? RTE_ASSERT(!(input_set & I40E_INSET_LAST_ETHER_TYPE)); -- Adrien Mazarguil 6WIND