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

[Yongseok Koh <yskoh@mellanox.com>]

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

> + *
> + * @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.

> + */
> +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().

> +	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.

> +	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.

> +	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.

> +	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.

> +	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?

> +	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;

> +	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?

>   */
>  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.

> +		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?

>  		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
>