diff mbox series

[v4,1/9] ethdev: introduce representor type

Message ID 1610968623-31345-2-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series ethdev: support SubFunction representor | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Xueming Li Jan. 18, 2021, 11:16 a.m. UTC
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

Thomas Monjalon Jan. 18, 2021, 3:44 p.m. UTC | #1
+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(&eth_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,
>  		&eth_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 */
>  };
Ajit Khaparde Jan. 18, 2021, 5:42 p.m. UTC | #2
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
>
Thomas Monjalon Jan. 18, 2021, 5:57 p.m. UTC | #3
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)
Ajit Khaparde Jan. 18, 2021, 6 p.m. UTC | #4
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.
Thomas Monjalon Jan. 18, 2021, 6:15 p.m. UTC | #5
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.
Ajit Khaparde Jan. 18, 2021, 6:17 p.m. UTC | #6
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

>
Xueming Li Jan. 18, 2021, 11:41 p.m. UTC | #7
>-----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!

>
>>
Andrew Rybchenko Jan. 19, 2021, 7:24 a.m. UTC | #8
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?

>  };
>  
>  /**
>
Xueming Li Jan. 19, 2021, 7:37 a.m. UTC | #9
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.
>
>>  };
>>
>>  /**
>>
Ajit Khaparde Jan. 19, 2021, 7:39 a.m. UTC | #10
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>

>
> >
> >>
Andrew Rybchenko Jan. 19, 2021, 7:49 a.m. UTC | #11
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.
>>
>>>  };
>>>
>>>  /**
>>>
>
Xueming Li Jan. 19, 2021, 7:56 a.m. UTC | #12
>-----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.
>>>
>>>>  };
>>>>
>>>>  /**
>>>>
>>
Thomas Monjalon Jan. 19, 2021, 8:39 a.m. UTC | #13
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)
diff mbox series

Patch

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(&eth_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,
 		&eth_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 */
 };
 
 /**