Re: [dpdk-dev] [RFC 1/7] ethdev: support sub function representor

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 12/18/20 5:55 PM, Xueming Li wrote:
> To support SF representor, this patch enhances devargs syntax:
> 
> [pf#]vf#: new VF port representor/s, example: vf[0-3], pf0vf3
> [pf#]sf#: new SF port representor/s, example: sf[0-3], pf1sf2
> 
> pf# is optional for SF and VF representor. If not present, representor
> is detected from PCI BDF of device argument. In case of bonding, pf#
> indicates owner PF device of SF or VF.
> 
> For backwards compatible, # default to vf#.
> 
> Enhances rte_eth_devargs_parse_representor_port() function to parse new
> representor syntax, replace rte_eth_devargs_parse_list() function which
> only parse simple list.

Consider to split it into few patches:
 1. rework infrastructure/functions, introduce enum, support old syntax only
 2. add new syntax for VFs
 3. add SubFunctions support, add enum member (don't forget to
    update existing drivers to reject if SF is requested)
 4. add PFs support, add enum member (also don't forget to
    update existing drivers as well)

> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  lib/librte_ethdev/ethdev_private.c    | 115 ++++++++++++++------------
>  lib/librte_ethdev/ethdev_private.h    |   3 -
>  lib/librte_ethdev/rte_class_eth.c     |   7 +-
>  lib/librte_ethdev/rte_ethdev.c        |   5 +-
>  lib/librte_ethdev/rte_ethdev_driver.h |   9 ++
>  5 files changed, 77 insertions(+), 62 deletions(-)
> 
> diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
> index 162a502fe7..2a057c5f36 100644
> --- a/lib/librte_ethdev/ethdev_private.c
> +++ b/lib/librte_ethdev/ethdev_private.c
> @@ -38,60 +38,13 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
>  	return NULL;
>  }
>  
> -int
> -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
> -	void *data)
> -{
> -	char *str_start;
> -	int state;
> -	int result;
> -
> -	if (*str != '[')
> -		/* Single element, not a list */
> -		return callback(str, data);
> -
> -	/* Sanity check, then strip the brackets */
> -	str_start = &str[strlen(str) - 1];
> -	if (*str_start != ']') {
> -		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
> -		return -EINVAL;
> -	}
> -	str++;
> -	*str_start = '\0';
> -
> -	/* Process list elements */
> -	state = 0;
> -	while (1) {
> -		if (state == 0) {
> -			if (*str == '\0')
> -				break;
> -			if (*str != ',') {
> -				str_start = str;
> -				state = 1;
> -			}
> -		} else if (state == 1) {
> -			if (*str == ',' || *str == '\0') {
> -				if (str > str_start) {
> -					/* Non-empty string fragment */
> -					*str = '\0';
> -					result = callback(str_start, data);
> -					if (result < 0)
> -						return result;
> -				}
> -				state = 0;
> -			}
> -		}
> -		str++;
> -	}
> -	return 0;
> -}
> -
>  static int
>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>  	const uint16_t max_list)
>  {
>  	uint16_t lo, hi, val;
>  	int result;
> +	char *pos = str;
>  
>  	result = sscanf(str, "%hu-%hu", &lo, &hi);
>  	if (result == 1) {
> @@ -99,7 +52,7 @@ rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>  			return -ENOMEM;
>  		list[(*len_list)++] = lo;
>  	} else if (result == 2) {
> -		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)
> +		if (lo >= hi)
>  			return -EINVAL;
>  		for (val = lo; val <= hi; val++) {
>  			if (*len_list >= max_list)
> @@ -108,14 +61,74 @@ rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>  		}
>  	} else
>  		return -EINVAL;
> -	return 0;
> +	while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))
> +		pos++;
> +	return pos - str;
>  }
>  
> +static int
> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
> +	const uint16_t max_list)
> +{
> +	char *pos = str;
> +	int ret;
> +
> +	if (*pos == '[')
> +		pos++;
> +	while (1) {
> +		ret = rte_eth_devargs_process_range(pos, list, len_list,
> +						    max_list);
> +		if (ret < 0)
> +			return ret;
> +		pos += ret;
> +		if (*pos != ',') /* end of list */
> +			break;
> +		pos++;
> +	}
> +	if (*str == '[' && *pos != ']')
> +		return -EINVAL;
> +	if (*pos == ']')
> +		pos++;
> +	return pos - str;
> +}
> +
> +/*
> + * representor format:
> + *   #: range or single number of VF representor - legacy
> + *   [pf#]vf#: VF port representor/s
> + *   [pf#]sf#: SF port representor/s
> + */
>  int
>  rte_eth_devargs_parse_representor_ports(char *str, void *data)
>  {
>  	struct rte_eth_devargs *eth_da = data;
> +	int ret;
>  
> -	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
> +	eth_da->type = RTE_ETH_REPRESENTOR_NONE;
> +	/* parse pf# */
> +	if (str[0] == 'p' && str[1] == 'f') {
> +		str += 2;
> +		eth_da->type = RTE_ETH_REPRESENTOR_PF;

Just to be consistent with similar lines below, please,
swap these two lines.

> +		ret = rte_eth_devargs_process_list(str, eth_da->ports,
> +				&eth_da->nb_ports, RTE_MAX_ETHPORTS);

I'm not sure that it matches ports and nb_ports fields purpose.

In any case it should update existing drivers which support
representors to reject devargs with PFs specified.

> +		if (ret < 0)
> +			goto err;
> +		str += ret;
> +	}
> +	/* parse vf# and sf#, number # alone implies VF */
> +	if (str[0] == 'v'  && str[1] == 'f') {
> +		eth_da->type = RTE_ETH_REPRESENTOR_VF;
> +		str += 2;
> +	} else if (str[0] == 's'  && str[1] == 'f') {
> +		eth_da->type = RTE_ETH_REPRESENTOR_SF;
> +		str += 2;
> +	} else {
> +		eth_da->type = RTE_ETH_REPRESENTOR_VF;
> +	}
> +	ret = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>  		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
> +err:
> +	if (ret < 0)
> +		RTE_LOG(ERR, EAL, "wrong representor format: %s", str);

Missing \n

> +	return ret < 0 ? ret : 0;
>  }
> diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
> index 905a45c337..220ddd4408 100644
> --- a/lib/librte_ethdev/ethdev_private.h
> +++ b/lib/librte_ethdev/ethdev_private.h
> @@ -26,9 +26,6 @@ eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
>  		const void *data);
>  
>  /* Parse devargs value for representor parameter. */
> -typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
> -int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
> -	void *data);
>  int rte_eth_devargs_parse_representor_ports(char *str, void *data);
>  
>  #ifdef __cplusplus
> diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
> index 6338355e25..7da06808ea 100644
> --- a/lib/librte_ethdev/rte_class_eth.c
> +++ b/lib/librte_ethdev/rte_class_eth.c
> @@ -66,7 +66,7 @@ eth_representor_cmp(const char *key __rte_unused,
>  	int ret;
>  	char *values;
>  	const struct rte_eth_dev_data *data = opaque;
> -	struct rte_eth_devargs representors;
> +	struct rte_eth_devargs representors = { 0 };
>  	uint16_t index;
>  
>  	if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
> @@ -76,10 +76,7 @@ eth_representor_cmp(const char *key __rte_unused,
>  	values = strdup(value);
>  	if (values == NULL)
>  		return -1;
> -	memset(&representors, 0, sizeof(representors));
> -	ret = rte_eth_devargs_parse_list(values,
> -			rte_eth_devargs_parse_representor_ports,
> -			&representors);
> +	ret = rte_eth_devargs_parse_representor_ports(values, &representors);
>  	free(values);
>  	if (ret != 0)
>  		return -1; /* invalid devargs value */
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 17ddacc78d..2ac51ac149 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5542,9 +5542,8 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
>  	for (i = 0; i < args.count; i++) {
>  		pair = &args.pairs[i];
>  		if (strcmp("representor", pair->key) == 0) {
> -			result = rte_eth_devargs_parse_list(pair->value,
> -				rte_eth_devargs_parse_representor_ports,
> -				eth_da);
> +			result = rte_eth_devargs_parse_representor_ports(
> +					pair->value, eth_da);
>  			if (result < 0)
>  				goto parse_cleanup;
>  		}
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 0eacfd8425..edb000cbd4 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -1193,6 +1193,14 @@ __rte_internal
>  int
>  rte_eth_switch_domain_free(uint16_t domain_id);
>  
> +/** Ethernet device representor type */
> +enum rte_eth_representor_type {
> +	RTE_ETH_REPRESENTOR_NONE, /* not a representor */
> +	RTE_ETH_REPRESENTOR_VF,   /* representor of VF */
> +	RTE_ETH_REPRESENTOR_SF,   /* representor of SF */
> +	RTE_ETH_REPRESENTOR_PF,   /* representor of host PF */
> +};
> +
>  /** Generic Ethernet device arguments  */
>  struct rte_eth_devargs {
>  	uint16_t ports[RTE_MAX_ETHPORTS];
> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
>  	/** representor port/s identifier to enable on device */
>  	uint16_t nb_representor_ports;
>  	/** number of ports in representor port field */
> +	enum rte_eth_representor_type type; /* type of representor */
>  };
>  
>  /**
>