Re: [dpdk-dev] [PATCH 2/2 v2] i40e: Add floating VEB support in i40e

["Wu, Jingjing" <jingjing.wu@intel.com>]

>  	if (ret != I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Add veb failed, aq_err: %d", @@ -
> 3688,20 +3702,21 @@ i40e_vsi_release(struct i40e_vsi *vsi)
>  		i40e_veb_release(vsi->veb);
>  	}
> 
> +	if (vsi->floating_veb) {
> +		TAILQ_FOREACH(vsi_list, &vsi->floating_veb->head, list) {
> +			if (i40e_vsi_release(vsi_list->vsi) != I40E_SUCCESS)
> +				return -1;
> +			TAILQ_REMOVE(&vsi->floating_veb->head, vsi_list,
> list);
> +		}
> +		i40e_veb_release(vsi->floating_veb);
> +	}
> +
>  	/* Remove all macvlan filters of the VSI */
>  	i40e_vsi_remove_all_macvlan_filter(vsi);
>  	TAILQ_FOREACH(f, &vsi->mac_list, next)
>  		rte_free(f);
> 
>  	if (vsi->type != I40E_VSI_MAIN) {
> -		/* Remove vsi from parent's sibling list */
> -		if (vsi->parent_vsi == NULL || vsi->parent_vsi->veb == NULL)
> {
> -			PMD_DRV_LOG(ERR, "VSI's parent VSI is NULL");
> -			return I40E_ERR_PARAM;
> -		}
> -		TAILQ_REMOVE(&vsi->parent_vsi->veb->head,
> -				&vsi->sib_vsi_list, list);
Why do you think these line is unnecessary? We need to remove it from vsi list.
> -
>  		/* Remove all switch element of the VSI */
>  		ret = i40e_aq_delete_element(hw, vsi->seid, NULL);
>  		if (ret != I40E_SUCCESS)
> @@ -3837,7 +3852,8 @@ i40e_vsi_setup(struct i40e_pf *pf,
>  	struct ether_addr broadcast =
>  		{.addr_bytes = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff}};
> 
> -	if (type != I40E_VSI_MAIN && uplink_vsi == NULL) {
> +	if (type != I40E_VSI_MAIN && type != I40E_VSI_SRIOV &&
> +	    uplink_vsi == NULL) {
>  		PMD_DRV_LOG(ERR, "VSI setup failed, "
>  			    "VSI link shouldn't be NULL");
>  		return NULL;
> @@ -3849,11 +3865,28 @@ i40e_vsi_setup(struct i40e_pf *pf,
>  		return NULL;
>  	}
> 
> -	/* If uplink vsi didn't setup VEB, create one first */
> -	if (type != I40E_VSI_MAIN && uplink_vsi->veb == NULL) {
> +	/* two situations
> +	 * 1.type is not MAIN and uplink vsi is not NULL
> +	 * If uplink vsi didn't setup VEB, create one first under veb field
> +	 * 2.type is SRIOV and the uplink is NULL
> +	 * If floating VEB is NULL, create one veb under floating veb field
> +	 */
> +
> +	if (type != I40E_VSI_MAIN && uplink_vsi != NULL &&
> +	    uplink_vsi->veb == NULL) {
>  		uplink_vsi->veb = i40e_veb_setup(pf, uplink_vsi);
> 
> -		if (NULL == uplink_vsi->veb) {
> +		if (uplink_vsi->veb == NULL) {
> +			PMD_DRV_LOG(ERR, "VEB setup failed");
> +			return NULL;
> +		}
> +	}
> +
> +	if (type == I40E_VSI_SRIOV && uplink_vsi == NULL &&
> +	    pf->main_vsi->floating_veb == NULL) {
> +		pf->main_vsi->floating_veb = i40e_veb_setup(pf, uplink_vsi);
> +
> +		if (pf->main_vsi->floating_veb == NULL) {
>  			PMD_DRV_LOG(ERR, "VEB setup failed");
>  			return NULL;
>  		}
> @@ -4022,7 +4055,11 @@ i40e_vsi_setup(struct i40e_pf *pf,
>  		 * For other VSI, the uplink_seid equals to uplink VSI's
>  		 * uplink_seid since they share same VEB
>  		 */
> -		vsi->uplink_seid = uplink_vsi->uplink_seid;
> +		if (uplink_vsi == NULL) {
> +			vsi->uplink_seid = pf->main_vsi->floating_veb->seid;
> +		} else {
> +			vsi->uplink_seid = uplink_vsi->uplink_seid;
> +		}
{} is not necessary.

>  		ctxt.pf_num = hw->pf_id;
>  		ctxt.vf_num = hw->func_caps.vf_base_id + user_param;
>  		ctxt.uplink_seid = vsi->uplink_seid;
> @@ -4130,8 +4167,13 @@ i40e_vsi_setup(struct i40e_pf *pf,
>  		vsi->seid = ctxt.seid;
>  		vsi->vsi_id = ctxt.vsi_number;
>  		vsi->sib_vsi_list.vsi = vsi;
> -		TAILQ_INSERT_TAIL(&uplink_vsi->veb->head,
> -				&vsi->sib_vsi_list, list);
> +		if (vsi->type == I40E_VSI_SRIOV && uplink_vsi == NULL) {
> +			TAILQ_INSERT_TAIL(&pf->main_vsi->floating_veb-
> >head,
> +					  &vsi->sib_vsi_list, list);
> +		} else {
> +			TAILQ_INSERT_TAIL(&uplink_vsi->veb->head,
> +					  &vsi->sib_vsi_list, list);
> +		}
>  	}
> 
>  	/* MAC/VLAN configuration */
> diff --git a/drivers/net/i40e/i40e_ethdev.h
> b/drivers/net/i40e/i40e_ethdev.h index 6edd7dd..19246db 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 <eal_internal_cfg.h>
> 
>  #define I40E_VLAN_TAG_SIZE        4
> 
> @@ -203,6 +204,7 @@ struct i40e_tx_queue;  struct i40e_veb {
>  	struct i40e_vsi_list_head head;
>  	struct i40e_vsi *associate_vsi; /* Associate VSI who owns the VEB */
> +	struct i40e_pf *associate_pf; /* Associate PF who owns the VEB */
>  	uint16_t seid; /* The seid of VEB itself */
>  	uint16_t uplink_seid; /* The uplink seid of this VEB */
>  	uint16_t stats_idx;
> @@ -254,6 +256,7 @@ struct i40e_vsi {
>  	struct i40e_vsi_list sib_vsi_list; /* sibling vsi list */
>  	struct i40e_vsi *parent_vsi;
>  	struct i40e_veb *veb;    /* Associated veb, could be null */
> +	struct i40e_veb *floating_veb; /* Associated floating veb */
>  	bool offset_loaded;
>  	enum i40e_vsi_type type; /* VSI types */
>  	uint16_t vlan_num;       /* Total VLAN number */
> diff --git a/drivers/net/i40e/i40e_pf.c b/drivers/net/i40e/i40e_pf.c index
> cbf4e5b..cc315f6 100644
> --- a/drivers/net/i40e/i40e_pf.c
> +++ b/drivers/net/i40e/i40e_pf.c
> @@ -223,9 +223,15 @@ i40e_pf_host_vf_reset(struct i40e_pf_vf *vf, bool
> do_hw_reset)
>  	vf->reset_cnt++;
>  	I40E_WRITE_FLUSH(hw);
> 
> +	if (internal_config.floating == true) {
> +		vf->vsi = i40e_vsi_setup(vf->pf, I40E_VSI_SRIOV,
> +			NULL, vf->vf_idx);
> +	} else {
>  	/* Allocate resource again */
> -	vf->vsi = i40e_vsi_setup(vf->pf, I40E_VSI_SRIOV,
> -			vf->pf->main_vsi, vf->vf_idx);
> +		vf->vsi = i40e_vsi_setup(vf->pf, I40E_VSI_SRIOV,
> +				vf->pf->main_vsi, vf->vf_idx);
> +	}
{} is not necessary. How about to check the NVM version?

Thanks
Jingjing