Re: [dpdk-dev] [PATCH v5] ethdev: fix representor port ID search by name

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

On Fri, 2021-10-01 at 14:39 +0300, Andrew Rybchenko wrote:
> Hello PMD maintainers,
> 
> please, review the patch.
> 
> It is especially important for net/mlx5 since changes there are
> not trivial.
> 
> Thanks,
> Andrew.
> 
> On 9/13/21 2:26 PM, Andrew Rybchenko wrote:
> > From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> > 
> > Getting a list of representors from a representor does not make sense.
> > Instead, a parent device should be used.
> > 
> > To this end, extend the rte_eth_dev_data structure to include the port ID
> > of the backing device for representors.
> > 
> > Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> > Acked-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> > The new field is added into the hole in rte_eth_dev_data structure.
> > The patch does not change ABI, but extra care is required since ABI
> > check is disabled for the structure because of the libabigail bug [1].
> > It should not be a problem anyway since 21.11 is a ABI breaking release.
> > 
> > Potentially it is bad for out-of-tree drivers which implement
> > representors but do not fill in a new parert_port_id field in
> > rte_eth_dev_data structure. Get ID by name will not work.
> > 
> > mlx5 changes should be reviwed by maintainers very carefully, since
> > we are not sure if we patch it correctly.
> > 
> > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28060
> > 
> > v5:
> >     - try to improve name: backer_port_id instead of parent_port_id
> >     - init new field to RTE_MAX_ETHPORTS on allocation to avoid
> >       zero port usage by default
> > 
> > v4:
> >     - apply mlx5 review notes: remove fallback from generic ethdev
> >       code and add fallback to mlx5 code to handle legacy usecase
> > 
> > v3:
> >     - fix mlx5 build breakage
> > 
> > v2:
> >     - fix mlx5 review notes
> >     - try device port ID first before parent in order to address
> >       backward compatibility issue
> > 
> >  drivers/net/bnxt/bnxt_reps.c             |  1 +
> >  drivers/net/enic/enic_vf_representor.c   |  1 +
> >  drivers/net/i40e/i40e_vf_representor.c   |  1 +
> >  drivers/net/ice/ice_dcf_vf_representor.c |  1 +
> >  drivers/net/ixgbe/ixgbe_vf_representor.c |  1 +
> >  drivers/net/mlx5/linux/mlx5_os.c         | 13 +++++++++++++
> >  drivers/net/mlx5/windows/mlx5_os.c       | 13 +++++++++++++
> >  lib/ethdev/ethdev_driver.h               |  6 +++---
> >  lib/ethdev/rte_class_eth.c               |  2 +-
> >  lib/ethdev/rte_ethdev.c                  |  9 +++++----
> >  lib/ethdev/rte_ethdev_core.h             |  6 ++++++
> >  11 files changed, 46 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/bnxt/bnxt_reps.c b/drivers/net/bnxt/bnxt_reps.c
> > index bdbad53b7d..0d50c0f1da 100644
> > --- a/drivers/net/bnxt/bnxt_reps.c
> > +++ b/drivers/net/bnxt/bnxt_reps.c
> > @@ -187,6 +187,7 @@ int bnxt_representor_init(struct rte_eth_dev *eth_dev, void *params)
> >  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
> >  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
> >  	eth_dev->data->representor_id = rep_params->vf_id;
> > +	eth_dev->data->backer_port_id = rep_params->parent_dev->data->port_id;
> >  
> >  	rte_eth_random_addr(vf_rep_bp->dflt_mac_addr);
> >  	memcpy(vf_rep_bp->mac_addr, vf_rep_bp->dflt_mac_addr,
> > diff --git a/drivers/net/enic/enic_vf_representor.c b/drivers/net/enic/enic_vf_representor.c
> > index 79dd6e5640..fedb09ecd6 100644
> > --- a/drivers/net/enic/enic_vf_representor.c
> > +++ b/drivers/net/enic/enic_vf_representor.c
> > @@ -662,6 +662,7 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void *init_params)
> >  	eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
> >  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
> >  	eth_dev->data->representor_id = vf->vf_id;
> > +	eth_dev->data->backer_port_id = pf->port_id;
> >  	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr_vf",
> >  		sizeof(struct rte_ether_addr) *
> >  		ENIC_UNICAST_PERFECT_FILTERS, 0);
> > diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
> > index 0481b55381..d65b821a01 100644
> > --- a/drivers/net/i40e/i40e_vf_representor.c
> > +++ b/drivers/net/i40e/i40e_vf_representor.c
> > @@ -514,6 +514,7 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
> >  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
> >  					RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
> >  	ethdev->data->representor_id = representor->vf_id;
> > +	ethdev->data->backer_port_id = pf->dev_data->port_id;
> >  
> >  	/* Setting the number queues allocated to the VF */
> >  	ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
> > diff --git a/drivers/net/ice/ice_dcf_vf_representor.c b/drivers/net/ice/ice_dcf_vf_representor.c
> > index 970461f3e9..e51d0aa6b9 100644
> > --- a/drivers/net/ice/ice_dcf_vf_representor.c
> > +++ b/drivers/net/ice/ice_dcf_vf_representor.c
> > @@ -418,6 +418,7 @@ ice_dcf_vf_repr_init(struct rte_eth_dev *vf_rep_eth_dev, void *init_param)
> >  
> >  	vf_rep_eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >  	vf_rep_eth_dev->data->representor_id = repr->vf_id;
> > +	vf_rep_eth_dev->data->backer_port_id = repr->dcf_eth_dev->data->port_id;
> >  
> >  	vf_rep_eth_dev->data->mac_addrs = &repr->mac_addr;
> >  
> > diff --git a/drivers/net/ixgbe/ixgbe_vf_representor.c b/drivers/net/ixgbe/ixgbe_vf_representor.c
> > index d5b636a194..9fa75984fb 100644
> > --- a/drivers/net/ixgbe/ixgbe_vf_representor.c
> > +++ b/drivers/net/ixgbe/ixgbe_vf_representor.c
> > @@ -197,6 +197,7 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
> >  
> >  	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >  	ethdev->data->representor_id = representor->vf_id;
> > +	ethdev->data->backer_port_id = representor->pf_ethdev->data->port_id;
> >  
> >  	/* Set representor device ops */
> >  	ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
> > diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
> > index 470b16cb9a..1cddaaba1a 100644
> > --- a/drivers/net/mlx5/linux/mlx5_os.c
> > +++ b/drivers/net/mlx5/linux/mlx5_os.c
> > @@ -1677,6 +1677,19 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >  	if (priv->representor) {
> >  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >  		eth_dev->data->representor_id = priv->representor_id;
> > +		MLX5_ETH_FOREACH_DEV(port_id, &priv->pci_dev->device) {
> > +			struct mlx5_priv *opriv =
> > +				rte_eth_devices[port_id].data->dev_private;
> > +			if (opriv &&
> > +			    opriv->master &&
> > +			    opriv->domain_id == priv->domain_id &&
> > +			    opriv->sh == priv->sh) {
> > +				eth_dev->data->backer_port_id = port_id;
> > +				break;
> > +			}
> > +		}
> > +		if (port_id >= RTE_MAX_ETHPORTS)
> > +			eth_dev->data->backer_port_id = eth_dev->data->port_id;
> >  	}
> >  	priv->mp_id.port_id = eth_dev->data->port_id;
> >  	strlcpy(priv->mp_id.name, MLX5_MP_NAME, RTE_MP_MAX_NAME_LEN);
> > diff --git a/drivers/net/mlx5/windows/mlx5_os.c b/drivers/net/mlx5/windows/mlx5_os.c
> > index 26fa927039..a9c244c7dc 100644
> > --- a/drivers/net/mlx5/windows/mlx5_os.c
> > +++ b/drivers/net/mlx5/windows/mlx5_os.c
> > @@ -543,6 +543,19 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >  	if (priv->representor) {
> >  		eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >  		eth_dev->data->representor_id = priv->representor_id;
> > +		MLX5_ETH_FOREACH_DEV(port_id, &priv->pci_dev->device) {
> > +			struct mlx5_priv *opriv =
> > +				rte_eth_devices[port_id].data->dev_private;
> > +			if (opriv &&
> > +			    opriv->master &&
> > +			    opriv->domain_id == priv->domain_id &&
> > +			    opriv->sh == priv->sh) {
> > +				eth_dev->data->backer_port_id = port_id;
> > +				break;
> > +			}
> > +		}
> > +		if (port_id >= RTE_MAX_ETHPORTS)
> > +			eth_dev->data->backer_port_id = eth_dev->data->port_id;
> >  	}
> >  	/*
> >  	 * Store associated network device interface index. This index
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index 40e474aa7e..b940e6cb38 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1248,8 +1248,8 @@ struct rte_eth_devargs {
> >   * For backward compatibility, if no representor info, direct
> >   * map legacy VF (no controller and pf).
> >   *
> > - * @param ethdev
> > - *  Handle of ethdev port.
> > + * @param port_id
> > + *  Port ID of the backing device.
> >   * @param type
> >   *  Representor type.
> >   * @param controller
> > @@ -1266,7 +1266,7 @@ struct rte_eth_devargs {
> >   */
> >  __rte_internal
> >  int
> > -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> > +rte_eth_representor_id_get(uint16_t port_id,
> >  			   enum rte_eth_representor_type type,
> >  			   int controller, int pf, int representor_port,
> >  			   uint16_t *repr_id);
> > diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
> > index 1fe5fa1f36..eda216ced5 100644
> > --- a/lib/ethdev/rte_class_eth.c
> > +++ b/lib/ethdev/rte_class_eth.c
> > @@ -95,7 +95,7 @@ eth_representor_cmp(const char *key __rte_unused,
> >  		c = i / (np * nf);
> >  		p = (i / nf) % np;
> >  		f = i % nf;
> > -		if (rte_eth_representor_id_get(edev,
> > +		if (rte_eth_representor_id_get(edev->data->backer_port_id,
> >  			eth_da.type,
> >  			eth_da.nb_mh_controllers == 0 ? -1 :
> >  					eth_da.mh_controllers[c],
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index daf5ca9242..7c9b0d6b3b 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -524,6 +524,7 @@ rte_eth_dev_allocate(const char *name)
> >  	eth_dev = eth_dev_get(port_id);
> >  	strlcpy(eth_dev->data->name, name, sizeof(eth_dev->data->name));
> >  	eth_dev->data->port_id = port_id;
> > +	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
> >  	eth_dev->data->mtu = RTE_ETHER_MTU;
> >  	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
> >  
> > @@ -5996,7 +5997,7 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> >  }
> >  
> >  int
> > -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> > +rte_eth_representor_id_get(uint16_t port_id,
> >  			   enum rte_eth_representor_type type,
> >  			   int controller, int pf, int representor_port,
> >  			   uint16_t *repr_id)
> > @@ -6012,7 +6013,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> >  		return -EINVAL;
> >  
> >  	/* Get PMD representor range info. */
> > -	ret = rte_eth_representor_info_get(ethdev->data->port_id, NULL);
> > +	ret = rte_eth_representor_info_get(port_id, NULL);
> >  	if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
> >  	    controller == -1 && pf == -1) {
> >  		/* Direct mapping for legacy VF representor. */
> > @@ -6027,7 +6028,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> >  	if (info == NULL)
> >  		return -ENOMEM;
> >  	info->nb_ranges_alloc = n;
> > -	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> > +	ret = rte_eth_representor_info_get(port_id, info);
> >  	if (ret < 0)
> >  		goto out;
> >  
> > @@ -6046,7 +6047,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> >  			continue;
> >  		if (info->ranges[i].id_end < info->ranges[i].id_base) {
> >  			RTE_LOG(WARNING, EAL, "Port %hu invalid representor ID Range %u - %u, entry %d\n",
> > -				ethdev->data->port_id, info->ranges[i].id_base,
> > +				port_id, info->ranges[i].id_base,
> >  				info->ranges[i].id_end, i);
> >  			continue;
> >  
> > diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> > index edf96de2dc..48b814e8a1 100644
> > --- a/lib/ethdev/rte_ethdev_core.h
> > +++ b/lib/ethdev/rte_ethdev_core.h
> > @@ -185,6 +185,12 @@ struct rte_eth_dev_data {
> >  			/**< Switch-specific identifier.
> >  			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> >  			 */
> > +	uint16_t backer_port_id;
> > +			/**< Port ID of the backing device.
> > +			 *   This device will be used to query representor
> > +			 *   info and calculate representor IDs.
> > +			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> > +			 */
> >  
> >  	pthread_mutex_t flow_ops_mutex; /**< rte_flow ops mutex. */
> >  	uint64_t reserved_64s[4]; /**< Reserved for future fields */
> > 
> 

Reviewed-by: Xueming Li <xuemingl@nvidia.com>