> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Tuesday, July 13, 2021 12:18 AM
> To: Ajit Khaparde <ajit.khapa...@broadcom.com>; Somnath Kotur
> <somnath.ko...@broadcom.com>; John Daley
> <johnd...@cisco.com>; Hyong Youb Kim <hyon...@cisco.com>; Beilei Xing
> <beilei.x...@intel.com>; Qiming Yang
> <qiming.y...@intel.com>; Qi Zhang <qi.z.zh...@intel.com>; Haiyue Wang
> <haiyue.w...@intel.com>; Matan Azrad
> <ma...@nvidia.com>; Shahaf Shuler <shah...@nvidia.com>; Slava Ovsiienko
> <viachesl...@nvidia.com>; NBU-Contact-Thomas
> Monjalon <tho...@monjalon.net>; Ferruh Yigit <ferruh.yi...@intel.com>;
> Xueming(Steven) Li <xuemi...@nvidia.com>
> Cc: dev@dpdk.org; Viacheslav Galaktionov
> <viacheslav.galaktio...@oktetlabs.ru>; sta...@dpdk.org
> Subject: [PATCH] ethdev: fix representor port ID search by name
>
> From: Viacheslav Galaktionov <viacheslav.galaktio...@oktetlabs.ru>
>
> Fix representor port ID search by name if the representor itself does not
> provide representors info. 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 parent device for representors.
>
> Fixes: df7547a6a2cc ("ethdev: add helper function to get representor ID")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktio...@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> ---
> 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].
>
> 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. Do we care?
>
> May be the patch should add lines to release notes, but I'd like to get
> initial feedback first.
>
> 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
>
> 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 | 11 +++++++++++
> drivers/net/mlx5/windows/mlx5_os.c | 11 +++++++++++
> lib/ethdev/ethdev_driver.h | 6 +++---
> lib/ethdev/rte_class_eth.c | 2 +-
> lib/ethdev/rte_ethdev.c | 8 ++++----
> lib/ethdev/rte_ethdev_core.h | 4 ++++
> 11 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_reps.c b/drivers/net/bnxt/bnxt_reps.c
> index bdbad53b7d..902591cd39 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->parent_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..6ee7967ce9 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->parent_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..865b637585 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->parent_port_id = pf->dev_data->parent_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..c7cd3fd290 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->parent_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..7a2063849e 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->parent_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 be22d9cbd2..5550d30628 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -1511,6 +1511,17 @@ 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) {
> + const struct mlx5_priv *opriv =
> + rte_eth_devices[port_id].data->dev_private;
> +
> + if (!opriv ||
> + opriv->sh != priv->sh ||
> + opriv->representor)
> + continue;
> + eth_dev->data->parent_port_id = port_id;
> + break;
> + }
At line 126, there is a logic that locate priv->domain_id, parent port_id could
be found there.
> }
> 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 e30b682822..037c928dc1 100644
> --- a/drivers/net/mlx5/windows/mlx5_os.c
> +++ b/drivers/net/mlx5/windows/mlx5_os.c
> @@ -506,6 +506,17 @@ 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) {
> + const struct mlx5_priv *opriv =
> + rte_eth_devices[port_id].data->dev_private;
> +
> + if (!opriv ||
> + opriv->sh != priv->sh ||
> + opriv->representor)
> + continue;
> + eth_dev->data->parent_port_id = port_id;
> + break;
> + }
> }
> /*
> * Store associated network device interface index. This index diff
> --git a/lib/ethdev/ethdev_driver.h
> b/lib/ethdev/ethdev_driver.h index 40e474aa7e..07f6d1f9a4 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 parent_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 parent_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..e3b7ab9728 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->parent_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
> 6ebf52b641..acda1d43fb 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5997,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 parent_port_id,
> enum rte_eth_representor_type type,
> int controller, int pf, int representor_port,
> uint16_t *repr_id)
> @@ -6012,7 +6012,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(parent_port_id, NULL);
> if (ret == -ENOTSUP && type == RTE_ETH_REPRESENTOR_VF &&
> controller == -1 && pf == -1) {
> /* Direct mapping for legacy VF representor. */ @@ -6026,7
> +6026,7 @@ rte_eth_representor_id_get(const struct
> rte_eth_dev *ethdev,
> info = calloc(1, size);
> if (info == NULL)
> return -ENOMEM;
> - ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
> + ret = rte_eth_representor_info_get(parent_port_id, info);
> if (ret < 0)
> goto out;
>
> @@ -6045,7 +6045,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,
> + parent_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..13cb84b52f 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -185,6 +185,10 @@ struct rte_eth_dev_data {
> /**< Switch-specific identifier.
> * Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> */
> + uint16_t parent_port_id;
> + /**< Port ID of the backing device.
> + * 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 */
> --
> 2.30.2
