Hi Andrew,
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>Sent: Monday, December 28, 2020 9:37 PM
>To: Xueming(Steven) Li <xuemi...@nvidia.com>; Slava Ovsiienko
><viachesl...@nvidia.com>; NBU-Contact-Thomas Monjalon
><tho...@monjalon.net>; Ferruh Yigit <ferruh.yi...@intel.com>; Olivier Matz
><olivier.m...@6wind.com>; Matan Azrad <ma...@nvidia.com>
>Cc: dev@dpdk.org; Asaf Penso <as...@nvidia.com>
>Subject: Re: [RFC 3/7] devarg: change reprsentor ID to bitmap
>
>On 12/18/20 5:55 PM, Xueming Li wrote:
>> In eth representor comparer callback, ethdev was compared with devarg.
>
>comparer -> comparator?
>
>> Since ethdev representor port didn't contain controller(host) and
>> owner port information, callback only compared representor port and
>> returned representor port on other PF port.
>>
>> This patch changes representor port to bitmap encoding, expands and
>> updates representor port ID after parsing, when device representor ID
>> uses the same bitmap encoding, the eth representor comparer callback
>> returns correct ethdev.
>>
>> Representor port ID bitmap definition:
>> Representor ID bitmap:
>> xxxx xxxx xxxx xxxx
>> |||| |LLL LLLL LLLL vf/sf id
>> |||| L 1:sf, 0:vf
>> ||LL pf id
>
>Just 2 bits for PF ID is definitely not future proof.
Yes, this is a valid concern, to keep ABI compatibility, need to wait next LTS
to
change it to u32 or u64.
>
>> LL controller(host) id
>
>Same here.
>
>In general, I'm not sure that such approch with bitmap makes sense. I think
>we need a new API which returns information about available functions which
>could be represented and IDs there could be used as representor IDs.
Agree, will introduce rte_eth_representor_id_encode() and
rte_eth_representor_id_parse()
in next vesion.
>
>>
>> Signed-off-by: Xueming Li <xuemi...@nvidia.com>
>> ---
>> 0000-cover-letter.patch | 44 +++++++++++++++++++++++++++
>
>I guess it should not be added to the changeset.
>
>> lib/librte_ethdev/ethdev_private.c | 42 ++++++++++++++++++++++++-
>> lib/librte_ethdev/rte_ethdev_driver.h | 22 ++++++++++++++
>> 3 files changed, 107 insertions(+), 1 deletion(-) create mode 100644
>> 0000-cover-letter.patch
>>
>> diff --git a/0000-cover-letter.patch b/0000-cover-letter.patch new
>> file mode 100644 index 0000000000..3f8ce2be72
>> --- /dev/null
>> +++ b/0000-cover-letter.patch
>> @@ -0,0 +1,44 @@
>> +From 4e1f8fc062fa6813e0b57f78ad72760601ca1d98 Mon Sep 17 00:00:00
>> +2001
>> +From: Xueming Li <xuemi...@nvidia.com>
>> +Date: Fri, 18 Dec 2020 22:31:53 +0800
>> +Subject: [RFC 0/7] *** SUBJECT HERE ***
>> +To: Viacheslav Ovsiienko <viachesl...@nvidia.com>,
>> + Thomas Monjalon <tho...@monjalon.net>,
>> + Ferruh Yigit <ferruh.yi...@intel.com>,
>> + Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>,
>> + Olivier Matz <olivier.m...@6wind.com>,
>> + Matan Azrad <ma...@nvidia.com>
>> +Cc: dev@dpdk.org,
>> + xuemi...@nvidia.com,
>> + Asaf Penso <as...@nvidia.com>
>> +
>> +*** BLURB HERE ***
>> +
>> +Xueming Li (7):
>> + ethdev: support sub function representor
>> + ethdev: support multi-host representor
>> + devarg: change reprsentor ID to bitmap
>> + ethdev: capability for new representor syntax
>> + kvargs: update parser for new representor syntax
>> + common/mlx5: update representor name parsing
>> + net/mlx5: support representor of sub function
>> +
>> + config/rte_config.h | 1 +
>> + drivers/common/mlx5/linux/mlx5_common_os.c | 32 ++--
>> + drivers/common/mlx5/linux/mlx5_nl.c | 2 +
>> + drivers/common/mlx5/mlx5_common.h | 2 +
>> + drivers/net/mlx5/linux/mlx5_ethdev_os.c | 5 +
>> + drivers/net/mlx5/linux/mlx5_os.c | 69 ++++++++-
>> + drivers/net/mlx5/mlx5_ethdev.c | 2 +
>> + lib/librte_ethdev/ethdev_private.c | 163 ++++++++++++++-------
>> + lib/librte_ethdev/ethdev_private.h | 3 -
>> + lib/librte_ethdev/rte_class_eth.c | 7 +-
>> + lib/librte_ethdev/rte_ethdev.c | 5 +-
>> + lib/librte_ethdev/rte_ethdev.h | 2 +
>> + lib/librte_ethdev/rte_ethdev_driver.h | 35 +++++
>> + lib/librte_kvargs/rte_kvargs.c | 82 +++++++----
>> + 14 files changed, 306 insertions(+), 104 deletions(-)
>> +
>> +--
>> +2.25.1
>> +
>> diff --git a/lib/librte_ethdev/ethdev_private.c
>> b/lib/librte_ethdev/ethdev_private.c
>> index 3e455acea9..a0fc187378 100644
>> --- a/lib/librte_ethdev/ethdev_private.c
>> +++ b/lib/librte_ethdev/ethdev_private.c
>> @@ -93,16 +93,20 @@ rte_eth_devargs_process_list(char *str, uint16_t
>> *list, uint16_t *len_list, }
>>
>> /*
>> - * representor format:
>> + * Parse representor ports, expand and update representor port ID.
>> + * Representor format:
>> * #: range or single number of VF representor - legacy
>> * [[c#]pf#]vf#: VF port representor/s
>> * [[c#]pf#]sf#: SF port representor/s
>> + *
>> + * See RTE_ETH_REPR() for representor ID format.
>> */
>> int
>> rte_eth_devargs_parse_representor_ports(char *str, void *data) {
>> struct rte_eth_devargs *eth_da = data;
>> int ret;
>> + uint32_t c, p, f, i = 0;
>>
>> eth_da->type = RTE_ETH_REPRESENTOR_NONE;
>> if (str[0] == 'c') {
>> @@ -136,6 +140,42 @@ rte_eth_devargs_parse_representor_ports(char
>*str, void *data)
>> }
>> ret = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>> ð_da->nb_representor_ports, RTE_MAX_ETHPORTS);
>> + if (ret < 0)
>> + goto err;
>> +
>> + /* Set default values, expand and update representor ID. */
>> + if (!eth_da->nb_mh_controllers) {
>
>DPDK coding style requires to compare vs 0 expliticly.
>
>> + eth_da->nb_mh_controllers = 1;
>> + eth_da->mh_controllers[0] = 0;
>> + }
>> + if (!eth_da->nb_ports) {
>
>DPDK coding style requires to compare vs 0 expliticly.
>
>> + eth_da->nb_ports = 1;
>> + eth_da->ports[0] = 0;
>> + }
>> + if (!eth_da->nb_representor_ports) {
>
>DPDK coding style requires to compare vs 0 expliticly.
>
>> + eth_da->nb_representor_ports = 1;
>> + eth_da->representor_ports[0] = 0;
>> + }
>> + for (c = 0; c < eth_da->nb_mh_controllers; ++c) {
>> + for (p = 0; p < eth_da->nb_ports; ++p) {
>> + for (f = 0; f < eth_da->nb_representor_ports; ++f) {
>> + i = c * eth_da->nb_ports *
>> + eth_da->nb_representor_ports +
>> + p * eth_da->nb_representor_ports + f;
>> + if (i >= RTE_DIM(eth_da->representor_ports))
>{
>> + RTE_LOG(ERR, EAL, "too many
>representor specified: %s",
>
>Missing \n
>
>> + str);
>> + return -EINVAL;
>> + }
>> + eth_da->representor_ports[i] =
>RTE_ETH_REPR(
>> + eth_da->mh_controllers[c],
>> + eth_da->ports[p],
>> + eth_da->type ==
>RTE_ETH_REPRESENTOR_SF,
>> + eth_da->representor_ports[f]);
>> + }
>> + }
>> + }
>> + eth_da->nb_representor_ports = i + 1;
>> err:
>> if (ret < 0)
>> RTE_LOG(ERR, EAL, "wrong representor format: %s", str); diff -
>-git
>> a/lib/librte_ethdev/rte_ethdev_driver.h
>> b/lib/librte_ethdev/rte_ethdev_driver.h
>> index a7969c9408..dbad55c704 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -1218,6 +1218,28 @@ struct rte_eth_devargs {
>> enum rte_eth_representor_type type; /* type of representor */ };
>>
>> +/**
>> + * Encoding representor port ID.
>> + *
>> + * The compact format is used for device iterator that comparing
>> + * ethdev representor ID with target devargs.
>> + *
>> + * xxxx xxxx xxxx xxxx
>> + * |||| |LLL LLLL LLLL vf/sf id
>> + * |||| L 1:sf, 0:vf
>> + * ||LL pf id
>> + * LL controller(host) id
>> + */
>> +#define RTE_ETH_REPR(c, pf, sf, port) \
>> + ((((c) & 3) << 14) | \
>> + (((pf) & 3) << 12) | \
>> + (!!(sf) << 11) | \
>> + ((port) & 0x7ff))
>> +/** Get 'pf' port id from representor ID */ #define
>> +RTE_ETH_REPR_PF(repr) (((repr) >> 12) & 3)
>> +/** Get 'vf' or 'sf' port from representor ID */ #define
>> +RTE_ETH_REPR_PORT(repr) ((repr) & 0x7ff)
>> +
>> /**
>> * PMD helper function to parse ethdev arguments
>> *
>>
