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

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Monday, July 19, 2021 8:36 PM
> To: Xueming(Steven) Li <xuemingl at nvidia.com>; Ajit Khaparde <ajit.khaparde at broadcom.com>; Somnath Kotur
> <somnath.kotur at broadcom.com>; John Daley <johndale at cisco.com>; Hyong Youb Kim <hyonkim at cisco.com>; Beilei Xing
> <beilei.xing at intel.com>; Qiming Yang <qiming.yang at intel.com>; Qi Zhang <qi.z.zhang at intel.com>; Haiyue Wang
> <haiyue.wang at intel.com>; Matan Azrad <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>; Slava Ovsiienko
> <viacheslavo at nvidia.com>; NBU-Contact-Thomas Monjalon <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>
> Cc: dev at dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov at oktetlabs.ru>; stable at 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.rybchenko at oktetlabs.ru>
> >> Sent: Monday, July 19, 2021 4:46 PM
> >> To: Xueming(Steven) Li <xuemingl at nvidia.com>; Ajit Khaparde
> >> <ajit.khaparde at broadcom.com>; Somnath Kotur
> >> <somnath.kotur at broadcom.com>; John Daley <johndale at cisco.com>; Hyong
> >> Youb Kim <hyonkim at cisco.com>; Beilei Xing <beilei.xing at intel.com>;
> >> Qiming Yang <qiming.yang at intel.com>; Qi Zhang <qi.z.zhang at intel.com>;
> >> Haiyue Wang <haiyue.wang at intel.com>; Matan Azrad <matan at nvidia.com>;
> >> Shahaf Shuler <shahafs at nvidia.com>; Slava Ovsiienko
> >> <viacheslavo at nvidia.com>; NBU-Contact-Thomas Monjalon
> >> <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>
> >> Cc: dev at dpdk.org; Viacheslav Galaktionov
> >> <viacheslav.galaktionov at oktetlabs.ru>; stable at 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.rybchenko at oktetlabs.ru>
> >>>> Sent: Tuesday, July 13, 2021 12:18 AM
> >>>> To: Ajit Khaparde <ajit.khaparde at broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur at broadcom.com>; John Daley <johndale at cisco.com>;
> >>>> Hyong Youb Kim <hyonkim at cisco.com>; Beilei Xing
> >>>> <beilei.xing at intel.com>; Qiming Yang <qiming.yang at intel.com>; Qi
> >>>> Zhang <qi.z.zhang at intel.com>; Haiyue Wang <haiyue.wang at intel.com>;
> >>>> Matan Azrad <matan at nvidia.com>; Shahaf Shuler <shahafs at nvidia.com>;
> >>>> Slava Ovsiienko <viacheslavo at nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas at monjalon.net>; Ferruh Yigit
> >>>> <ferruh.yigit at intel.com>;
> >>>> Xueming(Steven) Li <xuemingl at nvidia.com>
> >>>> Cc: dev at dpdk.org; Viacheslav Galaktionov
> >>>> <viacheslav.galaktionov at oktetlabs.ru>; stable at dpdk.org
> >>>> Subject: [PATCH] ethdev: fix representor port ID search by name
> >>>>
> >>>> From: Viacheslav Galaktionov <viacheslav.galaktionov at 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: stable at dpdk.org
> >>>>
> >>>> Signed-off-by: Viacheslav Galaktionov
> >>>> <viacheslav.galaktionov at oktetlabs.ru>
> >>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko at 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.

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.