> -----Original Message----- > From: Andrew Rybchenko <[email protected]> > Sent: Sunday, August 1, 2021 4:40 PM > To: Xueming(Steven) Li <[email protected]>; Ajit Khaparde > <[email protected]>; Somnath Kotur > <[email protected]>; John Daley <[email protected]>; Hyong Youb Kim > <[email protected]>; Beilei Xing > <[email protected]>; Qiming Yang <[email protected]>; Qi Zhang > <[email protected]>; Haiyue Wang > <[email protected]>; Matan Azrad <[email protected]>; Shahaf Shuler > <[email protected]>; Slava Ovsiienko > <[email protected]>; NBU-Contact-Thomas Monjalon <[email protected]>; > Ferruh Yigit <[email protected]> > Cc: [email protected]; Viacheslav Galaktionov > <[email protected]>; [email protected] > 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 <[email protected]> > >> Sent: Tuesday, July 20, 2021 5:00 PM > >> To: Xueming(Steven) Li <[email protected]>; Ajit Khaparde > >> <[email protected]>; Somnath Kotur > >> <[email protected]>; John Daley <[email protected]>; Hyong > >> Youb Kim <[email protected]>; Beilei Xing <[email protected]>; > >> Qiming Yang <[email protected]>; Qi Zhang <[email protected]>; > >> Haiyue Wang <[email protected]>; Matan Azrad <[email protected]>; > >> Shahaf Shuler <[email protected]>; Slava Ovsiienko > >> <[email protected]>; NBU-Contact-Thomas Monjalon > >> <[email protected]>; Ferruh Yigit <[email protected]> > >> Cc: [email protected]; Viacheslav Galaktionov > >> <[email protected]>; [email protected] > >> 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 <[email protected]> > >>>> Sent: Monday, July 19, 2021 8:36 PM > >>>> To: Xueming(Steven) Li <[email protected]>; Ajit Khaparde > >>>> <[email protected]>; Somnath Kotur > >>>> <[email protected]>; John Daley <[email protected]>; > >>>> Hyong Youb Kim <[email protected]>; Beilei Xing > >>>> <[email protected]>; Qiming Yang <[email protected]>; Qi > >>>> Zhang <[email protected]>; Haiyue Wang <[email protected]>; > >>>> Matan Azrad <[email protected]>; Shahaf Shuler <[email protected]>; > >>>> Slava Ovsiienko <[email protected]>; NBU-Contact-Thomas > >>>> Monjalon <[email protected]>; Ferruh Yigit > >>>> <[email protected]> > >>>> Cc: [email protected]; Viacheslav Galaktionov > >>>> <[email protected]>; [email protected] > >>>> 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 <[email protected]> > >>>>>> Sent: Monday, July 19, 2021 4:46 PM > >>>>>> To: Xueming(Steven) Li <[email protected]>; Ajit Khaparde > >>>>>> <[email protected]>; Somnath Kotur > >>>>>> <[email protected]>; John Daley <[email protected]>; > >>>>>> Hyong Youb Kim <[email protected]>; Beilei Xing > >>>>>> <[email protected]>; Qiming Yang <[email protected]>; Qi > >>>>>> Zhang <[email protected]>; Haiyue Wang > >>>>>> <[email protected]>; Matan Azrad <[email protected]>; Shahaf > >>>>>> Shuler <[email protected]>; Slava Ovsiienko > >>>>>> <[email protected]>; NBU-Contact-Thomas Monjalon > >>>>>> <[email protected]>; Ferruh Yigit <[email protected]> > >>>>>> Cc: [email protected]; Viacheslav Galaktionov > >>>>>> <[email protected]>; [email protected] > >>>>>> 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 <[email protected]> > >>>>>>>> Sent: Tuesday, July 13, 2021 12:18 AM > >>>>>>>> To: Ajit Khaparde <[email protected]>; Somnath Kotur > >>>>>>>> <[email protected]>; John Daley <[email protected]>; > >>>>>>>> Hyong Youb Kim <[email protected]>; Beilei Xing > >>>>>>>> <[email protected]>; Qiming Yang <[email protected]>; > >>>>>>>> Qi Zhang <[email protected]>; Haiyue Wang > >>>>>>>> <[email protected]>; Matan Azrad <[email protected]>; Shahaf > >>>>>>>> Shuler <[email protected]>; Slava Ovsiienko > >>>>>>>> <[email protected]>; NBU-Contact-Thomas Monjalon > >>>>>>>> <[email protected]>; Ferruh Yigit <[email protected]>; > >>>>>>>> Xueming(Steven) Li <[email protected]> > >>>>>>>> Cc: [email protected]; Viacheslav Galaktionov > >>>>>>>> <[email protected]>; [email protected] > >>>>>>>> Subject: [PATCH] ethdev: fix representor port ID search by name > >>>>>>>> > >>>>>>>> From: Viacheslav Galaktionov > >>>>>>>> <[email protected]> > >>>>>>>> > >>>>>>>> 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: [email protected] > >>>>>>>> > >>>>>>>> Signed-off-by: Viacheslav Galaktionov > >>>>>>>> <[email protected]> > >>>>>>>> Signed-off-by: Andrew Rybchenko <[email protected]> > >>>>>>>> --- > >>>>>>>> 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
