Re: [dpdk-dev] [PATCH v12 1/2] i40e: support floating VEB config

[Ferruh Yigit <ferruh.yigit@intel.com>]

Hi Zhe,

On 6/24/2016 9:29 AM, Zhe Tao wrote:
> Add the new floating VEB related arguments option in the devarg.
> Using this parameter, all the applications can decide whether to use legacy
> VEB/VEPA or floating VEB.
> To enable this feature, the user should pass a devargs parameter to the
> EAL like "-w 84:00.0,enable_floating_veb=1", and the application will
> tell PMD whether to use the floating VEB feature or not.
Is providing the enable_floating_veb=1 devarg enough, or should
application call an driver API to enable this feature, if so documenting
what application needs to do helps app developers.

> Once the floating VEB feature is enabled, all the VFs created by
> this PF device are connected to the floating VEB.
Technically there can be multiple floating VEBs, right? And with this
param only one floating VEB created and all VFs connected to it. Is
there any use case to create multiple VEBs with selective VFs connected
to them?

> 
> Also user can specify which VF need to connect to this floating veb using
> "floating_veb_list".
> Like "-w 84:00.0,enable_floating_veb=1,floating_veb_list=1/3-4", means VF1, VF3,
> VF4 connect to the floating VEB, other VFs connect to the legacy VEB.The "/"
> is used for delimiter of the floating VEB list.
Is there a use case to change VF VEB connection on runtime?

> 
> All the VEB/VEPA concepts are not specific for FVL, they are defined in
> the 802.1Qbg spec.
> 
> But for floating VEB, it has two major difference.
> 1. doesn't has a up link connection which means
> the traffic cannot go to outside world.
> 2. doesn't need to connect to the physical port which means
> when the physical link is down the floating VEB can still works
> fine.
> 
> Signed-off-by: Zhe Tao <zhe.tao@intel.com>
> ---

...

>  
> +static int vf_floating_veb_handler(__rte_unused const char *key,
What about floating_veb_list_handler, to be consistent with argument name?

> +				   const char *floating_veb_value,
> +				   void *opaque)
> +{
> +	int idx = 0;
> +	unsigned count = 0;
> +	char *end = NULL;
> +	int min, max;
> +	bool *vf_floating_veb = opaque;
> +
> +	while (isblank(*floating_veb_value))
> +		floating_veb_value++;
> +
> +	/* Reset floating VEB configuration for VFs */
> +	for (idx = 0; idx < I40E_MAX_VF; idx++)
> +		vf_floating_veb[idx] = false;
> +
> +	min = I40E_MAX_VF;
> +	do {
> +		while (isblank(*floating_veb_value))
> +			floating_veb_value++;
> +		if (*floating_veb_value == '\0')
> +			return -1;
> +		errno = 0;
> +		idx = strtoul(floating_veb_value, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = idx;
> +		} else if ((*end == '/') || (*end == '\0')) {
> +			max = idx;
> +			if (min == I40E_MAX_VF)
> +				min = idx;
> +			if (max >= I40E_MAX_VF)
> +				max = I40E_MAX_VF - 1;
> +			for (idx = min; idx <= max; idx++) {
> +				vf_floating_veb[idx] = true;
> +				count++;
> +			}
> +			min = I40E_MAX_VF;
> +		} else {
> +			return -1;
> +		}
> +		floating_veb_value = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void config_vf_floating_veb(struct rte_devargs *devargs,
According DPDK coding convention, return type should be in a separate line

> +				   uint16_t floating,
floating itself confusing, what about floating_veb?

> +				   bool *vf_floating_veb)
> +{
> +	struct rte_kvargs *kvlist;
> +	int i;
> +	const char *floating_veb_list = ETH_I40E_FLOATING_VEB_LIST_ARG;
> +
> +	if (floating == false)
> +		return;
> +	for (i = 0; i < I40E_MAX_VF; i++)
> +		vf_floating_veb[i] = true;
> +
> +	if (devargs == NULL)
> +		return;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, NULL);
> +	if (kvlist == NULL)
> +		return;
> +
> +	if (!rte_kvargs_count(kvlist, floating_veb_list)) {
> +		rte_kvargs_free(kvlist);
> +		return;
> +	}
> +
> +	if (rte_kvargs_process(kvlist, floating_veb_list,
> +			       vf_floating_veb_handler,
> +			       vf_floating_veb) < 0) {
A comment can be good to say, here floating VEB disabled for all VFs

> +		rte_kvargs_free(kvlist);
> +		return;
> +	}
> +	rte_kvargs_free(kvlist);
> +
> +	return;
not required

> +}
> +
> +static int i40e_check_floating_handler(__rte_unused const char *key,
i40e_check_floating_veb_handler?

> +				       const char *value,
> +				       __rte_unused void *opaque)
> +{
> +	if (strcmp(value, "1"))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int
> +floating_veb_supported(struct rte_devargs *devargs)
It can be good add a verb to the function name,

> +{
> +	struct rte_kvargs *kvlist;
> +	const char *floating_veb_key = ETH_I40E_FLOATING_VEB_ARG;
> +
> +	if (devargs == NULL)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, NULL);
> +	if (kvlist == NULL)
> +		return 0;
> +
> +	if (!rte_kvargs_count(kvlist, floating_veb_key)) {
> +		rte_kvargs_free(kvlist);
> +		return 0;
> +	}
> +	/* Floating VEB is enabled when there's key-value:
> +	 * enable_floating_veb=1
> +	 */
> +	if (rte_kvargs_process(kvlist, floating_veb_key,
> +			       i40e_check_floating_handler, NULL) < 0) {
> +		rte_kvargs_free(kvlist);
> +		return 0;
> +	}
> +	rte_kvargs_free(kvlist);
> +
> +	return 1;
> +}
> +
>  static int
>  eth_i40e_dev_init(struct rte_eth_dev *dev)
>  {
> @@ -843,6 +976,15 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
>  		     ((hw->nvm.version >> 4) & 0xff),
>  		     (hw->nvm.version & 0xf), hw->nvm.eetrack);
>  
> +	/* Need the special FW version support floating VEB */
> +	memset(pf->vf_floating_veb, 0, sizeof(pf->vf_floating_veb));
> +	if (hw->aq.fw_maj_ver >= FLOATING_VEB_SUPPORTED_FW_MAJ) {
Does it make sense to move this check into a function, like
is_floating_veb_supported()? This makes what the criteria of floating
veb support more clear, and hides that detail from this piece of code.


> +		pf->floating_veb = floating_veb_supported(pci_dev->devargs);
> +		config_vf_floating_veb(pci_dev->devargs, pf->floating_veb,
> +				       pf->vf_floating_veb);
> +	} else {
> +		pf->floating_veb = false;
> +	}
>  	/* Clear PXE mode */
>  	i40e_clear_pxe_mode(hw);
>  
> diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
> index cfd2399..71bca65 100644
> --- a/drivers/net/i40e/i40e_ethdev.h
> +++ b/drivers/net/i40e/i40e_ethdev.h
> @@ -36,6 +36,7 @@
>  
>  #include <rte_eth_ctrl.h>
>  #include <rte_time.h>
> +#include <rte_kvargs.h>
>  
>  #define I40E_VLAN_TAG_SIZE        4
>  
> @@ -171,6 +172,10 @@ enum i40e_flxpld_layer_idx {
>  #define I40E_QUEUE_ITR_INTERVAL_DEFAULT 32 /* 32 us */
>  #define I40E_QUEUE_ITR_INTERVAL_MAX     8160 /* 8160 us */
>  
> +/* Special FW support this floating VEB feature */
> +#define FLOATING_VEB_SUPPORTED_FW_MAJ 5
> +#define FLOATING_VEB_SUPPORTED_FW_MIN 0
> +
>  struct i40e_adapter;
>  
>  /**
> @@ -398,6 +403,8 @@ struct i40e_mirror_rule {
>  
>  TAILQ_HEAD(i40e_mirror_rule_list, i40e_mirror_rule);
>  
> +#define I40E_MAX_VF		128
> +
>  /*
>   * Structure to store private data specific for PF instance.
>   */
> @@ -450,6 +457,9 @@ struct i40e_pf {
>  	struct i40e_fc_conf fc_conf; /* Flow control conf */
>  	struct i40e_mirror_rule_list mirror_list;
>  	uint16_t nb_mirror_rule;   /* The number of mirror rules */
> +	bool floating_veb; /* The flag to use the floating VEB */
> +	/* The floating enable flag for the specific VF */
> +	bool vf_floating_veb[I40E_MAX_VF];
what about floating_veb_list, to be consistent with argument name?

>  };
>  
>  enum pending_msg {
>