Re: [dpdk-dev] [PATCH 4/9] net/mlx5: add validation for Direct Rule E-Switch

[Ori Kam <orika@mellanox.com>]

Hi Koh,
PSB

> -----Original Message-----
> From: Yongseok Koh
> Sent: Thursday, April 18, 2019 3:00 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Matan Azrad
> <matan@mellanox.com>; Slava Ovsiienko <viacheslavo@mellanox.com>; Moti
> Haimovsky <motih@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH 4/9] net/mlx5: add validation for Direct Rule E-Switch
> 
> On Sun, Apr 14, 2019 at 09:12:32PM +0000, Ori Kam wrote:
> > Add validation logic for E-Switch using Direct Rules.
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5.h         |   2 +
> >  drivers/net/mlx5/mlx5_ethdev.c  |  39 +++++++
> >  drivers/net/mlx5/mlx5_flow.h    |   5 +
> >  drivers/net/mlx5/mlx5_flow_dv.c | 252
> ++++++++++++++++++++++++++++++++++++++--
> >  4 files changed, 287 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> > index 33a4127..8d63575 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -412,6 +412,8 @@ int mlx5_ibv_device_to_pci_addr(const struct
> ibv_device *device,
> >  unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
> >  				 uint16_t *port_list,
> >  				 unsigned int port_list_n);
> > +int mlx5_port_to_eswitch_info(uint16_t port, uint16_t *es_domain_id,
> > +			      uint16_t *es_port_id);
> >  int mlx5_sysfs_switch_info(unsigned int ifindex,
> >  			   struct mlx5_switch_info *info);
> >  bool mlx5_translate_port_name(const char *port_name_in,
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c
> > index 3992918..c821772 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1376,6 +1376,45 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
> >  }
> >
> >  /**
> > + * Get the e-switch domain id this port belongs to.
> 
> E-Switch

Will fix.
> 
> > + *
> > + * @param[in] port
> > + *   Device port id.
> > + * @param[out] es_domain_id
> > + *   e-switch domain id.
> 
> E-Switch
> Please correct in the entire patchset.
> 
> > + * @param[out] es_port_id
> > + *   The port id of the port in the switch.
> > + *
> > + * @return
> > + *   0 on success, Negative error otherwise.
> 
> From looking at the use-cases below, rte_errno must be set.
> 


Will fix.

> > + */
> > +int
> > +mlx5_port_to_eswitch_info(uint16_t port,
> > +			  uint16_t *es_domain_id, uint16_t *es_port_id)
> > +{
> > +	struct rte_eth_dev *dev;
> > +	struct mlx5_priv *priv;
> > +
> > +	if (port >= RTE_MAX_ETHPORTS)
> > +		return -EINVAL;
> > +	dev = &rte_eth_devices[port];
> > +	if (dev == NULL)
> > +		return -ENODEV;
> 
> dev is an l-value, it cannot be null.
> The above two checks can be replaced with rte_eth_dev_is_valid_port().
> 

Will  fix.

> > +	if (!dev->device ||
> > +	    !dev->device->driver ||
> > +	    strcmp(dev->device->driver->name, MLX5_DRIVER_NAME))
> > +		return -EINVAL;
> 
> Looks too paranoid. The function is just PMD-internal.
> 

Will delete.

> > +	priv = dev->data->dev_private;
> > +	if (!(priv->representor || priv->master))
> > +		return -EINVAL;
> > +	if (es_domain_id)
> > +		*es_domain_id = priv->domain_id;
> > +	if (es_port_id)
> > +		*es_port_id = priv->vport_id;
> 
> It is okay for now but we need to define a new struct like esw_info next time.
> This info should be grouped in the priv.
> 

I think I need a new member to mark it is pf or vd, can we in this patch set add it as is?
I will create a patch to move all of the E-Switch variables to dedicated structure for next version.

> > +	return 0;
> > +}
> > +
> > +/**
> >   * Get switch information associated with network interface.
> >   *
> >   * @param ifindex
> > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> > index 9f47fd4..85954c2 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -48,6 +48,7 @@
> >
> >  /* General pattern items bits. */
> >  #define MLX5_FLOW_ITEM_METADATA (1u << 16)
> > +#define MLX5_FLOW_ITEM_PORT_ID (1u << 17)
> >
> >  /* Outer Masks. */
> >  #define MLX5_FLOW_LAYER_OUTER_L3 \
> > @@ -118,6 +119,10 @@
> >  	(MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | \
> >  	 MLX5_FLOW_ACTION_RSS | MLX5_FLOW_ACTION_JUMP)
> >
> > +#define MLX5_FLOW_FATE_ESWITCH_ACTIONS \
> > +	(MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_PORT_ID | \
> > +	 MLX5_FLOW_ACTION_JUMP)
> > +
> >  #define MLX5_FLOW_ENCAP_ACTIONS
> 	(MLX5_FLOW_ACTION_VXLAN_ENCAP | \
> >  				 MLX5_FLOW_ACTION_NVGRE_ENCAP | \
> >  				 MLX5_FLOW_ACTION_RAW_ENCAP)
> > diff --git a/drivers/net/mlx5/mlx5_flow_dv.c
> b/drivers/net/mlx5/mlx5_flow_dv.c
> > index 7b582f0..fedc6cb 100644
> > --- a/drivers/net/mlx5/mlx5_flow_dv.c
> > +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> > @@ -613,6 +613,92 @@ struct field_modify_info modify_tcp[] = {
> >  }
> >
> >  /**
> > + * Validate vport item.
> > + *
> > + * @param[in] dev
> > + *   Pointer to the rte_eth_dev structure.
> > + * @param[in] item
> > + *   Item specification.
> > + * @param[in] attr
> > + *   Attributes of flow that includes this item.
> > + * @param[in] item_flags
> > + *   Bit-fields that holds the items detected until now.
> > + * @param[out] error
> > + *   Pointer to error structure.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +static int
> > +flow_dv_validate_item_port_id(struct rte_eth_dev *dev,
> > +			      const struct rte_flow_item *item,
> > +			      const struct rte_flow_attr *attr,
> > +			      uint64_t item_flags,
> > +			      struct rte_flow_error *error)
> > +{
> > +	const struct rte_flow_item_port_id *spec = item->spec;
> > +	const struct rte_flow_item_port_id *mask = item->mask;
> > +	const struct rte_flow_item_port_id switch_mask = {
> > +			.id = 0xffffffff,
> > +	};
> > +	uint16_t esw_domain_id;
> > +	uint16_t item_port_esw_domain_id;
> > +	uint16_t item_port_esw_port_id;
> > +	uint16_t port;
> > +	int ret;
> > +
> > +	if (!attr->transfer)
> > +		return rte_flow_error_set(error, EINVAL,
> > +					  RTE_FLOW_ERROR_TYPE_ITEM,
> > +					  NULL,
> > +					  "match on port id is valid for"
> > +					  " eswitch only");
> 
> Need to mention about 'transfer' flag here instead of esw. BTW, is it okay to
> speak in PMD specific language for error messages? Even if so, 'eswitch' should
> be fixed. Please re-visit all the error messages again.
> 

I think the correct message should be that the eswitch  should be replaced by E-Switch
Other option is to say something like this "matching on port id must have the transfer attribute"
Which do you prefer?

> > +	if (item_flags & MLX5_FLOW_ITEM_PORT_ID)
> > +		return rte_flow_error_set(error, ENOTSUP,
> > +					  RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> > +					  "multiple source vport are not"
> > +					  " supported");
> 
> Same here. 'vport' doesn't look appropriate.
>

Will change from vport to ports.
 
> > +	if (!mask)
> > +		mask = &switch_mask;
> > +	if (mask->id && mask->id != 0xffffffff)
> > +		return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ITEM_MASK,
> > +					   mask,
> > +					   "no support for partial mask on"
> > +					   " \"id\" field");
> > +	ret = mlx5_flow_item_acceptable
> > +				(item, (const uint8_t *)mask,
> > +				 (const uint8_t
> *)&rte_flow_item_port_id_mask,
> > +				 sizeof(struct rte_flow_item_port_id),
> > +				 error);
> > +	if (ret)
> > +		return ret;
> > +	if (!spec)
> > +		return 0;
> > +	port = mask->id ? spec->id : 0;
> 
> Non-masked value means 'any' value. Is it correct to set port 0 in
> this case?
> 

Will check again.

> > +	ret = mlx5_port_to_eswitch_info(port, &item_port_esw_domain_id,
> > +					&item_port_esw_port_id);
> 
> item_port_esw_port_id is of no use here;
> 

Will remove the variable.

> > +	if (ret)
> > +		return rte_flow_error_set(error, -ret,
> > +
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC, spec,
> > +					  "failed to obtain eswitch info for"
> > +					  " port");
> > +	ret = mlx5_port_to_eswitch_info(dev->data->port_id,
> > +					&esw_domain_id, NULL);
> > +	if (ret < 0)
> > +		return rte_flow_error_set(error, -ret,
> > +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +					  NULL,
> > +					  "failed to obtain eswitch info");
> > +	if (item_port_esw_domain_id != esw_domain_id)
> > +		return rte_flow_error_set(error, -ret,
> > +
> RTE_FLOW_ERROR_TYPE_ITEM_SPEC, spec,
> > +					  "cannot match on a port from a"
> > +					  " different eswitch");
> > +	return 0;
> > +}
> > +
> > +/**
> >   * Validate count action.
> >   *
> >   * @param[in] dev
> > @@ -622,6 +708,7 @@ struct field_modify_info modify_tcp[] = {
> >   *
> >   * @return
> >   *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + *   w
> 
> What is this change?
> 

Will fix

> >   */
> >  static int
> >  flow_dv_validate_action_count(struct rte_eth_dev *dev,
> > @@ -676,7 +763,7 @@ struct field_modify_info modify_tcp[] = {
> >  					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> >  					  "can only have a single encap or"
> >  					  " decap action in a flow");
> > -	if (attr->ingress)
> > +	if (!attr->transfer && attr->ingress)
> >  		return rte_flow_error_set(error, ENOTSUP,
> >
> RTE_FLOW_ERROR_TYPE_ATTR_INGRESS,
> >  					  NULL,
> > @@ -761,7 +848,8 @@ struct field_modify_info modify_tcp[] = {
> >  					  "can only have a single encap"
> >  					  " action in a flow");
> >  	/* encap without preceding decap is not supported for ingress */
> > -	if (attr->ingress && !(action_flags &
> MLX5_FLOW_ACTION_RAW_DECAP))
> > +	if (!attr->transfer &&  attr->ingress &&
> > +	    !(action_flags & MLX5_FLOW_ACTION_RAW_DECAP))
> >  		return rte_flow_error_set(error, ENOTSUP,
> >
> RTE_FLOW_ERROR_TYPE_ATTR_INGRESS,
> >  					  NULL,
> > @@ -1561,6 +1649,77 @@ struct field_modify_info modify_tcp[] = {
> >  	return 0;
> >  }
> >
> > +/*
> > + * Validate the port_id action.
> > + *
> > + * @param[in] dev
> > + *   Pointer to rte_eth_dev structure.
> > + * @param[in] action_flags
> > + *   Bit-fields that holds the actions detected until now.
> > + * @param[in] action
> > + *   Port_id RTE action structure.
> > + * @param[in] attr
> > + *   Attributes of flow that includes this action.
> > + * @param[out] error
> > + *   Pointer to error structure.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +static int
> > +flow_dv_validate_action_port_id(struct rte_eth_dev *dev,
> > +				uint64_t action_flags,
> > +				const struct rte_flow_action *action,
> > +				const struct rte_flow_attr *attr,
> > +				struct rte_flow_error *error)
> > +{
> > +	const struct rte_flow_action_port_id *port_id;
> > +	uint16_t port;
> > +	uint16_t esw_domain_id;
> > +	uint16_t act_port_domain_id;
> > +	int ret;
> > +
> > +	if (!attr->transfer)
> > +		return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +					  NULL,
> > +					  "port id action is valid in transfer"
> > +					  " mode only");
> > +	if (!action || !action->conf)
> > +		return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > +					  NULL,
> > +					  "port id action parameters must be"
> > +					  " specified");
> > +	if (action_flags & (MLX5_FLOW_FATE_ACTIONS |
> > +			    MLX5_FLOW_FATE_ESWITCH_ACTIONS))
> > +		return rte_flow_error_set(error, EINVAL,
> > +					  RTE_FLOW_ERROR_TYPE_ACTION,
> NULL,
> > +					  "can have only one fate actions in"
> > +					  " a flow");
> > +	ret = mlx5_port_to_eswitch_info(dev->data->port_id,
> > +					&esw_domain_id, NULL);
> > +	if (ret < 0)
> > +		return rte_flow_error_set(error, -ret,
> > +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +					  NULL,
> > +					  "failed to obtain eswitch info");
> > +	port_id = action->conf;
> > +	port = port_id->original ? dev->data->port_id : port_id->id;
> > +	ret = mlx5_port_to_eswitch_info(port, &act_port_domain_id, NULL);
> > +	if (ret)
> > +		return rte_flow_error_set
> > +				(error, -ret,
> > +				 RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> port_id,
> > +				 "failed to obtain eswitch port-id for port");
> > +	if (act_port_domain_id != esw_domain_id)
> > +		return rte_flow_error_set
> > +				(error, -ret,
> > +				 RTE_FLOW_ERROR_TYPE_ACTION, NULL,
> > +				 "port does not belong to"
> > +				 " eswitch being configured");
> > +	return 0;
> > +}
> >
> >  /**
> >   * Find existing modify-header resource or create and register a new one.
> > @@ -1759,11 +1918,34 @@ struct field_modify_info modify_tcp[] = {
> >
> RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY,
> >  					  NULL,
> >  					  "priority out of range");
> > -	if (attributes->transfer)
> > -		return rte_flow_error_set(error, ENOTSUP,
> > -
> RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER,
> > -					  NULL,
> > -					  "transfer is not supported");
> > +	if (attributes->transfer) {
> > +		if (!priv->config.dv_eswitch_en)
> > +			return rte_flow_error_set
> > +						(error, ENOTSUP,
> > +
> 	RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +						NULL,
> > +						"eswitch dr is not supported");
> 
> If you open a parenthesis in a new line, you should indent by a tab.
> 

Missed that, fill fix.

> > +		if (!(priv->representor || priv->master))
> > +			return rte_flow_error_set
> > +					(error, EINVAL,
> > +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +					 NULL,
> > +					 "eswitch configurationd can only be"
> > +					 " done by a master or a representor"
> > +					 " device");
> > +		if (attributes->egress)
> > +			return rte_flow_error_set
> > +					(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ATTR_EGRESS,
> > +					 attributes, "egress is not supported");
> > +		if (attributes->group >= MLX5_MAX_TABLES_FDB)
> > +			return rte_flow_error_set
> > +				       (error, EINVAL,
> > +
> 	RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER,
> > +					NULL,
> > +					"group must be smaller than "
> > +					RTE_STR(MLX5_MAX_FDB_TABLES));
> > +	}
> >  	if (!(attributes->egress ^ attributes->ingress))
> >  		return rte_flow_error_set(error, ENOTSUP,
> >  					  RTE_FLOW_ERROR_TYPE_ATTR,
> NULL,
> > @@ -1812,6 +1994,13 @@ struct field_modify_info modify_tcp[] = {
> >  		switch (items->type) {
> >  		case RTE_FLOW_ITEM_TYPE_VOID:
> >  			break;
> > +		case RTE_FLOW_ITEM_TYPE_PORT_ID:
> > +			ret = flow_dv_validate_item_port_id
> > +					(dev, items, attr, item_flags, error);
> > +			if (ret < 0)
> > +				return ret;
> > +			item_flags |= MLX5_FLOW_ITEM_PORT_ID;
> > +			break;
> 
> Shouldn't it use last_item?
> 

Good question, should the port id be in some specific order? 
We can force it to be first or last the which makes sense.
I will change to last_item.

 
> >  		case RTE_FLOW_ITEM_TYPE_ETH:
> >  			ret = mlx5_flow_validate_item_eth(items, item_flags,
> >  							  error);
> > @@ -1943,6 +2132,17 @@ struct field_modify_info modify_tcp[] = {
> >  		switch (actions->type) {
> >  		case RTE_FLOW_ACTION_TYPE_VOID:
> >  			break;
> > +		case RTE_FLOW_ACTION_TYPE_PORT_ID:
> > +			ret = flow_dv_validate_action_port_id(dev,
> > +							      action_flags,
> > +							      actions,
> > +							      attr,
> > +							      error);
> > +			if (ret)
> > +				return ret;
> > +			action_flags |= MLX5_FLOW_ACTION_PORT_ID;
> > +			++actions_n;
> > +			break;
> >  		case RTE_FLOW_ACTION_TYPE_FLAG:
> >  			ret = mlx5_flow_validate_action_flag(action_flags,
> >  							     attr, error);
> > @@ -2133,10 +2333,40 @@ struct field_modify_info modify_tcp[] = {
> >  						  "action not supported");
> >  		}
> >  	}
> > -	if (!(action_flags & MLX5_FLOW_FATE_ACTIONS) && attr->ingress)
> > -		return rte_flow_error_set(error, EINVAL,
> > -					  RTE_FLOW_ERROR_TYPE_ACTION,
> actions,
> > -					  "no fate action is found");
> > +	/* Eswitch has few restrictions on using items and actions */
> > +	if (attr->transfer) {
> > +		if (action_flags & MLX5_FLOW_ACTION_FLAG)
> > +			return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  NULL,
> > +						  "unsupported action FLAG");
> > +		if (action_flags & MLX5_FLOW_ACTION_MARK)
> > +			return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  NULL,
> > +						  "unsupported action MARK");
> > +		if (action_flags & MLX5_FLOW_ACTION_QUEUE)
> > +			return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  NULL,
> > +						  "unsupported action
> QUEUE");
> > +		if (action_flags & MLX5_FLOW_ACTION_RSS)
> > +			return rte_flow_error_set(error, ENOTSUP,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  NULL,
> > +						  "unsupported action RSS");
> > +		if (!(action_flags & MLX5_FLOW_FATE_ESWITCH_ACTIONS))
> > +			return rte_flow_error_set(error, EINVAL,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  actions,
> > +						  "no fate action is found");
> > +	} else {
> > +		if (!(action_flags & MLX5_FLOW_FATE_ACTIONS) && attr-
> >ingress)
> > +			return rte_flow_error_set(error, EINVAL,
> > +
> RTE_FLOW_ERROR_TYPE_ACTION,
> > +						  actions,
> > +						  "no fate action is found");
> > +	}
> >  	return 0;
> >  }
> >
> > --
> > 1.8.3.1
> >