From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BEE62A0A05; Tue, 19 Jan 2021 20:14:48 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 89565140D45; Tue, 19 Jan 2021 20:14:48 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 92A7B140D41; Tue, 19 Jan 2021 20:14:46 +0100 (CET) IronPort-SDR: GGOqoW4+P5kyk9i5gfLhq07Y1W63VZLKutAjfGQGnx+KksP5JDeUfnIC9NHePdmHvMtF/4Vko8 UThZPanZf0iA== X-IronPort-AV: E=McAfee;i="6000,8403,9869"; a="179130597" X-IronPort-AV: E=Sophos;i="5.79,359,1602572400"; d="scan'208";a="179130597" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2021 11:14:45 -0800 IronPort-SDR: zbLqGHRXGTEZENiMg5lYDEg98TdlBjPgVz0J9oftJz4VTyTfuVXeniZSxCl3EocwvIiRgGavqx qwGgCDlrvLyg== X-IronPort-AV: E=Sophos;i="5.79,359,1602572400"; d="scan'208";a="426586721" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.241.104]) ([10.213.241.104]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2021 11:14:44 -0800 To: "Zhang,Alvin" Cc: dev@dpdk.org, stable@dpdk.org References: <20210118085937.12072-1-alvinx.zhang@intel.com> From: Ferruh Yigit Message-ID: Date: Tue, 19 Jan 2021 19:14:40 +0000 MIME-Version: 1.0 In-Reply-To: <20210118085937.12072-1-alvinx.zhang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix RSS key X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 1/18/2021 8:59 AM, Zhang,Alvin wrote: > From: Alvin Zhang > > Since the patch '1848b117' has set the value of key in 'struct > rte_flow_action_rss' to NULL, the PMD cannot get the RSS key now. > > This patch sets offset and size of the key pointer as the first > parameter of the token 'key' and copies the start address of the > 'HEX' data to the location specified by the first parameter of > the token. > Can you please put the rte_flow command that enables reproducing this defect, it may help in the future? > Fixes: 1848b117cca1 ("app/testpmd: fix RSS key for flow API RSS rule") > Cc: stable@dpdk.org > > Signed-off-by: Alvin Zhang > --- > app/test-pmd/cmdline_flow.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index 585cab9..6eb46d3 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -3403,7 +3403,10 @@ static int comp_set_sample_index(struct context *, const struct token *, > .name = "key", > .help = "RSS hash key", > .next = NEXT(action_rss, NEXT_ENTRY(HEX)), > - .args = ARGS(ARGS_ENTRY_ARB(0, 0), > + .args = ARGS(ARGS_ENTRY_ARB > + (offsetof(struct action_rss_data, conf) + > + offsetof(struct rte_flow_action_rss, key), > + sizeof(((struct rte_flow_action_rss *)0)->key)), +1, it is required to write the address, and I confirm this enables getting 'key' value to the PMD. > ARGS_ENTRY_ARB > (offsetof(struct action_rss_data, conf) + > offsetof(struct rte_flow_action_rss, key_len), > @@ -6495,19 +6498,18 @@ static int comp_set_sample_index(struct context *, const struct token *, > if (ctx->objmask) > memset((uint8_t *)ctx->objmask + arg_data->offset, > 0xff, hexlen); > + > /* Save address if requested. */ > if (arg_addr->size) { > - memcpy((uint8_t *)ctx->object + arg_addr->offset, > - (void *[]){ > - (uint8_t *)ctx->object + arg_data->offset > - }, > - arg_addr->size); > + if (arg_addr->size < sizeof(void *)) > + goto error; Above two lines seems only actual change in this function, others are refactoring the assignment. 1) why this check required, I think we can ignore it. 2) What do you think to remove the refactoring to reduce the change to the actual fix? > + > + *(void **)((uint8_t *)ctx->object + arg_addr->offset) = > + (uint8_t *)ctx->object + arg_data->offset; > + > if (ctx->objmask) > - memcpy((uint8_t *)ctx->objmask + arg_addr->offset, > - (void *[]){ > - (uint8_t *)ctx->objmask + arg_data->offset > - }, > - arg_addr->size); > + *(void **)((uint8_t *)ctx->objmask + arg_addr->offset) = > + (uint8_t *)ctx->objmask + arg_data->offset; > } > return len; > error: >