[v5,7/9] devarg: change representor ID to bitmap
Checks
Commit Message
The NIC can have multiple PCIe links and can be attached to multiple
hosts, for example the same single NIC can be shared for multiple server
units in the rack. On each PCIe link NIC can provide multiple PFs and
VFs/SFs based on these ones. The full representor identifier consists of
three indices - controller index, PF index, and VF or SF index (if any).
SR-IOV and SubFunction are created on top of PF. PF index is introduced
because there might be multiple PFs in the bonding configuration and
only bonding device is probed.
In eth representor comparator callback, ethdev was compared with devarg.
Since ethdev representor port didn't contain controller index and PF
index information, callback returned representor from other PF or
controller.
This patch changes representor ID to bitmap so that the ethdev
representor comparer callback returns correct ethdev by comparing full
representor information including: controller index, PF index,
representor type, SF or VF index.
Representor ID bitmap definition:
xxxx xxxx xxxx xxxx
|||| |LLL LLLL LLLL vf/sf id
|||| L 1:sf, 0:vf
||LL pf id
LL controller(host) id
This approach keeps binary compatibility with all drivers, VF
representor id matches with simple id for non-bonding and non-multi-host
configurations.
In the future, the representor ID field and each section should extend
to bigger width to support more devices.
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
drivers/net/bnxt/bnxt_reps.c | 3 +-
drivers/net/enic/enic_vf_representor.c | 3 +-
drivers/net/i40e/i40e_vf_representor.c | 3 +-
drivers/net/ixgbe/ixgbe_vf_representor.c | 3 +-
drivers/net/mlx5/linux/mlx5_os.c | 4 +-
lib/librte_ethdev/rte_class_eth.c | 38 +++++++++++++----
lib/librte_ethdev/rte_ethdev.c | 26 ++++++++++++
lib/librte_ethdev/rte_ethdev_driver.h | 53 ++++++++++++++++++++++++
lib/librte_ethdev/version.map | 2 +
9 files changed, 122 insertions(+), 13 deletions(-)
Comments
> -----Original Message-----
> From: Xueming Li <xuemingl@nvidia.com>
> Sent: Tuesday, January 19, 2021 15:15
> Cc: dev@dpdk.org; Viacheslav Ovsiienko <viacheslavo@nvidia.com>; xuemingl@nvidia.com; Asaf Penso
> <asafp@nvidia.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Daley, John <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>;
> Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Thomas
> Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
> Subject: [PATCH v5 7/9] devarg: change representor ID to bitmap
>
> The NIC can have multiple PCIe links and can be attached to multiple
> hosts, for example the same single NIC can be shared for multiple server
> units in the rack. On each PCIe link NIC can provide multiple PFs and
> VFs/SFs based on these ones. The full representor identifier consists of
> three indices - controller index, PF index, and VF or SF index (if any).
>
> SR-IOV and SubFunction are created on top of PF. PF index is introduced
> because there might be multiple PFs in the bonding configuration and
> only bonding device is probed.
>
> In eth representor comparator callback, ethdev was compared with devarg.
> Since ethdev representor port didn't contain controller index and PF
> index information, callback returned representor from other PF or
> controller.
>
> This patch changes representor ID to bitmap so that the ethdev
> representor comparer callback returns correct ethdev by comparing full
> representor information including: controller index, PF index,
> representor type, SF or VF index.
>
> Representor ID bitmap definition:
> xxxx xxxx xxxx xxxx
> |||| |LLL LLLL LLLL vf/sf id
> |||| L 1:sf, 0:vf
> ||LL pf id
> LL controller(host) id
>
> This approach keeps binary compatibility with all drivers, VF
> representor id matches with simple id for non-bonding and non-multi-host
> configurations.
>
> In the future, the representor ID field and each section should extend
> to bigger width to support more devices.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> drivers/net/bnxt/bnxt_reps.c | 3 +-
> drivers/net/enic/enic_vf_representor.c | 3 +-
> drivers/net/i40e/i40e_vf_representor.c | 3 +-
> drivers/net/ixgbe/ixgbe_vf_representor.c | 3 +-
> drivers/net/mlx5/linux/mlx5_os.c | 4 +-
> lib/librte_ethdev/rte_class_eth.c | 38 +++++++++++++----
> lib/librte_ethdev/rte_ethdev.c | 26 ++++++++++++
> lib/librte_ethdev/rte_ethdev_driver.h | 53 ++++++++++++++++++++++++
> lib/librte_ethdev/version.map | 2 +
> 9 files changed, 122 insertions(+), 13 deletions(-)
>
> +uint16_t
> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
> + enum rte_eth_representor_type type,
> + uint16_t representor_port)
> +{
> + return (((controller & 3) << 14) |
> + ((pf & 3) << 12) |
> + (!!(type == RTE_ETH_REPRESENTOR_SF) << 11) |
> + (representor_port & 0x7ff));
> +}
> +
> +uint16_t
> +rte_eth_representor_id_parse(const uint16_t representor_id,
> + uint16_t *controller, uint16_t *pf,
> + enum rte_eth_representor_type *type)
> +{
> + if (controller)
> + *controller = (representor_id >> 14) & 3;
> + if (pf)
> + *pf = (representor_id >> 12) & 3;
> + if (type)
> + *type = ((representor_id >> 11) & 1) ?
> + RTE_ETH_REPRESENTOR_SF : RTE_ETH_REPRESENTOR_VF;
> + return representor_id & 0x7ff;
> +}
> +
Rename 'parse' to 'decode' ? Since not parse from string. ;-)
The these two functions are pair style.
rte_eth_representor_id_encode / rte_eth_representor_id_decode ?
> 2.25.1
Hi Haiyue,
>-----Original Message-----
>From: Wang, Haiyue <haiyue.wang@intel.com>
>Sent: Tuesday, January 19, 2021 3:37 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>;
>Somnath Kotur <somnath.kotur@broadcom.com>; Daley, John
><johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Xing, Beilei
><beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Matan Azrad
><matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; NBU-Contact-
>Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
><ferruh.yigit@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>; Neil
>Horman <nhorman@tuxdriver.com>
>Subject: RE: [PATCH v5 7/9] devarg: change representor ID to bitmap
>
>> -----Original Message-----
>> From: Xueming Li <xuemingl@nvidia.com>
>> Sent: Tuesday, January 19, 2021 15:15
>> Cc: dev@dpdk.org; Viacheslav Ovsiienko <viacheslavo@nvidia.com>;
>> xuemingl@nvidia.com; Asaf Penso <asafp@nvidia.com>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>> <somnath.kotur@broadcom.com>; Daley, John <johndale@cisco.com>;
>Hyong
>> Youb Kim <hyonkim@cisco.com>; Xing, Beilei <beilei.xing@intel.com>;
>> Guo, Jia <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
>> Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ray Kinsella <mdr@ashroe.eu>; Neil
>> Horman <nhorman@tuxdriver.com>
>> Subject: [PATCH v5 7/9] devarg: change representor ID to bitmap
>>
>> The NIC can have multiple PCIe links and can be attached to multiple
>> hosts, for example the same single NIC can be shared for multiple
>> server units in the rack. On each PCIe link NIC can provide multiple
>> PFs and VFs/SFs based on these ones. The full representor identifier
>> consists of three indices - controller index, PF index, and VF or SF index (if
>any).
>>
>> SR-IOV and SubFunction are created on top of PF. PF index is
>> introduced because there might be multiple PFs in the bonding
>> configuration and only bonding device is probed.
>>
>> In eth representor comparator callback, ethdev was compared with devarg.
>> Since ethdev representor port didn't contain controller index and PF
>> index information, callback returned representor from other PF or
>> controller.
>>
>> This patch changes representor ID to bitmap so that the ethdev
>> representor comparer callback returns correct ethdev by comparing full
>> representor information including: controller index, PF index,
>> representor type, SF or VF index.
>>
>> Representor ID bitmap definition:
>> xxxx xxxx xxxx xxxx
>> |||| |LLL LLLL LLLL vf/sf id
>> |||| L 1:sf, 0:vf
>> ||LL pf id
>> LL controller(host) id
>>
>> This approach keeps binary compatibility with all drivers, VF
>> representor id matches with simple id for non-bonding and
>> non-multi-host configurations.
>>
>> In the future, the representor ID field and each section should extend
>> to bigger width to support more devices.
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>> ---
>> drivers/net/bnxt/bnxt_reps.c | 3 +-
>> drivers/net/enic/enic_vf_representor.c | 3 +-
>> drivers/net/i40e/i40e_vf_representor.c | 3 +-
>> drivers/net/ixgbe/ixgbe_vf_representor.c | 3 +-
>> drivers/net/mlx5/linux/mlx5_os.c | 4 +-
>> lib/librte_ethdev/rte_class_eth.c | 38 +++++++++++++----
>> lib/librte_ethdev/rte_ethdev.c | 26 ++++++++++++
>> lib/librte_ethdev/rte_ethdev_driver.h | 53 ++++++++++++++++++++++++
>> lib/librte_ethdev/version.map | 2 +
>> 9 files changed, 122 insertions(+), 13 deletions(-)
>>
>
>
>> +uint16_t
>> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
>> + enum rte_eth_representor_type type,
>> + uint16_t representor_port)
>> +{
>> + return (((controller & 3) << 14) |
>> + ((pf & 3) << 12) |
>> + (!!(type == RTE_ETH_REPRESENTOR_SF) << 11) |
>> + (representor_port & 0x7ff));
>> +}
>> +
>> +uint16_t
>> +rte_eth_representor_id_parse(const uint16_t representor_id,
>> + uint16_t *controller, uint16_t *pf,
>> + enum rte_eth_representor_type *type) {
>> + if (controller)
>> + *controller = (representor_id >> 14) & 3;
>> + if (pf)
>> + *pf = (representor_id >> 12) & 3;
>> + if (type)
>> + *type = ((representor_id >> 11) & 1) ?
>> + RTE_ETH_REPRESENTOR_SF :
>RTE_ETH_REPRESENTOR_VF;
>> + return representor_id & 0x7ff;
>> +}
>> +
>
>Rename 'parse' to 'decode' ? Since not parse from string. ;-) The these two
>functions are pair style.
>
>rte_eth_representor_id_encode / rte_eth_representor_id_decode ?
Nice catch, thanks!
>
>> 2.25.1
On 1/19/21 10:14 AM, Xueming Li wrote:
> The NIC can have multiple PCIe links and can be attached to multiple
> hosts, for example the same single NIC can be shared for multiple server
> units in the rack. On each PCIe link NIC can provide multiple PFs and
> VFs/SFs based on these ones. The full representor identifier consists of
> three indices - controller index, PF index, and VF or SF index (if any).
>
> SR-IOV and SubFunction are created on top of PF. PF index is introduced
> because there might be multiple PFs in the bonding configuration and
> only bonding device is probed.
>
> In eth representor comparator callback, ethdev was compared with devarg.
> Since ethdev representor port didn't contain controller index and PF
> index information, callback returned representor from other PF or
> controller.
>
> This patch changes representor ID to bitmap so that the ethdev
> representor comparer callback returns correct ethdev by comparing full
> representor information including: controller index, PF index,
> representor type, SF or VF index.
>
> Representor ID bitmap definition:
> xxxx xxxx xxxx xxxx
> |||| |LLL LLLL LLLL vf/sf id
> |||| L 1:sf, 0:vf
> ||LL pf id
> LL controller(host) id
What about PF representor case? I.e. representor for entire PF.
Also it implies that controller ID 0 is the caller. I.e.
special meaning. So, space for just 3 specific contoller left
Similar for PF. In fact it is worse. E.g. PMD is bound to the
second PF (PF number 1). If so, vf0 means the first VF of the
PF itself, i.e. PF 1 VF 0. But, pf0vf0 should mean PF 1 VF 1.
>
> This approach keeps binary compatibility with all drivers, VF
> representor id matches with simple id for non-bonding and non-multi-host
> configurations.
>
> In the future, the representor ID field and each section should extend
> to bigger width to support more devices.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
> index efe6149df5..994db96960 100644
> --- a/lib/librte_ethdev/rte_class_eth.c
> +++ b/lib/librte_ethdev/rte_class_eth.c
> @@ -66,8 +66,8 @@ eth_representor_cmp(const char *key __rte_unused,
> int ret;
> char *values;
> const struct rte_eth_dev_data *data = opaque;
> - struct rte_eth_devargs representors;
> - uint16_t index;
> + struct rte_eth_devargs eth_da;
> + uint16_t index, c, p, f;
>
> if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
> return -1; /* not a representor port */
> @@ -76,17 +76,39 @@ eth_representor_cmp(const char *key __rte_unused,
> values = strdup(value);
> if (values == NULL)
> return -1;
> - memset(&representors, 0, sizeof(representors));
> - ret = rte_eth_devargs_parse_representor_ports(values, &representors);
> + memset(ð_da, 0, sizeof(eth_da));
> + ret = rte_eth_devargs_parse_representor_ports(values, ð_da);
> free(values);
> if (ret != 0)
> return -1; /* invalid devargs value */
>
> + /* Set default values. */
> + if (eth_da.nb_mh_controllers == 0) {
> + eth_da.nb_mh_controllers = 1;
> + eth_da.mh_controllers[0] = 0;
> + }
> + if (eth_da.nb_ports == 0) {
> + eth_da.nb_ports = 1;
> + eth_da.ports[0] = 0;
> + }
> + if (eth_da.nb_representor_ports == 0) {
> + eth_da.nb_representor_ports = 1;
> + eth_da.representor_ports[0] = 0;
> + }
> /* Return 0 if representor id is matching one of the values. */
> - for (index = 0; index < representors.nb_representor_ports; index++)
> - if (data->representor_id ==
> - representors.representor_ports[index])
> - return 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) {
> + index = rte_eth_representor_id_encode(
> + eth_da.mh_controllers[c],
> + eth_da.ports[p],
> + eth_da.type,
> + eth_da.representor_ports[f]);
> + if (data->representor_id == index)
> + return 0;
> + }
> + }
> + }
> return -1; /* no match */
> }
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 91b3263338..2cac0ccfbd 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -5575,6 +5575,32 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> return result;
> }
>
> +uint16_t
> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
> + enum rte_eth_representor_type type,
> + uint16_t representor_port)
> +{
> + return (((controller & 3) << 14) |
> + ((pf & 3) << 12) |
> + (!!(type == RTE_ETH_REPRESENTOR_SF) << 11) |
> + (representor_port & 0x7ff));
> +}
> +
> +uint16_t
> +rte_eth_representor_id_parse(const uint16_t representor_id,
> + uint16_t *controller, uint16_t *pf,
> + enum rte_eth_representor_type *type)
> +{
> + if (controller)
Compare vs NULL in accordance with DPDK coding style guide
> + *controller = (representor_id >> 14) & 3;
> + if (pf)
Compare vs NULL in accordance with DPDK coding style guide
> + *pf = (representor_id >> 12) & 3;
> + if (type)
Compare vs NULL in accordance with DPDK coding style guide
> + *type = ((representor_id >> 11) & 1) ?
> + RTE_ETH_REPRESENTOR_SF : RTE_ETH_REPRESENTOR_VF;
> + return representor_id & 0x7ff;
> +}
> +
> static int
> eth_dev_handle_port_list(const char *cmd __rte_unused,
> const char *params __rte_unused,
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index b01f118965..57253c8f90 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -1218,6 +1218,59 @@ struct rte_eth_devargs {
> enum rte_eth_representor_type type; /* type of representor */
> };
>
> +#define RTE_NO_REPRESENTOR_ID UINT16_MAX /**< No representor ID. */
> +
> +/**
> + * PMD helper function to encode representor 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
> + *
> + * @param controller
> + * Controller ID.
> + * @param pf
> + * PF port ID.
> + * @param type
> + * Representor type.
> + * @param representor_port
> + * Representor port ID.
> + *
> + * @return
> + * Encoded representor ID.
> + */
> +__rte_internal
> +uint16_t
> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
> + enum rte_eth_representor_type type,
> + uint16_t representor_port);
> +
> +/**
> + * PMD helper function to parse representor ID
> + *
> + * @param representor_id
> + * Representor ID.
> + * @param controller
> + * Parsed controller ID.
> + * @param pf
> + * Parsed PF port ID.
> + * @param type
> + * Parsed representor type.
> + *
> + * @return
> + * Parsed representor port ID.
> + */
> +__rte_internal
> +uint16_t
> +rte_eth_representor_id_parse(const uint16_t representor_id,
> + uint16_t *controller, uint16_t *pf,
> + enum rte_eth_representor_type *type);
> +
> /**
> * PMD helper function to parse ethdev arguments
> *
> diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> index d3f5410806..44edaed507 100644
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -257,6 +257,8 @@ INTERNAL {
> rte_eth_dev_release_port;
> rte_eth_dev_internal_reset;
> rte_eth_devargs_parse;
> + rte_eth_representor_id_encode;
> + rte_eth_representor_id_parse;
The place looks wrong. It must be sorted or added at bottom
with the comment with version number. I'd prefer to have
it simply alphanumberically sorted.
> rte_eth_dma_zone_free;
> rte_eth_dma_zone_reserve;
> rte_eth_hairpin_queue_peer_bind;
>
19/01/2021 09:20, Andrew Rybchenko:
> On 1/19/21 10:14 AM, Xueming Li wrote:
> > --- a/lib/librte_ethdev/version.map
> > +++ b/lib/librte_ethdev/version.map
> > @@ -257,6 +257,8 @@ INTERNAL {
> > rte_eth_dev_release_port;
> > rte_eth_dev_internal_reset;
> > rte_eth_devargs_parse;
> > + rte_eth_representor_id_encode;
> > + rte_eth_representor_id_parse;
>
> The place looks wrong. It must be sorted or added at bottom
> with the comment with version number. I'd prefer to have
> it simply alphanumberically sorted.
We add a version number for experimental symbols to help
moving old ones to stable.
In this case, they are internal symbols, so we don't need version,
but you're right it must be alphabetically sorted.
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 4:21 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>;
>Somnath Kotur <somnath.kotur@broadcom.com>; John Daley
><johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
><beilei.xing@intel.com>; Jeff Guo <jia.guo@intel.com>; Haiyue Wang
><haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler
><shahafs@nvidia.com>; NBU-Contact-Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Ray Kinsella
><mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
>Subject: Re: [PATCH v5 7/9] devarg: change representor ID to bitmap
>
>On 1/19/21 10:14 AM, Xueming Li wrote:
>> The NIC can have multiple PCIe links and can be attached to multiple
>> hosts, for example the same single NIC can be shared for multiple
>> server units in the rack. On each PCIe link NIC can provide multiple
>> PFs and VFs/SFs based on these ones. The full representor identifier
>> consists of three indices - controller index, PF index, and VF or SF index (if
>any).
>>
>> SR-IOV and SubFunction are created on top of PF. PF index is
>> introduced because there might be multiple PFs in the bonding
>> configuration and only bonding device is probed.
>>
>> In eth representor comparator callback, ethdev was compared with devarg.
>> Since ethdev representor port didn't contain controller index and PF
>> index information, callback returned representor from other PF or
>> controller.
>>
>> This patch changes representor ID to bitmap so that the ethdev
>> representor comparer callback returns correct ethdev by comparing full
>> representor information including: controller index, PF index,
>> representor type, SF or VF index.
>>
>> Representor ID bitmap definition:
>> xxxx xxxx xxxx xxxx
>> |||| |LLL LLLL LLLL vf/sf id
>> |||| L 1:sf, 0:vf
>> ||LL pf id
>> LL controller(host) id
>
>What about PF representor case? I.e. representor for entire PF.
>
>Also it implies that controller ID 0 is the caller. I.e.
>special meaning. So, space for just 3 specific contoller left
>
>Similar for PF. In fact it is worse. E.g. PMD is bound to the second PF (PF
>number 1). If so, vf0 means the first VF of the PF itself, i.e. PF 1 VF 0. But,
>pf0vf0 should mean PF 1 VF 1.
Agree, need to extend bits width in LTS release.
PF representor is not considered here, how about moving one bit from vf/sf id?
1k SF devices should be fine for me so far.
The controller ID and PF ID is related to the context device, how device configured
and bonding state is critical for PMD to interpret the IDs. For example:
"<BDF>,representor=pf1vf1" is valid for bonding device, invalid for standalone device.
"c#" is meaningful for multi-host scenario, invalid for normal NIC. PMD is responsible to
encode representor ID correctly according to device configuration to make Device+ReprID
unique, because the ReprID is used in device iterator. So the user app should specify
representor syntax with necessary parts to cover device configuration, PMD should
extract required info according to device state.
>
>>
>> This approach keeps binary compatibility with all drivers, VF
>> representor id matches with simple id for non-bonding and
>> non-multi-host configurations.
>>
>> In the future, the representor ID field and each section should extend
>> to bigger width to support more devices.
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
>
>> diff --git a/lib/librte_ethdev/rte_class_eth.c
>> b/lib/librte_ethdev/rte_class_eth.c
>> index efe6149df5..994db96960 100644
>> --- a/lib/librte_ethdev/rte_class_eth.c
>> +++ b/lib/librte_ethdev/rte_class_eth.c
>> @@ -66,8 +66,8 @@ eth_representor_cmp(const char *key __rte_unused,
>> int ret;
>> char *values;
>> const struct rte_eth_dev_data *data = opaque;
>> - struct rte_eth_devargs representors;
>> - uint16_t index;
>> + struct rte_eth_devargs eth_da;
>> + uint16_t index, c, p, f;
>>
>> if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
>> return -1; /* not a representor port */ @@ -76,17 +76,39 @@
>> eth_representor_cmp(const char *key __rte_unused,
>> values = strdup(value);
>> if (values == NULL)
>> return -1;
>> - memset(&representors, 0, sizeof(representors));
>> - ret = rte_eth_devargs_parse_representor_ports(values,
>&representors);
>> + memset(ð_da, 0, sizeof(eth_da));
>> + ret = rte_eth_devargs_parse_representor_ports(values, ð_da);
>> free(values);
>> if (ret != 0)
>> return -1; /* invalid devargs value */
>>
>> + /* Set default values. */
>> + if (eth_da.nb_mh_controllers == 0) {
>> + eth_da.nb_mh_controllers = 1;
>> + eth_da.mh_controllers[0] = 0;
>> + }
>> + if (eth_da.nb_ports == 0) {
>> + eth_da.nb_ports = 1;
>> + eth_da.ports[0] = 0;
>> + }
>> + if (eth_da.nb_representor_ports == 0) {
>> + eth_da.nb_representor_ports = 1;
>> + eth_da.representor_ports[0] = 0;
>> + }
>> /* Return 0 if representor id is matching one of the values. */
>> - for (index = 0; index < representors.nb_representor_ports; index++)
>> - if (data->representor_id ==
>> - representors.representor_ports[index])
>> - return 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) {
>> + index = rte_eth_representor_id_encode(
>> + eth_da.mh_controllers[c],
>> + eth_da.ports[p],
>> + eth_da.type,
>> + eth_da.representor_ports[f]);
>> + if (data->representor_id == index)
>> + return 0;
>> + }
>> + }
>> + }
>> return -1; /* no match */
>> }
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>> b/lib/librte_ethdev/rte_ethdev.c index 91b3263338..2cac0ccfbd 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -5575,6 +5575,32 @@ rte_eth_devargs_parse(const char *dargs, struct
>rte_eth_devargs *eth_da)
>> return result;
>> }
>>
>> +uint16_t
>> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
>> + enum rte_eth_representor_type type,
>> + uint16_t representor_port)
>> +{
>> + return (((controller & 3) << 14) |
>> + ((pf & 3) << 12) |
>> + (!!(type == RTE_ETH_REPRESENTOR_SF) << 11) |
>> + (representor_port & 0x7ff));
>> +}
>> +
>> +uint16_t
>> +rte_eth_representor_id_parse(const uint16_t representor_id,
>> + uint16_t *controller, uint16_t *pf,
>> + enum rte_eth_representor_type *type) {
>> + if (controller)
>
>Compare vs NULL in accordance with DPDK coding style guide
>
>> + *controller = (representor_id >> 14) & 3;
>> + if (pf)
>
>Compare vs NULL in accordance with DPDK coding style guide
>
>> + *pf = (representor_id >> 12) & 3;
>> + if (type)
>
>Compare vs NULL in accordance with DPDK coding style guide
>
>> + *type = ((representor_id >> 11) & 1) ?
>> + RTE_ETH_REPRESENTOR_SF :
>RTE_ETH_REPRESENTOR_VF;
>> + return representor_id & 0x7ff;
>> +}
>> +
>> static int
>> eth_dev_handle_port_list(const char *cmd __rte_unused,
>> const char *params __rte_unused,
>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>> b/lib/librte_ethdev/rte_ethdev_driver.h
>> index b01f118965..57253c8f90 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -1218,6 +1218,59 @@ struct rte_eth_devargs {
>> enum rte_eth_representor_type type; /* type of representor */ };
>>
>> +#define RTE_NO_REPRESENTOR_ID UINT16_MAX /**< No representor ID.
>*/
>> +
>> +/**
>> + * PMD helper function to encode representor 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
>> + *
>> + * @param controller
>> + * Controller ID.
>> + * @param pf
>> + * PF port ID.
>> + * @param type
>> + * Representor type.
>> + * @param representor_port
>> + * Representor port ID.
>> + *
>> + * @return
>> + * Encoded representor ID.
>> + */
>> +__rte_internal
>> +uint16_t
>> +rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
>> + enum rte_eth_representor_type type,
>> + uint16_t representor_port);
>> +
>> +/**
>> + * PMD helper function to parse representor ID
>> + *
>> + * @param representor_id
>> + * Representor ID.
>> + * @param controller
>> + * Parsed controller ID.
>> + * @param pf
>> + * Parsed PF port ID.
>> + * @param type
>> + * Parsed representor type.
>> + *
>> + * @return
>> + * Parsed representor port ID.
>> + */
>> +__rte_internal
>> +uint16_t
>> +rte_eth_representor_id_parse(const uint16_t representor_id,
>> + uint16_t *controller, uint16_t *pf,
>> + enum rte_eth_representor_type *type);
>> +
>> /**
>> * PMD helper function to parse ethdev arguments
>> *
>> diff --git a/lib/librte_ethdev/version.map
>> b/lib/librte_ethdev/version.map index d3f5410806..44edaed507 100644
>> --- a/lib/librte_ethdev/version.map
>> +++ b/lib/librte_ethdev/version.map
>> @@ -257,6 +257,8 @@ INTERNAL {
>> rte_eth_dev_release_port;
>> rte_eth_dev_internal_reset;
>> rte_eth_devargs_parse;
>> + rte_eth_representor_id_encode;
>> + rte_eth_representor_id_parse;
>
>The place looks wrong. It must be sorted or added at bottom with the
>comment with version number. I'd prefer to have it simply alphanumberically
>sorted.
>
>> rte_eth_dma_zone_free;
>> rte_eth_dma_zone_reserve;
>> rte_eth_hairpin_queue_peer_bind;
>>
On 1/19/21 2:04 PM, Xueming(Steven) Li wrote:
>
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, January 19, 2021 4:21 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
>> <asafp@nvidia.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>;
>> Somnath Kotur <somnath.kotur@broadcom.com>; John Daley
>> <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
>> <beilei.xing@intel.com>; Jeff Guo <jia.guo@intel.com>; Haiyue Wang
>> <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler
>> <shahafs@nvidia.com>; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Ray Kinsella
>> <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>
>> Subject: Re: [PATCH v5 7/9] devarg: change representor ID to bitmap
>>
>> On 1/19/21 10:14 AM, Xueming Li wrote:
>>> The NIC can have multiple PCIe links and can be attached to multiple
>>> hosts, for example the same single NIC can be shared for multiple
>>> server units in the rack. On each PCIe link NIC can provide multiple
>>> PFs and VFs/SFs based on these ones. The full representor identifier
>>> consists of three indices - controller index, PF index, and VF or SF index (if
>> any).
>>>
>>> SR-IOV and SubFunction are created on top of PF. PF index is
>>> introduced because there might be multiple PFs in the bonding
>>> configuration and only bonding device is probed.
>>>
>>> In eth representor comparator callback, ethdev was compared with devarg.
>>> Since ethdev representor port didn't contain controller index and PF
>>> index information, callback returned representor from other PF or
>>> controller.
>>>
>>> This patch changes representor ID to bitmap so that the ethdev
>>> representor comparer callback returns correct ethdev by comparing full
>>> representor information including: controller index, PF index,
>>> representor type, SF or VF index.
>>>
>>> Representor ID bitmap definition:
>>> xxxx xxxx xxxx xxxx
>>> |||| |LLL LLLL LLLL vf/sf id
>>> |||| L 1:sf, 0:vf
>>> ||LL pf id
>>> LL controller(host) id
>>
>> What about PF representor case? I.e. representor for entire PF.
>>
>> Also it implies that controller ID 0 is the caller. I.e.
>> special meaning. So, space for just 3 specific contoller left
>>
>> Similar for PF. In fact it is worse. E.g. PMD is bound to the second PF (PF
>> number 1). If so, vf0 means the first VF of the PF itself, i.e. PF 1 VF 0. But,
>> pf0vf0 should mean PF 1 VF 1.
>
> Agree, need to extend bits width in LTS release.
See my reply to cover mail.
> PF representor is not considered here, how about moving one bit from vf/sf id?
> 1k SF devices should be fine for me so far.
We could reserve max VF/SF number to denote PF itself.
> The controller ID and PF ID is related to the context device, how device configured
> and bonding state is critical for PMD to interpret the IDs. For example:
> "<BDF>,representor=pf1vf1" is valid for bonding device, invalid for standalone device.
I guess it is mlx5 specific. IMHO, pf1vf1 makes sense even
without bonding.
> "c#" is meaningful for multi-host scenario, invalid for normal NIC. PMD is responsible to
> encode representor ID correctly according to device configuration to make Device+ReprID
> unique, because the ReprID is used in device iterator. So the user app should specify
> representor syntax with necessary parts to cover device configuration, PMD should
> extract required info according to device state.
See cover mail reply.
@@ -186,7 +186,8 @@ int bnxt_representor_init(struct rte_eth_dev *eth_dev, void *params)
eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
- eth_dev->data->representor_id = rep_params->vf_id;
+ eth_dev->data->representor_id = rte_eth_representor_id_encode(
+ 0, 0, RTE_ETH_REPRESENTOR_VF, rep_params->vf_id);
rte_eth_random_addr(vf_rep_bp->dflt_mac_addr);
memcpy(vf_rep_bp->mac_addr, vf_rep_bp->dflt_mac_addr,
@@ -674,7 +674,8 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, void *init_params)
eth_dev->dev_ops = &enic_vf_representor_dev_ops;
eth_dev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
- eth_dev->data->representor_id = vf->vf_id;
+ eth_dev->data->representor_id = rte_eth_representor_id_encode(
+ 0, 0, RTE_ETH_REPRESENTOR_VF, vf->vf_id);
eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr_vf",
sizeof(struct rte_ether_addr) *
ENIC_UNICAST_PERFECT_FILTERS, 0);
@@ -510,7 +510,8 @@ i40e_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR |
RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
- ethdev->data->representor_id = representor->vf_id;
+ ethdev->data->representor_id = rte_eth_representor_id_encode(
+ 0, 0, RTE_ETH_REPRESENTOR_VF, representor->vf_id);
/* Setting the number queues allocated to the VF */
ethdev->data->nb_rx_queues = vf->vsi->nb_qps;
@@ -196,7 +196,8 @@ ixgbe_vf_representor_init(struct rte_eth_dev *ethdev, void *init_params)
return -ENODEV;
ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
- ethdev->data->representor_id = representor->vf_id;
+ ethdev->data->representor_id = rte_eth_representor_id_encode(
+ 0, 0, RTE_ETH_REPRESENTOR_VF, representor->vf_id);
/* Set representor device ops */
ethdev->dev_ops = &ixgbe_vf_representor_dev_ops;
@@ -1025,7 +1025,9 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
#endif
/* representor_id field keeps the unmodified VF index. */
priv->representor_id = switch_info->representor ?
- switch_info->port_name : -1;
+ rte_eth_representor_id_encode(0, 0, RTE_ETH_REPRESENTOR_VF,
+ switch_info->port_name) :
+ -1;
/*
* Look for sibling devices in order to reuse their switch domain
* if any, otherwise allocate one.
@@ -66,8 +66,8 @@ eth_representor_cmp(const char *key __rte_unused,
int ret;
char *values;
const struct rte_eth_dev_data *data = opaque;
- struct rte_eth_devargs representors;
- uint16_t index;
+ struct rte_eth_devargs eth_da;
+ uint16_t index, c, p, f;
if ((data->dev_flags & RTE_ETH_DEV_REPRESENTOR) == 0)
return -1; /* not a representor port */
@@ -76,17 +76,39 @@ eth_representor_cmp(const char *key __rte_unused,
values = strdup(value);
if (values == NULL)
return -1;
- memset(&representors, 0, sizeof(representors));
- ret = rte_eth_devargs_parse_representor_ports(values, &representors);
+ memset(ð_da, 0, sizeof(eth_da));
+ ret = rte_eth_devargs_parse_representor_ports(values, ð_da);
free(values);
if (ret != 0)
return -1; /* invalid devargs value */
+ /* Set default values. */
+ if (eth_da.nb_mh_controllers == 0) {
+ eth_da.nb_mh_controllers = 1;
+ eth_da.mh_controllers[0] = 0;
+ }
+ if (eth_da.nb_ports == 0) {
+ eth_da.nb_ports = 1;
+ eth_da.ports[0] = 0;
+ }
+ if (eth_da.nb_representor_ports == 0) {
+ eth_da.nb_representor_ports = 1;
+ eth_da.representor_ports[0] = 0;
+ }
/* Return 0 if representor id is matching one of the values. */
- for (index = 0; index < representors.nb_representor_ports; index++)
- if (data->representor_id ==
- representors.representor_ports[index])
- return 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) {
+ index = rte_eth_representor_id_encode(
+ eth_da.mh_controllers[c],
+ eth_da.ports[p],
+ eth_da.type,
+ eth_da.representor_ports[f]);
+ if (data->representor_id == index)
+ return 0;
+ }
+ }
+ }
return -1; /* no match */
}
@@ -5575,6 +5575,32 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
return result;
}
+uint16_t
+rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
+ enum rte_eth_representor_type type,
+ uint16_t representor_port)
+{
+ return (((controller & 3) << 14) |
+ ((pf & 3) << 12) |
+ (!!(type == RTE_ETH_REPRESENTOR_SF) << 11) |
+ (representor_port & 0x7ff));
+}
+
+uint16_t
+rte_eth_representor_id_parse(const uint16_t representor_id,
+ uint16_t *controller, uint16_t *pf,
+ enum rte_eth_representor_type *type)
+{
+ if (controller)
+ *controller = (representor_id >> 14) & 3;
+ if (pf)
+ *pf = (representor_id >> 12) & 3;
+ if (type)
+ *type = ((representor_id >> 11) & 1) ?
+ RTE_ETH_REPRESENTOR_SF : RTE_ETH_REPRESENTOR_VF;
+ return representor_id & 0x7ff;
+}
+
static int
eth_dev_handle_port_list(const char *cmd __rte_unused,
const char *params __rte_unused,
@@ -1218,6 +1218,59 @@ struct rte_eth_devargs {
enum rte_eth_representor_type type; /* type of representor */
};
+#define RTE_NO_REPRESENTOR_ID UINT16_MAX /**< No representor ID. */
+
+/**
+ * PMD helper function to encode representor 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
+ *
+ * @param controller
+ * Controller ID.
+ * @param pf
+ * PF port ID.
+ * @param type
+ * Representor type.
+ * @param representor_port
+ * Representor port ID.
+ *
+ * @return
+ * Encoded representor ID.
+ */
+__rte_internal
+uint16_t
+rte_eth_representor_id_encode(uint16_t controller, uint16_t pf,
+ enum rte_eth_representor_type type,
+ uint16_t representor_port);
+
+/**
+ * PMD helper function to parse representor ID
+ *
+ * @param representor_id
+ * Representor ID.
+ * @param controller
+ * Parsed controller ID.
+ * @param pf
+ * Parsed PF port ID.
+ * @param type
+ * Parsed representor type.
+ *
+ * @return
+ * Parsed representor port ID.
+ */
+__rte_internal
+uint16_t
+rte_eth_representor_id_parse(const uint16_t representor_id,
+ uint16_t *controller, uint16_t *pf,
+ enum rte_eth_representor_type *type);
+
/**
* PMD helper function to parse ethdev arguments
*
@@ -257,6 +257,8 @@ INTERNAL {
rte_eth_dev_release_port;
rte_eth_dev_internal_reset;
rte_eth_devargs_parse;
+ rte_eth_representor_id_encode;
+ rte_eth_representor_id_parse;
rte_eth_dma_zone_free;
rte_eth_dma_zone_reserve;
rte_eth_hairpin_queue_peer_bind;