Re: [dpdk-dev] [PATCH v5 5/9] ethdev: support PF index in representor

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

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 5:36 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>; NBU-Contact-Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>Subject: Re: [PATCH v5 5/9] ethdev: support PF index in representor
>
>On 1/19/21 12:30 PM, Xueming(Steven) Li wrote:
>> Hi Andrew,
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Sent: Tuesday, January 19, 2021 4:01 PM
>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
>>> Penso <asafp@nvidia.com>; NBU-Contact-Thomas Monjalon
>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
>>> Subject: Re: [PATCH v5 5/9] ethdev: support PF index in representor
>>>
>>> On 1/19/21 10:14 AM, Xueming Li wrote:
>>>> With Kernel bonding, multiple underlying PFs are bonded, VFs come
>>>> from different PF, need to identify representor of VFs unambiguously
>>>> by adding PF index.
>>>>
>>>> This patch introduces optional 'pf' section to representor devargs
>>>> syntax, examples:
>>>>  representor=pf0vf0             - single VF representor
>>>>  representor=pf[0-1]sf[0-1023]  - SF representors from 2 PFs
>>>
>>>
>>> Don't we need
>>> representor=pf3
>>> i.e. without VF or sub-function?
>>
>> Standalone PF not used by Mellnaox PMD, but should be supported.
>> Will update.
>>>
>>>>
>>>>
>>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>>> ---
>>>>  doc/guides/prog_guide/poll_mode_drv.rst |  2 ++
>>>>  lib/librte_ethdev/ethdev_private.c      | 13 +++++++++++--
>>>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
>>>> b/doc/guides/prog_guide/poll_mode_drv.rst
>>>> index 86e5867f1b..b2147aad30 100644
>>>> --- a/doc/guides/prog_guide/poll_mode_drv.rst
>>>> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
>>>> @@ -382,6 +382,8 @@ parameters to those ports.
>>>>     -a DBDF,representor=sf[1,3,5]
>>>>     -a DBDF,representor=sf[0-1023]
>>>>     -a DBDF,representor=sf[0,2-4,7,9-11]
>>>> +   -a DBDF,representor=pf1vf0
>>>> +   -a DBDF,representor=pf[0-1]sf[0-127]
>>>>
>>>>  Note: PMDs are not required to support the standard device
>>>> arguments and users  should consult the relevant PMD documentation
>>>> to see support
>>> devargs.
>>>> diff --git a/lib/librte_ethdev/ethdev_private.c
>>>> b/lib/librte_ethdev/ethdev_private.c
>>>> index d513f035d0..b9fdbd0f72 100644
>>>> --- a/lib/librte_ethdev/ethdev_private.c
>>>> +++ b/lib/librte_ethdev/ethdev_private.c
>>>> @@ -120,8 +120,8 @@ rte_eth_devargs_process_list(char *str, uint16_t
>>> *list, uint16_t *len_list,
>>>>   *
>>>>   * Representor format:
>>>>   *   #: range or single number of VF representor - legacy
>>>> - *   vf#: VF port representor/s
>>>> - *   sf#: SF port representor/s
>>>> + *   [pf#]vf#: VF port representor/s
>>>> + *   [pf#]sf#: SF port representor/s
>>>>   *
>>>>   * Examples of #:
>>>>   *  2               - single
>>>> @@ -133,6 +133,14 @@ rte_eth_devargs_parse_representor_ports(char
>>>> *str, void *data)  {
>>>>  	struct rte_eth_devargs *eth_da = data;
>>>>
>>>> +	if (str[0] == 'p' && str[1] == 'f') {
>>>> +		eth_da->type = RTE_ETH_REPRESENTOR_PF;
>>>> +		str += 2;
>>>> +		str = rte_eth_devargs_process_list(str, eth_da->ports,
>>>> +				&eth_da->nb_ports, RTE_MAX_ETHPORTS);
>>>
>>> May be RTE_MAX_ETHPORTS -> RTE_DIM(eth_da->ports) ?
>>
>> Same here, the dim could be different than MAX value.
>
>Hold on, just for my understanding. The maximum says how many entries
>could be added to the array. So, why?

Right, will change to RTE_DIM(), thanks!
>
>>>
>>>> +		if (str == NULL)
>>>> +			goto err;
>>>
>>> Below we should not allow legacy VF syntax without "vf" prefix.
>>
>> For backward compatibility, default numbers to "vf", otherwise some
>> existing app like OVS that working with VF will break.
>
>I mean if new syntax is used (i.e. we have pfX prefix), we must deny legacy
>syntax for VFs below.

Correct, will add check, thanks!
>
>>>
>>>> +	}
>>>>  	if (str[0] == 'v' && str[1] == 'f') {
>>>>  		eth_da->type = RTE_ETH_REPRESENTOR_VF;
>>>>  		str += 2;
>>>> @@ -144,6 +152,7 @@ rte_eth_devargs_parse_representor_ports(char
>>>> *str,
>>> void *data)
>>>>  	}
>>>>  	str = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>>>>  		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
>>>> +err:
>>>>  	if (str == NULL)
>>>>  		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
>>>>  	return str == NULL ? -1 : 0;
>>>>
>>