Re: [PATCH v2 2/2] net/cpfl: add TDI to flow engine

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 16725 bytes --]

Hi Wenjing,

Please find comments inlined

On 05/01/2024 08:16, wenjing.qiao@intel.com wrote:
> From: Wenjing Qiao<wenjing.qiao@intel.com>
>
> Add TDI implementation to a flow engine.
>
> Signed-off-by: Wenjing Qiao<wenjing.qiao@intel.com>
> ---
<snip>
> --- /dev/null
> +++ b/drivers/net/cpfl/cpfl_tdi.c
> @@ -0,0 +1,1282 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Intel Corporation
> + */
> +#include <rte_hash_crc.h>
> +#include <rte_tailq.h>
RTE includes should be after system includes
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "cpfl_actions.h"
> +#include "cpfl_flow.h"
> +#include "cpfl_fxp_rule.h"
> +#include "cpfl_tdi.h"
> +#include "cpfl_tdi_parser.h"
> +#include "rte_branch_prediction.h"
> +#include "rte_common.h"
> +#include "rte_flow.h"
Those includes should be together with other RTE includes and have angle 
brackets
> +
> +#define CPFL_NO_FWD 0
> +uint64_t cpfl_tdi_rule_cookie = CPFL_COOKIE_DEF;
> +
> +/*help function to do left shift on a byte array */
> +static void
> +cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount)
> +{
> +	uint32_t i;
> +	int j;
> +	uint32_t carry = 0;
> +
> +	if (shift_amount == 0)
> +		return;
> +
> +	for (i = 0; i < shift_amount; i += 8) {
> +		for (j = 0; j < length; j++) {
> +			uint32_t temp = (buf[j] << (shift_amount - i)) | carry;
> +
> +			carry = (temp >> 8) & 0xff;
> +			buf[j] = temp & 0xff;
> +		}
> +	}
> +}

This function does not looks correct to me, at least in case when 
shift_amount is larger than 8.

You can try something like:

static void
cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount)
{
         int i;
         int bit_shift = shift_amount & 0x7;
         int byte_shift = shift_amount >> 3;

         if (shift_amount >= (length * 8))
                 return;

         uint8_t prev = 0;
         for (i = 0; i < length; i++) {
                 uint8_t val = (buf[i] << bit_shift) | prev;
                 prev = buf[i] >> (8 - bit_shift);
                 buf[i] = val;
         }

         for (i = length - 1; i >= byte_shift; i--) {
                 buf[i] = buf[i - byte_shift];
         }

         for(; i >= 0; i--)
                 buf[i] = 0;
}

> +
> +/* help function to init a mask array with bit_width */
> +static void
> +cpfl_tdi_init_msk_buf(uint8_t *buf, int length, uint32_t bit_width)
> +{
> +	uint32_t i;
> +
> +	memset(buf, 0, length);
> +	for (i = 0; i < bit_width; i++) {
> +		cpfl_tdi_shift_left(buf, length, 1);
> +		buf[0] += 1;
> +	}
> +}

It seems like this function sets bit_width bits into the buf starting 
from the LSB. If so I'd suggest to make it simpler and not use 
cpfl_tdi_shift_left(). Instead you can just write UINT8/16/32/64_MAX 
into the corresponding buffer and decrease bit_width accordingly if it 
is bigger than 8/16/32/64 and finally write (1 << bit_width) - 1.

> +
> +/* help function to OR a byte array with value and mask */
> +static void
> +cpfl_tdi_or_buf(uint8_t *buf, int length, uint8_t *values, uint8_t *mask)
> +{
> +	int i;
> +
> +	for (i = 0; i < length; i++)
> +		buf[i] = (values[i] & mask[i]) | (buf[i] & ~mask[i]);
I guess "& ~mask[i]" is not necessary here if this function is really OR.
> +}
<snip>
> +
> +static void
> +cpfl_tdi_pack_sem_entry(struct cpfl_tdi_rule_info *rinfo,
> +			struct cpfl_tdi_ma_hardware_block *hb,
> +			enum cpfl_tdi_table_entry_op op,
> +			struct idpf_dma_mem *dma,
> +			struct idpf_ctlq_msg *msg)
> +{
> +	union cpfl_rule_cfg_pkt_record *blob;
> +	struct cpfl_rule_cfg_data cfg = {0};
> +	uint16_t cfg_ctrl;
> +	enum cpfl_ctlq_rule_cfg_opc opc = 0;
> +	const struct cpfl_tdi_table_key_obj *key = &rinfo->kobj;
> +	const struct cpfl_tdi_action_obj *action = &rinfo->aobj;
> +
> +	blob = (void *)dma->va;
Why not cast to (union cpfl_rule_cfg_pkt_record *)?
> +	memset(blob, 0, sizeof(*blob));
> +
> +	cfg_ctrl = CPFL_GET_MEV_SEM_RULE_CFG_CTRL(hb->profile[0], hb->sem.sub_profile, 0, 0);
> +
<snip>
> +static int
> +cpfl_tdi_rule_process(struct cpfl_itf *itf,
> +		      struct idpf_ctlq_info *tx_cq,
> +		      struct idpf_ctlq_info *rx_cq,
> +		      struct cpfl_tdi_rule_info *rinfo,
> +		      int rule_num,
> +		      enum cpfl_tdi_table_entry_op op)
> +{
> +	const struct cpfl_tdi_table_key_obj *kobj;
> +	struct idpf_hw *hw = &itf->adapter->base.hw;
> +	struct cpfl_tdi_ma_hardware_block *hb;
> +	const struct cpfl_tdi_table *table;
> +	int ret = 0;
> +
> +	if (rule_num == 0)
> +		return 0;

Is it necessary to check this value? This function is called with 
nonzero value always.

As a general question, do we need to check input parameters in all these 
static functions? It seems some of them are callbacks and their 
parameters are probably checked by API, but for others it may be the case.

> +
> +	kobj = &rinfo->kobj;
> +
> +	table = kobj->tnode->table;
<snip>
> +static int
> +cpfl_tdi_fxp_rule_destroy(struct rte_eth_dev *dev,
> +			  struct rte_flow *flow,
> +			  struct rte_flow_error *error)
> +{
> +	struct cpfl_itf *itf = CPFL_DEV_TO_ITF(dev);
> +	struct cpfl_adapter_ext *ad = itf->adapter;
> +	struct cpfl_vport *vport;
> +	struct cpfl_repr *repr;
> +	struct cpfl_tdi_rule_info *rinfo = (struct cpfl_tdi_rule_info *)flow->rule;
> +	int ret = 0;
> +	uint32_t cpq_id = 0;
> +
> +	if (itf->type == CPFL_ITF_TYPE_VPORT) {
> +		vport = (struct cpfl_vport *)itf;
> +		cpq_id = vport->base.devarg_id * 2;
> +	} else if (itf->type == CPFL_ITF_TYPE_REPRESENTOR) {
> +		repr = (struct cpfl_repr *)itf;
> +		cpq_id = ((repr->repr_id.pf_id + repr->repr_id.vf_id) & (CPFL_TX_CFGQ_NUM - 1)) * 2;
> +	} else {
> +		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +				   "fail to find correct control queue");
> +		ret = -rte_errno;
> +		goto err;
> +	}
> +
> +	ret = cpfl_tdi_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1], rinfo, 1,
> +				    CPFL_TDI_TABLE_ENTRY_OP_DEL);
> +	if (ret)
> +		goto err;
What's the purpose of this goto here?
> +
> +err:
> +	rte_free(rinfo);
> +	flow->rule = NULL;
> +	return ret;
> +}
> +
> +void
> +cpfl_tdi_free_table_list(struct cpfl_flow_parser *flow_parser)
> +{
> +	struct cpfl_tdi_table_node *node;
Should this parameter be checked since this is non static function?
> +
> +	while ((node = TAILQ_FIRST(&flow_parser->tdi_table_list))) {
> +		TAILQ_REMOVE(&flow_parser->tdi_table_list, node, next);
> +		rte_free(node);
> +	}
> +}
> +
<snip>
> +
> +static int
> +cpfl_tdi_table_key_create(struct rte_eth_dev *dev,
> +			  uint32_t table_id,
> +			  struct cpfl_tdi_table_node **table_node,
> +			  struct cpfl_tdi_table_key_obj *kobj)
> +{
> +	int ret;
> +
> +	if (!kobj)
> +		return -EINVAL;

is it required for static function? in cpfl_tdi_parse_pattern_action() 
kobj is zero inited, so in can not be NULL here. Otherwise it would make 
sense to check all other arguments as well. I'd suggest to replace 
runtime if check with RTE_ASSERT on all input arguments instead.

> +
> +	ret = cpfl_tdi_table_info_get(dev, table_id, table_node);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	ret = cpfl_tdi_table_key_node_init(dev, *table_node, kobj);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +cpfl_tdi_table_key_field_info_get(__rte_unused struct rte_eth_dev *dev,
why not remove this argument is it is unused? Same applied to several 
other functions
> +				  struct cpfl_tdi_table_node *node,
> +				  uint32_t field_id,
> +				  struct cpfl_tdi_table_key_field_info **key_field_info)
> +{
<snip>
> +static int
> +cpfl_tdi_table_key_field_set(struct rte_eth_dev *dev,
> +			     struct cpfl_tdi_table_node *table_node,
> +			     struct cpfl_tdi_table_key_obj *kobj,
> +			     uint32_t field_id,
> +			     const uint8_t *value,
> +			     uint16_t size)
> +{
> +	struct cpfl_tdi_table_key_field_info *key_field_info;
> +	int ret;
> +
> +	if (!kobj || !value)
> +		return -EINVAL;
> +
> +	ret = cpfl_tdi_table_key_field_info_get(dev, table_node, field_id, &key_field_info);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	if (key_field_info->field->match_type != CPFL_TDI_MATCH_TYPE_EXACT)
> +		return -EINVAL;
possible memory leak since cpfl_tdi_table_key_field_info_get() allocated 
memory for key_field_info. Also it would be better to name properly 
functions where memory allocation was done.
> +
> +	if (unlikely(key_field_info->field->is_vsi)) {
No need to use branch predictor hints since this is not a fast path
> +		struct cpfl_itf *src_itf;
> +		uint16_t vsi_id, port_id;
> +		uint8_t ids[2];
> +
> +		if (size != 1 && size != 2) /* input port id should be 8/16 bits */
> +			return -EINVAL;
> +		port_id = size == 1 ? value[0] : value[0] << 8 | value[1];
please use parenthesis for easier reading
> +		src_itf = cpfl_get_itf_by_port_id(port_id);
> +		if (!src_itf)
> +			return -EINVAL;
> +		vsi_id = cpfl_get_vsi_id(src_itf);
> +		if (vsi_id == CPFL_INVALID_HW_ID)
> +			return -EINVAL;
> +		ids[0] = (uint8_t)vsi_id;
> +		ids[1] = (uint8_t)vsi_id >> 8;
> +
> +		ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, ids, size);
I think here it would be safer to set size = 2. otherwise stack overflow 
can happen
> +		if (ret != 0)
> +			return -EINVAL;
> +	} else {
> +		ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, value, size);
> +		if (ret != 0)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
<snip>
> +
> +static inline bool __rte_unused
is this function necessary in this patch series?
> +verify_action(struct cpfl_tdi_table_node *table_node, uint32_t spec_id)
> +{
> +	int i;
> +	const struct cpfl_tdi_table *table = table_node->table;
> +
> +	for (i = 0; i < table->action_num; i++)
> +		if (spec_id == table->actions[i].handle)
> +			return true;
> +
> +	return false;
> +}
> +
> +static int
> +cpfl_tdi_action_node_get_by_spec_id(struct rte_eth_dev *dev,
> +				    uint32_t table_id __rte_unused,
> +				    uint32_t spec_id,
> +				    struct cpfl_tdi_action_node **action_node)
> +{
> +	return cpfl_tdi_action_spec_info_get(dev, spec_id, action_node);
> +}
What's the purpose of this static wrapper function?
> +
> +static int
> +cpfl_tdi_action_spec_field_info_get(struct rte_eth_dev *dev __rte_unused,
> +				    struct cpfl_tdi_action_node *anode,
> +				    uint32_t field_id,
> +				    struct cpfl_tdi_action_spec_field_info **info)
> +{
> +	int ret = -EINVAL;
> +	int i, j;
> +
> +	if (anode->format == NULL)
> +		goto err;
> +
> +	for (i = 0; i < anode->format->immediate_field_num; i++) {
> +		struct cpfl_tdi_immediate_field *field = &anode->format->immediate_fields[i];
> +		struct cpfl_tdi_action_spec_field_info *asfinfo;
> +
> +		if (field->param_handle != field_id)
> +			continue;
> +
> +		asfinfo = rte_malloc(NULL, sizeof(struct cpfl_tdi_action_spec_field_info), 0);
> +		if (asfinfo == NULL) {
> +			ret = -ENOMEM;
> +			goto err;
why not just return from here?
> +		}
> +
> +		asfinfo->field = field;
> +		asfinfo->param = anode->params[i];
> +
<snip>
> +static int
> +_cpfl_tdi_action_field_set(struct rte_eth_dev *dev __rte_unused,
> +			   struct cpfl_tdi_action_obj *aobj,
> +			   struct cpfl_tdi_action_spec_field_info *asfinfo,
> +			   const uint8_t *value,
> +			   uint16_t size,
> +			   enum cpfl_act_fwd_type fwd_type)
> +{
> +	struct cpfl_tdi_action_node *node = aobj->node;
> +	struct cpfl_tdi_param_info *pi = &asfinfo->param;
> +	uint8_t val_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0};
> +	uint8_t msk_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0};
> +
> +	memcpy(val_buf, value, size);
> +
> +	if (node->format->mod_content_format.mod_field_num > 0) {
> +		struct cpfl_tdi_mod_field *mf = asfinfo->mod_field;
> +
> +		cpfl_tdi_shift_left(val_buf, pi->size, mf->start_bit_offset);

how it is guaranteed that pi->size is less than CPFL_TDI_VALUE_SIZE_MAX?

What is the difference between size argument and pi->size here?

> +		cpfl_tdi_init_msk_buf(msk_buf, pi->size, mf->bit_width);
> +		cpfl_tdi_shift_left(msk_buf, pi->size, mf->start_bit_offset);
> +		cpfl_tdi_or_buf(&aobj->buf[pi->offset], pi->size, val_buf, msk_buf);
> +	} else {
> +		struct cpfl_tdi_hw_action *ha = asfinfo->hw_action;
> +		uint32_t action_code = cpfl_tdi_to_action_code(ha, val_buf, fwd_type);
> +
> +		memcpy(&aobj->buf[pi->offset], &action_code, 4);
> +	}
> +
> +	return 0;
why this function returns anything when it always returns 0?
> +}
> +
> +static int
> +cpfl_tdi_action_field_set(struct rte_eth_dev *dev,
> +			  struct cpfl_tdi_action_obj *aobj,
> +			  struct cpfl_tdi_action_node *action_node,
> +			  uint32_t field_id,
> +			  const struct rte_flow_action_prog_argument *arg)
> +{
> +	struct cpfl_tdi_action_spec_field_info *asfinfo;
> +	enum rte_flow_action_type action_type;
> +	/* used when action is PORT_REPRESENTOR type */
> +	struct cpfl_itf *dst_itf;
> +	uint16_t dev_id; /* vsi id */
> +	uint16_t port_id;
> +	uint8_t value;
> +	bool is_vsi;
> +	int ret;
> +	enum cpfl_act_fwd_type fwd_type;
> +
> +	if (!aobj || !arg)
> +		return -EINVAL;
> +
> +	ret = cpfl_tdi_action_spec_field_info_get(dev, action_node, field_id, &asfinfo);

it seems like asfinfo that was allocated inside 
cpfl_tdi_action_spec_field_info_get() is only used locally inside the 
cpfl_tdi_action_field_set(). It should be freed somewhere in this 
function before it returns.

Or as a better solution why not just have this struct as stack 
allocated, don't allocate memory for it at all and just init it inside 
the cpfl_tdi_action_spec_field_info_get()? Otherwise you'll have 
problems with possible memory leak.

> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	if (!strcmp(arg->name, "port_representor"))
> +		action_type = RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR;
> +	else if (!strcmp(arg->name, "represented_port"))
> +		action_type = RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT;
> +	else
> +		return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, arg->value, arg->size,
> +						  CPFL_NO_FWD);
> +
> +	if (arg->size != 1 && arg->size != 2) /* input port_id should be 8/16 bits */
> +		return -EINVAL;
same problem with memory leak after cpfl_tdi_action_spec_field_info_get()
> +
> +	port_id = arg->size == 1 ? arg->value[0] : arg->value[0] << 8 | arg->value[1];
> +	dst_itf = cpfl_get_itf_by_port_id(port_id);
> +	if (!dst_itf)
> +		goto err;
> +
> +	is_vsi = (action_type == RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR ||
> +		  dst_itf->type == CPFL_ITF_TYPE_REPRESENTOR);
> +	if (is_vsi)
> +		dev_id = cpfl_get_vsi_id(dst_itf);
> +	else
> +		dev_id = cpfl_get_port_id(dst_itf);
> +
> +	if (dev_id == CPFL_INVALID_HW_ID)
> +		goto err;
> +
> +	value = (uint8_t)dev_id;
> +	fwd_type = is_vsi ? CPFL_ACT_FWD_VSI : CPFL_ACT_FWD_PORT;
> +
> +	return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, &value, arg->size, fwd_type);
> +err:
> +	PMD_DRV_LOG(ERR, "Can not get dev id.");
> +	return -EINVAL;
> +}
> +
> +int
> +cpfl_tdi_build(struct cpfl_flow_parser *flow_parser)
> +{
> +	int ret;
> +
for non static function input arguments should probably be checked.
> +	ret = cpfl_tdi_build_table_list(flow_parser);
> +	if (ret != 0) {
> +		PMD_INIT_LOG(ERR, "Failed to build tdi table list");
> +		return ret;
> +	}
> +
<snip>
> +static int
> +cpfl_tdi_parse_action(struct rte_eth_dev *dev,
> +		      const struct rte_flow_action actions[],
> +		      uint32_t table_id,
> +		      struct cpfl_tdi_table_node *table_node,
> +		      struct cpfl_tdi_action_node **action_node,
> +		      struct cpfl_tdi_action_obj *aobj)
> +{
> +	const struct rte_flow_action_prog *prog;
> +	const struct rte_flow_action_prog_argument *arg;
> +	uint32_t action_spec_id;
> +	int i, ret;
> +	uint32_t j;
> +
> +	for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
> +		if (actions[i].type != RTE_FLOW_ACTION_TYPE_PROG)
> +			continue;
> +
> +		prog = actions[i].conf;
> +		action_spec_id = atoi(prog->name);
> +		/* Get action node */
> +		ret =
> +		    cpfl_tdi_action_node_get_by_spec_id(dev, table_id, action_spec_id, action_node);
> +		if (ret) {
> +			PMD_INIT_LOG(ERR, "Failed to get action node.");
> +			return -EINVAL;
> +		}
> +
> +		ret = cpfl_tdi_action_obj_init(dev, table_node, *action_node, aobj);
> +		if (ret != 0) {
nothing can happen inside cpfl_tdi_action_obj_init(), it always returns 0
> +			PMD_INIT_LOG(ERR, "Failed to init action obj.");
> +			return -EINVAL;
> +		}
> +
> +		for (j = 0; j < prog->args_num; j++) {
> +			arg = &prog->args[j];
> +			/* Set the action fields */
> +			ret = cpfl_tdi_action_field_set(dev, aobj, *action_node, j, arg);
> +			if (ret) {
> +				PMD_INIT_LOG(ERR, "Failed to set action field.");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
<snip>
> -- 

Regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 23809 bytes --]