[v4,1/9] ethdev: introduce representor type
Checks
Commit Message
To support more representor type, this patch introduces representor type
enum. The enum is subject to extend for new types upcoming.
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
drivers/net/bnxt/bnxt_ethdev.c | 7 +++++++
drivers/net/enic/enic_ethdev.c | 7 +++++++
drivers/net/i40e/i40e_ethdev.c | 8 ++++++++
drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++++++
drivers/net/mlx5/linux/mlx5_os.c | 11 +++++++++++
lib/librte_ethdev/ethdev_private.c | 5 +++++
lib/librte_ethdev/rte_ethdev_driver.h | 9 +++++++++
7 files changed, 55 insertions(+)
Comments
+Cc more maintainers
18/01/2021 12:16, Xueming Li:
> To support more representor type, this patch introduces representor type
> enum. The enum is subject to extend for new types upcoming.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> drivers/net/bnxt/bnxt_ethdev.c | 7 +++++++
> drivers/net/enic/enic_ethdev.c | 7 +++++++
> drivers/net/i40e/i40e_ethdev.c | 8 ++++++++
> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++++++
> drivers/net/mlx5/linux/mlx5_os.c | 11 +++++++++++
> lib/librte_ethdev/ethdev_private.c | 5 +++++
> lib/librte_ethdev/rte_ethdev_driver.h | 9 +++++++++
> 7 files changed, 55 insertions(+)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 74b0f3d1dc..d7c8b3ec07 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -5586,6 +5586,13 @@ static int bnxt_rep_port_probe(struct rte_pci_device *pci_dev,
> int i, ret = 0;
> struct rte_kvargs *kvlist = NULL;
>
> + if (eth_da->type == RTE_ETH_REPRESENTOR_NONE)
> + return 0;
> + if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
> + PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
> + eth_da->type);
> + return -ENOTSUP;
> + }
> num_rep = eth_da->nb_representor_ports;
> if (num_rep > BNXT_MAX_VF_REPS) {
> PMD_DRV_LOG(ERR, "nb_representor_ports = %d > %d MAX VF REPS\n",
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index d041a6bee9..dd085caa93 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -1303,6 +1303,13 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> if (retval)
> return retval;
> }
> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> + return 0;
> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> + ENICPMD_LOG(ERR, "unsupported representor type: %s\n",
> + pci_dev->device.devargs->args);
> + return -ENOTSUP;
> + }
> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> sizeof(struct enic),
> eth_dev_pci_specific_init, pci_dev,
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index c1c2327b3f..3cea6de2bd 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -638,6 +638,14 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> return retval;
> }
>
> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> + return 0;
> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> + PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
> + pci_dev->device.devargs->args);
> + return -ENOTSUP;
> + }
> +
> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> sizeof(struct i40e_adapter),
> eth_dev_pci_specific_init, pci_dev,
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index d7a1806ab8..eb2c4929e2 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1717,6 +1717,14 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> } else
> memset(ð_da, 0, sizeof(eth_da));
>
> + if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
> + return 0;
> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> + PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
> + pci_dev->device.devargs->args);
> + return -ENOTSUP;
> + }
> +
> retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
> sizeof(struct ixgbe_adapter),
> eth_dev_pci_specific_init, pci_dev,
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
> index 9ac1d46b1b..caead107b0 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -705,6 +705,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> strerror(rte_errno));
> return NULL;
> }
> + if (eth_da.type != RTE_ETH_REPRESENTOR_NONE) {
> + /* Representor not specified. */
> + rte_errno = EBUSY;
> + return NULL;
> + }
> + if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
> + rte_errno = ENOTSUP;
> + DRV_LOG(ERR, "unsupported representor type: %s",
> + dpdk_dev->devargs->args);
> + return NULL;
> + }
> for (i = 0; i < eth_da.nb_representor_ports; ++i)
> if (eth_da.representor_ports[i] ==
> (uint16_t)switch_info->port_name)
> diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
> index 162a502fe7..c1a411dba4 100644
> --- a/lib/librte_ethdev/ethdev_private.c
> +++ b/lib/librte_ethdev/ethdev_private.c
> @@ -111,11 +111,16 @@ rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
> return 0;
> }
>
> +/*
> + * representor format:
> + * #: range or single number of VF representor
> + */
> int
> rte_eth_devargs_parse_representor_ports(char *str, void *data)
> {
> struct rte_eth_devargs *eth_da = data;
>
> + eth_da->type = RTE_ETH_REPRESENTOR_VF;
> return rte_eth_devargs_process_range(str, eth_da->representor_ports,
> ð_da->nb_representor_ports, RTE_MAX_ETHPORTS);
> }
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 0eacfd8425..3bc5c5bbbb 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -1193,6 +1193,14 @@ __rte_internal
> int
> rte_eth_switch_domain_free(uint16_t domain_id);
>
> +/** Ethernet device representor type */
> +enum rte_eth_representor_type {
> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
> +};
> +
> /** Generic Ethernet device arguments */
> struct rte_eth_devargs {
> uint16_t ports[RTE_MAX_ETHPORTS];
> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
> /** representor port/s identifier to enable on device */
> uint16_t nb_representor_ports;
> /** number of ports in representor port field */
> + enum rte_eth_representor_type type; /* type of representor */
> };
On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com> wrote:
>
> To support more representor type, this patch introduces representor type
> enum. The enum is subject to extend for new types upcoming.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> drivers/net/bnxt/bnxt_ethdev.c | 7 +++++++
> drivers/net/enic/enic_ethdev.c | 7 +++++++
> drivers/net/i40e/i40e_ethdev.c | 8 ++++++++
> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++++++
> drivers/net/mlx5/linux/mlx5_os.c | 11 +++++++++++
> lib/librte_ethdev/ethdev_private.c | 5 +++++
> lib/librte_ethdev/rte_ethdev_driver.h | 9 +++++++++
> 7 files changed, 55 insertions(+)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 74b0f3d1dc..d7c8b3ec07 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -5586,6 +5586,13 @@ static int bnxt_rep_port_probe(struct rte_pci_device *pci_dev,
> int i, ret = 0;
> struct rte_kvargs *kvlist = NULL;
>
> + if (eth_da->type == RTE_ETH_REPRESENTOR_NONE)
> + return 0;
> + if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
> + PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
> + eth_da->type);
> + return -ENOTSUP;
> + }
> num_rep = eth_da->nb_representor_ports;
> if (num_rep > BNXT_MAX_VF_REPS) {
> PMD_DRV_LOG(ERR, "nb_representor_ports = %d > %d MAX VF REPS\n",
Ack. Thanks
:::: snip ::::
> +/** Ethernet device representor type */
> +enum rte_eth_representor_type {
> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
Till we get used to the terminology...
Can we also have SF = "Sub Function" mentioned in the docs or comments?
> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
> +};
> +
> /** Generic Ethernet device arguments */
> struct rte_eth_devargs {
> uint16_t ports[RTE_MAX_ETHPORTS];
> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
> /** representor port/s identifier to enable on device */
> uint16_t nb_representor_ports;
> /** number of ports in representor port field */
> + enum rte_eth_representor_type type; /* type of representor */
> };
>
> /**
> --
> 2.25.1
>
18/01/2021 18:42, Ajit Khaparde:
> On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > +enum rte_eth_representor_type {
> > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> Till we get used to the terminology...
> Can we also have SF = "Sub Function" mentioned in the docs or comments?
Are we sure about the definition?
I remember seeing SF = Scalable Function somewhere else (maybe from Intel)
On Mon, Jan 18, 2021 at 9:57 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 18/01/2021 18:42, Ajit Khaparde:
> > On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > > +enum rte_eth_representor_type {
> > > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> > > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> > > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> > Till we get used to the terminology...
> > Can we also have SF = "Sub Function" mentioned in the docs or comments?
>
> Are we sure about the definition?
> I remember seeing SF = Scalable Function somewhere else (maybe from Intel)
That complicates it. But if they mean the same thing, let's pick one.
18/01/2021 19:00, Ajit Khaparde:
> On Mon, Jan 18, 2021 at 9:57 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 18/01/2021 18:42, Ajit Khaparde:
> > > On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > > > +enum rte_eth_representor_type {
> > > > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> > > > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> > > > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> > > Till we get used to the terminology...
> > > Can we also have SF = "Sub Function" mentioned in the docs or comments?
> >
> > Are we sure about the definition?
> > I remember seeing SF = Scalable Function somewhere else (maybe from Intel)
> That complicates it. But if they mean the same thing, let's pick one.
I think "Sub Function" and "Virtual Function" are easy to understand
for everybody.
I suggest picking these two for comments above.
On Mon, Jan 18, 2021 at 10:15 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 18/01/2021 19:00, Ajit Khaparde:
> > On Mon, Jan 18, 2021 at 9:57 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 18/01/2021 18:42, Ajit Khaparde:
> > > > On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > > > > +enum rte_eth_representor_type {
> > > > > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> > > > > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> > > > > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> > > > Till we get used to the terminology...
> > > > Can we also have SF = "Sub Function" mentioned in the docs or comments?
> > >
> > > Are we sure about the definition?
> > > I remember seeing SF = Scalable Function somewhere else (maybe from Intel)
> > That complicates it. But if they mean the same thing, let's pick one.
>
> I think "Sub Function" and "Virtual Function" are easy to understand
> for everybody.
> I suggest picking these two for comments above.
+1
>
>-----Original Message-----
>From: Ajit Khaparde <ajit.khaparde@broadcom.com>
>Sent: Tuesday, January 19, 2021 2:18 AM
>To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
>Cc: Xueming(Steven) Li <xuemingl@nvidia.com>; Ferruh Yigit
><ferruh.yigit@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>;
>dpdk-dev <dev@dpdk.org>; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
>Penso <asafp@nvidia.com>
>Subject: Re: [dpdk-dev] [PATCH v4 1/9] ethdev: introduce representor type
>
>On Mon, Jan 18, 2021 at 10:15 AM Thomas Monjalon <thomas@monjalon.net>
>wrote:
>>
>> 18/01/2021 19:00, Ajit Khaparde:
>> > On Mon, Jan 18, 2021 at 9:57 AM Thomas Monjalon
><thomas@monjalon.net> wrote:
>> > > 18/01/2021 18:42, Ajit Khaparde:
>> > > > On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com>
>wrote:
>> > > > > +enum rte_eth_representor_type {
>> > > > > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
>> > > > > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
>> > > > > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
>> > > > Till we get used to the terminology...
>> > > > Can we also have SF = "Sub Function" mentioned in the docs or
>comments?
>> > >
>> > > Are we sure about the definition?
>> > > I remember seeing SF = Scalable Function somewhere else (maybe from
>Intel)
>> > That complicates it. But if they mean the same thing, let's pick one.
>>
>> I think "Sub Function" and "Virtual Function" are easy to understand
>> for everybody.
>> I suggest picking these two for comments above.
>+1
There was an internal discussion and the conclusion is to align with kernel driver name.
Will update comment in next version, thanks!
>
>>
On 1/18/21 2:16 PM, Xueming Li wrote:
> To support more representor type, this patch introduces representor type
> enum. The enum is subject to extend for new types upcoming.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
One nit below and a question below.
In any case:
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
[snip]
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 0eacfd8425..3bc5c5bbbb 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -1193,6 +1193,14 @@ __rte_internal
> int
> rte_eth_switch_domain_free(uint16_t domain_id);
>
> +/** Ethernet device representor type */
> +enum rte_eth_representor_type {
> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
IMHO, addition of these members here make future patches
which add support inconsistent.
> +};
> +
> /** Generic Ethernet device arguments */
> struct rte_eth_devargs {
> uint16_t ports[RTE_MAX_ETHPORTS];
> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
> /** representor port/s identifier to enable on device */
> uint16_t nb_representor_ports;
> /** number of ports in representor port field */
> + enum rte_eth_representor_type type; /* type of representor */
Is it intended and documented limitation that we can't add
different type representors in one request? Or am I missing
something and it is possible?
> };
>
> /**
>
Hi Andrew,
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 3:25 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>Olivier Matz <olivier.matz@6wind.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>
>Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>
>On 1/18/21 2:16 PM, Xueming Li wrote:
>> To support more representor type, this patch introduces representor
>> type enum. The enum is subject to extend for new types upcoming.
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>
>One nit below and a question below.
>
>In any case:
>
>Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>
>[snip]
>
>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>> b/lib/librte_ethdev/rte_ethdev_driver.h
>> index 0eacfd8425..3bc5c5bbbb 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -1193,6 +1193,14 @@ __rte_internal
>> int
>> rte_eth_switch_domain_free(uint16_t domain_id);
>>
>> +/** Ethernet device representor type */ enum rte_eth_representor_type
>> +{
>> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
>> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
>> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
>> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
>
>RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
>IMHO, addition of these members here make future patches which add
>support inconsistent.
Yes, later patch in this patchset will support it.
>
>> +};
>> +
>> /** Generic Ethernet device arguments */ struct rte_eth_devargs {
>> uint16_t ports[RTE_MAX_ETHPORTS];
>> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
>> /** representor port/s identifier to enable on device */
>> uint16_t nb_representor_ports;
>> /** number of ports in representor port field */
>> + enum rte_eth_representor_type type; /* type of representor */
>
>Is it intended and documented limitation that we can't add different type
>representors in one request? Or am I missing something and it is possible?
Correct, current devargs structure can't support mix of different types.
I'll update in next version if any.
>
>> };
>>
>> /**
>>
On Mon, Jan 18, 2021 at 3:41 PM Xueming(Steven) Li <xuemingl@nvidia.com> wrote:
>
> >-----Original Message-----
> >From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> >Sent: Tuesday, January 19, 2021 2:18 AM
> >To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> >Cc: Xueming(Steven) Li <xuemingl@nvidia.com>; Ferruh Yigit
> ><ferruh.yigit@intel.com>; Andrew Rybchenko
> ><andrew.rybchenko@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>;
> >dpdk-dev <dev@dpdk.org>; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
> >Penso <asafp@nvidia.com>
> >Subject: Re: [dpdk-dev] [PATCH v4 1/9] ethdev: introduce representor type
> >
> >On Mon, Jan 18, 2021 at 10:15 AM Thomas Monjalon <thomas@monjalon.net>
> >wrote:
> >>
> >> 18/01/2021 19:00, Ajit Khaparde:
> >> > On Mon, Jan 18, 2021 at 9:57 AM Thomas Monjalon
> ><thomas@monjalon.net> wrote:
> >> > > 18/01/2021 18:42, Ajit Khaparde:
> >> > > > On Mon, Jan 18, 2021 at 3:17 AM Xueming Li <xuemingl@nvidia.com>
> >wrote:
> >> > > > > +enum rte_eth_representor_type {
> >> > > > > + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> >> > > > > + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> >> > > > > + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> >> > > > Till we get used to the terminology...
> >> > > > Can we also have SF = "Sub Function" mentioned in the docs or
> >comments?
> >> > >
> >> > > Are we sure about the definition?
> >> > > I remember seeing SF = Scalable Function somewhere else (maybe from
> >Intel)
> >> > That complicates it. But if they mean the same thing, let's pick one.
> >>
> >> I think "Sub Function" and "Virtual Function" are easy to understand
> >> for everybody.
> >> I suggest picking these two for comments above.
> >+1
>
> There was an internal discussion and the conclusion is to align with kernel driver name.
> Will update comment in next version, thanks!
Ok. In that case for the series:
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> >
> >>
On 1/19/21 10:37 AM, Xueming(Steven) Li wrote:
> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, January 19, 2021 3:25 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>> Olivier Matz <olivier.matz@6wind.com>
>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
>> <asafp@nvidia.com>
>> Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>>
>> On 1/18/21 2:16 PM, Xueming Li wrote:
>>> To support more representor type, this patch introduces representor
>>> type enum. The enum is subject to extend for new types upcoming.
>>>
>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>
>> One nit below and a question below.
>>
>> In any case:
>>
>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>> [snip]
>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>>> b/lib/librte_ethdev/rte_ethdev_driver.h
>>> index 0eacfd8425..3bc5c5bbbb 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>> @@ -1193,6 +1193,14 @@ __rte_internal
>>> int
>>> rte_eth_switch_domain_free(uint16_t domain_id);
>>>
>>> +/** Ethernet device representor type */ enum rte_eth_representor_type
>>> +{
>>> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
>>> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
>>> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
>>> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
>>
>> RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
>> IMHO, addition of these members here make future patches which add
>> support inconsistent.
>
> Yes, later patch in this patchset will support it.
I know. The question is why it is not added in the
later patches when these types are actually supported.
>>
>>> +};
>>> +
>>> /** Generic Ethernet device arguments */ struct rte_eth_devargs {
>>> uint16_t ports[RTE_MAX_ETHPORTS];
>>> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
>>> /** representor port/s identifier to enable on device */
>>> uint16_t nb_representor_ports;
>>> /** number of ports in representor port field */
>>> + enum rte_eth_representor_type type; /* type of representor */
>>
>> Is it intended and documented limitation that we can't add different type
>> representors in one request? Or am I missing something and it is possible?
>
> Correct, current devargs structure can't support mix of different types.
> I'll update in next version if any.
>>
>>> };
>>>
>>> /**
>>>
>
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 3:49 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>Olivier Matz <olivier.matz@6wind.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>
>Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>
>On 1/19/21 10:37 AM, Xueming(Steven) Li wrote:
>> Hi Andrew,
>>
>>> -----Original Message-----
>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Sent: Tuesday, January 19, 2021 3:25 PM
>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>> <ferruh.yigit@intel.com>; Olivier Matz <olivier.matz@6wind.com>
>>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
>>> Penso <asafp@nvidia.com>
>>> Subject: Re: [PATCH v4 1/9] ethdev: introduce representor type
>>>
>>> On 1/18/21 2:16 PM, Xueming Li wrote:
>>>> To support more representor type, this patch introduces representor
>>>> type enum. The enum is subject to extend for new types upcoming.
>>>>
>>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>>
>>> One nit below and a question below.
>>>
>>> In any case:
>>>
>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>
>>> [snip]
>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>>>> b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> index 0eacfd8425..3bc5c5bbbb 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> @@ -1193,6 +1193,14 @@ __rte_internal int
>>>> rte_eth_switch_domain_free(uint16_t domain_id);
>>>>
>>>> +/** Ethernet device representor type */ enum
>>>> +rte_eth_representor_type {
>>>> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
>>>> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
>>>> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
>>>> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
>>>
>>> RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
>>> IMHO, addition of these members here make future patches which add
>>> support inconsistent.
>>
>> Yes, later patch in this patchset will support it.
>
>
>I know. The question is why it is not added in the later patches when these
>types are actually supported.
Good suggestion, will update
>
>>>
>>>> +};
>>>> +
>>>> /** Generic Ethernet device arguments */ struct rte_eth_devargs {
>>>> uint16_t ports[RTE_MAX_ETHPORTS];
>>>> @@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
>>>> /** representor port/s identifier to enable on device */
>>>> uint16_t nb_representor_ports;
>>>> /** number of ports in representor port field */
>>>> + enum rte_eth_representor_type type; /* type of representor */
>>>
>>> Is it intended and documented limitation that we can't add different
>>> type representors in one request? Or am I missing something and it is
>possible?
>>
>> Correct, current devargs structure can't support mix of different types.
>> I'll update in next version if any.
>>>
>>>> };
>>>>
>>>> /**
>>>>
>>
19/01/2021 08:56, Xueming(Steven) Li:
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >On 1/19/21 10:37 AM, Xueming(Steven) Li wrote:
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>> On 1/18/21 2:16 PM, Xueming Li wrote:
> >>>> +/** Ethernet device representor type */ enum
> >>>> +rte_eth_representor_type {
> >>>> + RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
> >>>> + RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
> >>>> + RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
> >>>> + RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
> >>>
> >>> RTE_ETH_REPRESENTOR_SF and PF looks dead in the patch.
> >>> IMHO, addition of these members here make future patches which add
> >>> support inconsistent.
> >>
> >> Yes, later patch in this patchset will support it.
> >
> >I know. The question is why it is not added in the later patches when these
> >types are actually supported.
>
> Good suggestion, will update
+1 (I was sure it was already the case)
@@ -5586,6 +5586,13 @@ static int bnxt_rep_port_probe(struct rte_pci_device *pci_dev,
int i, ret = 0;
struct rte_kvargs *kvlist = NULL;
+ if (eth_da->type == RTE_ETH_REPRESENTOR_NONE)
+ return 0;
+ if (eth_da->type != RTE_ETH_REPRESENTOR_VF) {
+ PMD_DRV_LOG(ERR, "unsupported representor type %d\n",
+ eth_da->type);
+ return -ENOTSUP;
+ }
num_rep = eth_da->nb_representor_ports;
if (num_rep > BNXT_MAX_VF_REPS) {
PMD_DRV_LOG(ERR, "nb_representor_ports = %d > %d MAX VF REPS\n",
@@ -1303,6 +1303,13 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
if (retval)
return retval;
}
+ if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
+ return 0;
+ if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
+ ENICPMD_LOG(ERR, "unsupported representor type: %s\n",
+ pci_dev->device.devargs->args);
+ return -ENOTSUP;
+ }
retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
sizeof(struct enic),
eth_dev_pci_specific_init, pci_dev,
@@ -638,6 +638,14 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
return retval;
}
+ if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
+ return 0;
+ if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
+ PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
+ pci_dev->device.devargs->args);
+ return -ENOTSUP;
+ }
+
retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
sizeof(struct i40e_adapter),
eth_dev_pci_specific_init, pci_dev,
@@ -1717,6 +1717,14 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
} else
memset(ð_da, 0, sizeof(eth_da));
+ if (eth_da.type == RTE_ETH_REPRESENTOR_NONE)
+ return 0;
+ if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
+ PMD_DRV_LOG(ERR, "unsupported representor type: %s\n",
+ pci_dev->device.devargs->args);
+ return -ENOTSUP;
+ }
+
retval = rte_eth_dev_create(&pci_dev->device, pci_dev->device.name,
sizeof(struct ixgbe_adapter),
eth_dev_pci_specific_init, pci_dev,
@@ -705,6 +705,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
strerror(rte_errno));
return NULL;
}
+ if (eth_da.type != RTE_ETH_REPRESENTOR_NONE) {
+ /* Representor not specified. */
+ rte_errno = EBUSY;
+ return NULL;
+ }
+ if (eth_da.type != RTE_ETH_REPRESENTOR_VF) {
+ rte_errno = ENOTSUP;
+ DRV_LOG(ERR, "unsupported representor type: %s",
+ dpdk_dev->devargs->args);
+ return NULL;
+ }
for (i = 0; i < eth_da.nb_representor_ports; ++i)
if (eth_da.representor_ports[i] ==
(uint16_t)switch_info->port_name)
@@ -111,11 +111,16 @@ rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
return 0;
}
+/*
+ * representor format:
+ * #: range or single number of VF representor
+ */
int
rte_eth_devargs_parse_representor_ports(char *str, void *data)
{
struct rte_eth_devargs *eth_da = data;
+ eth_da->type = RTE_ETH_REPRESENTOR_VF;
return rte_eth_devargs_process_range(str, eth_da->representor_ports,
ð_da->nb_representor_ports, RTE_MAX_ETHPORTS);
}
@@ -1193,6 +1193,14 @@ __rte_internal
int
rte_eth_switch_domain_free(uint16_t domain_id);
+/** Ethernet device representor type */
+enum rte_eth_representor_type {
+ RTE_ETH_REPRESENTOR_NONE, /**< not a representor. */
+ RTE_ETH_REPRESENTOR_VF, /**< representor of VF. */
+ RTE_ETH_REPRESENTOR_SF, /**< representor of SF. */
+ RTE_ETH_REPRESENTOR_PF, /**< representor of host PF. */
+};
+
/** Generic Ethernet device arguments */
struct rte_eth_devargs {
uint16_t ports[RTE_MAX_ETHPORTS];
@@ -1203,6 +1211,7 @@ struct rte_eth_devargs {
/** representor port/s identifier to enable on device */
uint16_t nb_representor_ports;
/** number of ports in representor port field */
+ enum rte_eth_representor_type type; /* type of representor */
};
/**