> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Sunday, August 1, 2021 4:40 PM
> To: Xueming(Steven) Li <xuemi...@nvidia.com>; 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>
> Cc: dev@dpdk.org; Viacheslav Galaktionov
> <viacheslav.galaktio...@oktetlabs.ru>; sta...@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
>
> On 7/29/21 7:13 AM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >> Sent: Tuesday, July 20, 2021 5:00 PM
> >> To: Xueming(Steven) Li <xuemi...@nvidia.com>; 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>
> >> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >> <viacheslav.galaktio...@oktetlabs.ru>; sta...@dpdk.org
> >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >>>> Sent: Monday, July 19, 2021 8:36 PM
> >>>> To: Xueming(Steven) Li <xuemi...@nvidia.com>; 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>
> >>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >>>> <viacheslav.galaktio...@oktetlabs.ru>; sta...@dpdk.org
> >>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>>>
> >>>> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >>>>>> Sent: Monday, July 19, 2021 4:46 PM
> >>>>>> To: Xueming(Steven) Li <xuemi...@nvidia.com>; 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>
> >>>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >>>>>> <viacheslav.galaktio...@oktetlabs.ru>; sta...@dpdk.org
> >>>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by
> >>>>>> name
> >>>>>>
> >>>>>> On 7/19/21 9:58 AM, Xueming(Steven) Li wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----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
> >>>>>>
> >>>>>> [snip]
> >>>>>>
> >>>>>>>> --- 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,
> >>>>>>>
> >>>>>>> It make more sense to get representor info from parent port.
> >>>>>>> Representor is a member of switch domain, PMD owns the
> >>>>>>> information of the representor owner port and info of
> >>>>>>> representors. This change looks better, but not sure whether it
> >>>>>>> valuable to introduce a new
> >>>>>> member to the EAL data structure.
> >>>>>>
> >>>>>> IMHO, it is simply incorrect to return representors info on a
> >>>>>> representor itself. Representor info is an information which
> >>>>>> representors may be populated using the device.
> >>>>>>
> >>>>>> If above statement is correct, we need a way to get parent device
> >>>>>> by representor to do name to representor ID mapping. I see two options
> >>>>>> to do it:
> >>>>>> A. Dedicated field in rte_eth_dev_data as the patch does.
> >>>>>> B. Dedicated ethdev op (since representor knows parent port ID
> >>>>>> anyway).
> >>>>>> We have chosen (A) because of simplicity.
> >>>>>
> >>>>> Just recalled that representor port could be probed w/o owner PF, is a
> >>>>> force for parent port?
> >>>>
> >>>> I thought that it is impossible and parent port is absolutely
> >>>> required for a representor. Could you provide an example and explain how
> >>>> will it work?
> >>>
> >>> In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address
> >>> is PF0.
> >>> -a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.
> >>
> >> Is it net/bonding or vendor-specific bonding in HW?
> >> If I remember correctly in the case of net/bonding we have ethdev ports
> >> for bonded devices.
> >
> > Not net/bonding pmd, it's Linux bonding, supported by hw driver.
>
> Got it.
>
> >>
> >>>
> >>> To be backward compatible, also support the following 2 devargs:
> >>> -a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
> >>> -a <pf1>,representor=[0-99] // probe representors on pf1.
> >>> If devargs start with PF1 devargs, no owner PF1 created as it
> >>> disabled in bonding. Can't create bond0(PF0) automatically here as device
> >>> is located by PCI address(PF1) from devargs.
> >>
> >> So, I guess the problem is vendor-specific bonding in HW. Anyway
> >> legacy backward compatible representor spec should not require
> >> representors info since it worked before without it. So, it does not sound
> >> like a reason to have representors info on a representor
> itself.
> >
> > Legacy backward logic could be something like this: if PF owner port found,
> > use it, fallback to current representor.
> > This won't break anything I guess, how do you think?
>
> Logically even in legacy backward compatibility PF1 VFs representors have
> parent port ID - PF0 which is a bond of PF0 & PF1. So,
> parent_port_id should be filled in. In this case eth_representor_cmp() will
> do rte_eth_representor_id_get(PF0-bond-id, -1, -1, VF, &id)
> which will return PF0 VF representor ID. Most likely it will even match and
> everything works, but it is still incorrect.
The PF0, bond of PF0 and PF1 will return representor info for VF/SFs under both
PFs. It should work.
>
> In fact, I have another idea. Try to do:
> rte_eth_representor_id_get(representor-port-id, ...) first for the backward
> compatibility case and, if not supported, do it on parent
> port ID.
Looks good to me
