[PATCH 0/2] multiple representors in one device

[PATCH 0/2] multiple representors in one device

[Harman Kalra]

Following series adds support to enable creation of multiple representors
under one base device. There may be scenarios where port representors for
multiple PFs or VFs under PF are required and all these representor ports
created under a single pci device. Marvell CNXK port representor solution
is designed around this scenario where all representors are backed by a
single switch device.

Earlier this change was implemented as part of the Marvell CNXK port
representor series but after suggestions from Thomas we would like
to propose these changes in common code.
https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785

Harman Kalra (2):
  ethdev: parsing multiple representor devargs string
  doc: multiple representors in one device

 doc/guides/prog_guide/poll_mode_drv.rst       |  4 +++-
 .../prog_guide/switch_representation.rst      |  1 +
 drivers/net/bnxt/bnxt_ethdev.c                |  2 +-
 drivers/net/enic/enic_ethdev.c                |  2 +-
 drivers/net/i40e/i40e_ethdev.c                |  2 +-
 drivers/net/ice/ice_dcf_ethdev.c              |  2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |  2 +-
 drivers/net/mlx5/linux/mlx5_os.c              |  4 ++--
 .../net/nfp/flower/nfp_flower_representor.c   |  2 +-
 drivers/net/sfc/sfc_ethdev.c                  |  2 +-
 lib/ethdev/ethdev_driver.c                    | 19 ++++++++-----------
 lib/ethdev/ethdev_driver.h                    |  4 ++--
 12 files changed, 23 insertions(+), 23 deletions(-)

-- 
2.18.0

[PATCH 1/2] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Adding support for parsing multiple representor devargs strings
passed to a PCI BDF. There may be scenario where port representors
for various PFs or VFs under PFs are required and all these are
representor ports shall be backed by single pci device. In such
case port representors can be created using devargs string:
<PCI BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 drivers/net/bnxt/bnxt_ethdev.c                |  2 +-
 drivers/net/enic/enic_ethdev.c                |  2 +-
 drivers/net/i40e/i40e_ethdev.c                |  2 +-
 drivers/net/ice/ice_dcf_ethdev.c              |  2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |  2 +-
 drivers/net/mlx5/linux/mlx5_os.c              |  4 ++--
 .../net/nfp/flower/nfp_flower_representor.c   |  2 +-
 drivers/net/sfc/sfc_ethdev.c                  |  2 +-
 lib/ethdev/ethdev_driver.c                    | 19 ++++++++-----------
 lib/ethdev/ethdev_driver.h                    |  4 ++--
 10 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index acf7e6e46e..bc2fcbc9c6 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6384,7 +6384,7 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	if (pci_dev->device.devargs) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
 					    &eth_da);
-		if (ret)
+		if (ret < 0)
 			return ret;
 	}
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index b04b6c9aa1..62505fe810 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1318,7 +1318,7 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
 				&eth_da);
-		if (retval)
+		if (retval < 0)
 			return retval;
 	}
 	if (eth_da.nb_representor_ports > 0 &&
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..7dad62313b 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -647,7 +647,7 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
 				&eth_da);
-		if (retval)
+		if (retval < 0)
 			return retval;
 	}
 
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 5d845bba31..f908190226 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -2042,7 +2042,7 @@ eth_ice_dcf_pci_probe(__rte_unused struct rte_pci_driver *pci_drv,
 		return 1;
 
 	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	ret = rte_eth_dev_pci_generic_probe(pci_dev,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d6cf00317e..4650cdb369 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1766,7 +1766,7 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
 				&eth_da);
-		if (retval)
+		if (retval < 0)
 			return retval;
 	} else
 		memset(&eth_da, 0, sizeof(eth_da));
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index ae82e1e5d8..263ba471f1 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2734,7 +2734,7 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	/* Parse representor information first from class argument. */
 	if (dev->devargs->cls_str)
 		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
-	if (ret != 0) {
+	if (ret < 0) {
 		DRV_LOG(ERR, "failed to parse device arguments: %s",
 			dev->devargs->cls_str);
 		return -rte_errno;
@@ -2742,7 +2742,7 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev->devargs->args) {
 		/* Parse legacy device argument */
 		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
-		if (ret) {
+		if (ret < 0) {
 			DRV_LOG(ERR, "failed to parse device arguments: %s",
 				dev->devargs->args);
 			return -rte_errno;
diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 7d8c055b80..8fc36eb426 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -780,7 +780,7 @@ nfp_flower_repr_create(struct nfp_app_fw_flower *app_fw_flower)
 	/* Now parse PCI device args passed for representor info */
 	if (pci_dev->device.devargs != NULL) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-		if (ret != 0) {
+		if (ret < 0) {
 			PMD_INIT_LOG(ERR, "devarg parse failed");
 			return -EINVAL;
 		}
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6d57b2ba26..6daf542160 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3306,7 +3306,7 @@ sfc_parse_rte_devargs(const char *args, struct rte_eth_devargs *devargs)
 
 	if (args != NULL) {
 		rc = rte_eth_devargs_parse(args, &eth_da);
-		if (rc != 0) {
+		if (rc < 0) {
 			SFC_GENERIC_LOG(ERR,
 					"Failed to parse generic devargs '%s'",
 					args);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..62a06b75a2 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -470,15 +470,14 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 }
 
 int
-rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
+rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs)
 {
-	struct rte_kvargs args;
+	struct rte_eth_devargs *eth_da;
 	struct rte_kvargs_pair *pair;
-	unsigned int i;
+	struct rte_kvargs args;
+	unsigned int i, j = 0;
 	int result = 0;
 
-	memset(eth_da, 0, sizeof(*eth_da));
-
 	result = eth_dev_devargs_tokenise(&args, dargs);
 	if (result < 0)
 		goto parse_cleanup;
@@ -486,18 +485,16 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
-					dargs);
-				result = -1;
-				goto parse_cleanup;
-			}
+			eth_da = &eth_devargs[j];
+			memset(eth_da, 0, sizeof(*eth_da));
 			result = rte_eth_devargs_parse_representor_ports(
 					pair->value, eth_da);
 			if (result < 0)
 				goto parse_cleanup;
+			j++;
 		}
 	}
+	result = (int)j;
 
 parse_cleanup:
 	free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index b482cd12bb..6f4f1a1606 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1800,10 +1800,10 @@ rte_eth_representor_id_get(uint16_t port_id,
  * @param devargs
  *  device arguments
  * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.
  *
  * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
  */
 __rte_internal
 int
-- 
2.18.0

[PATCH 2/2] doc: multiple representors in one device

[Harman Kalra]

Updating the documentation with the pattern format for enabling
multiple representors in one device

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst         | 4 +++-
 doc/guides/prog_guide/switch_representation.rst | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..76285e71e2 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -376,7 +376,7 @@ parameters to those ports.
 
 * ``representor`` for a device which supports the creation of representor ports
   this argument allows user to specify which switch ports to enable port
-  representors for. Multiple representors in one device argument is invalid::
+  representors for::
 
    -a DBDF,representor=vf0
    -a DBDF,representor=vf[0,4,6,9]
@@ -389,6 +389,8 @@ parameters to those ports.
    -a DBDF,representor=pf1vf0
    -a DBDF,representor=pf[0-1]sf[0-127]
    -a DBDF,representor=pf1
+   -a DBDF,representor=pf[0-1],representor=pf2vf[0-2],representor=pf3[3,5-8]
+   (Enabling creation of multiple representors in one device argument)
 
 Note: PMDs are not required to support the standard device arguments and users
 should consult the relevant PMD documentation to see support devargs.
diff --git a/doc/guides/prog_guide/switch_representation.rst b/doc/guides/prog_guide/switch_representation.rst
index 6fd7b98bdc..fde8c790ba 100644
--- a/doc/guides/prog_guide/switch_representation.rst
+++ b/doc/guides/prog_guide/switch_representation.rst
@@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for applications.
    -a pci:dbdf,representor=sf1
    -a pci:dbdf,representor=sf[0-1023]
    -a pci:dbdf,representor=sf[0,2-1023]
+   -a pci:dbdf,representor=pf[0-1],representor=pf2vf[0-2],representor=pf3[3,5]
 
 - As virtual devices, they may be more limited than their physical
   counterparts, for instance by exposing only a subset of device
-- 
2.18.0

Re: [PATCH 1/2] ethdev: parsing multiple representor devargs string

[Andrew Rybchenko]

On 1/11/24 09:44, Harman Kalra wrote:
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>

[snip]

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index bd917a15fc..62a06b75a2 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -470,15 +470,14 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   }
>   
>   int
> -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs)
>   {
> -	struct rte_kvargs args;
> +	struct rte_eth_devargs *eth_da;
>   	struct rte_kvargs_pair *pair;
> -	unsigned int i;
> +	struct rte_kvargs args;
> +	unsigned int i, j = 0;

Please, avoid multiple variable in one line declaration with
initialization. It is too error prone. Also j is a bad name
here. It is not a loop counter or seomthing like this.
Maybe 'parsed' or 'devargs_parsed'?

>   	int result = 0;
>   
> -	memset(eth_da, 0, sizeof(*eth_da));
> -
>   	result = eth_dev_devargs_tokenise(&args, dargs);
>   	if (result < 0)
>   		goto parse_cleanup;
> @@ -486,18 +485,16 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
>   	for (i = 0; i < args.count; i++) {
>   		pair = &args.pairs[i];
>   		if (strcmp("representor", pair->key) == 0) {
> -			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
> -				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
> -					dargs);
> -				result = -1;
> -				goto parse_cleanup;
> -			}
> +			eth_da = &eth_devargs[j];
> +			memset(eth_da, 0, sizeof(*eth_da));
>   			result = rte_eth_devargs_parse_representor_ports(
>   					pair->value, eth_da);
>   			if (result < 0)
>   				goto parse_cleanup;
> +			j++;
>   		}
>   	}
> +	result = (int)j;
>   
>   parse_cleanup:
>   	free(args.str);
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index b482cd12bb..6f4f1a1606 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1800,10 +1800,10 @@ rte_eth_representor_id_get(uint16_t port_id,
>    * @param devargs
>    *  device arguments
>    * @param eth_devargs
> - *  parsed ethdev specific arguments.
> + *  contiguous memory populated with parsed ethdev specific arguments.

Caller must provide information about array size. Right now you simply
introduce out-of-bound array access.
All drivers just provide one entry and if I pass many-many devargs I
can write far beyond that location and corrupt stack.

>    *
>    * @return
> - *   Negative errno value on error, 0 on success.
> + *   Negative errno value on error, no of devargs parsed on success.
>    */
>   __rte_internal
>   int

Re: [PATCH 2/2] doc: multiple representors in one device

[Andrew Rybchenko]

On 1/11/24 09:44, Harman Kalra wrote:
> Updating the documentation with the pattern format for enabling
> multiple representors in one device
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>

IMHO it should be squashed into previous patch.

RE: [EXT] Re: [PATCH 1/2] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi Andrew

Thanks for review
Please find response inline

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, January 12, 2024 12:56 PM
> To: Harman Kalra <hkalra@marvell.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>;
> Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@amd.com>
> Cc: dev@dpdk.org
> Subject: [EXT] Re: [PATCH 1/2] ethdev: parsing multiple representor devargs
> string
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 1/11/24 09:44, Harman Kalra wrote:
> > Adding support for parsing multiple representor devargs strings passed
> > to a PCI BDF. There may be scenario where port representors for
> > various PFs or VFs under PFs are required and all these are
> > representor ports shall be backed by single pci device. In such case
> > port representors can be created using devargs string:
> > <PCI
> > BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]
> >
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> 
> [snip]
> 
> > diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> > index bd917a15fc..62a06b75a2 100644
> > --- a/lib/ethdev/ethdev_driver.c
> > +++ b/lib/ethdev/ethdev_driver.c
> > @@ -470,15 +470,14 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
> >   }
> >
> >   int
> > -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs
> > *eth_da)
> > +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs
> > +*eth_devargs)
> >   {
> > -	struct rte_kvargs args;
> > +	struct rte_eth_devargs *eth_da;
> >   	struct rte_kvargs_pair *pair;
> > -	unsigned int i;
> > +	struct rte_kvargs args;
> > +	unsigned int i, j = 0;
> 
> Please, avoid multiple variable in one line declaration with initialization. It is
> too error prone. Also j is a bad name here. It is not a loop counter or
> seomthing like this.
> Maybe 'parsed' or 'devargs_parsed'?

Sure, will replace 'j' with a better name.

> 
> >   	int result = 0;
> >
> > -	memset(eth_da, 0, sizeof(*eth_da));
> > -
> >   	result = eth_dev_devargs_tokenise(&args, dargs);
> >   	if (result < 0)
> >   		goto parse_cleanup;
> > @@ -486,18 +485,16 @@ rte_eth_devargs_parse(const char *dargs, struct
> rte_eth_devargs *eth_da)
> >   	for (i = 0; i < args.count; i++) {
> >   		pair = &args.pairs[i];
> >   		if (strcmp("representor", pair->key) == 0) {
> > -			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
> > -				RTE_ETHDEV_LOG_LINE(ERR, "duplicated
> representor key: %s",
> > -					dargs);
> > -				result = -1;
> > -				goto parse_cleanup;
> > -			}
> > +			eth_da = &eth_devargs[j];
> > +			memset(eth_da, 0, sizeof(*eth_da));
> >   			result = rte_eth_devargs_parse_representor_ports(
> >   					pair->value, eth_da);
> >   			if (result < 0)
> >   				goto parse_cleanup;
> > +			j++;
> >   		}
> >   	}
> > +	result = (int)j;
> >
> >   parse_cleanup:
> >   	free(args.str);
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index b482cd12bb..6f4f1a1606 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1800,10 +1800,10 @@ rte_eth_representor_id_get(uint16_t port_id,
> >    * @param devargs
> >    *  device arguments
> >    * @param eth_devargs
> > - *  parsed ethdev specific arguments.
> > + *  contiguous memory populated with parsed ethdev specific arguments.
> 
> Caller must provide information about array size. Right now you simply
> introduce out-of-bound array access.
> All drivers just provide one entry and if I pass many-many devargs I can write
> far beyond that location and corrupt stack.

We thought of adding an additional third argument to the API:
int rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs, uint8_t nb_da);

Later skipped just with an intention to have minimal changes to existing driver calls.
But yes, stack corruption case may happen without passing array size.

Will update the implementation as per above protype with 3 args.

@All, any thoughts on this??

Thanks
Harman


> 
> >    *
> >    * @return
> > - *   Negative errno value on error, 0 on success.
> > + *   Negative errno value on error, no of devargs parsed on success.
> >    */
> >   __rte_internal
> >   int

Re: [PATCH 1/2] ethdev: parsing multiple representor devargs string

[David Marchand]

On Thu, Jan 11, 2024 at 7:45 AM Harman Kalra <hkalra@marvell.com> wrote:
>
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]

Is it possible to make the representor devargs value a list?
Like: <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]


-- 
David Marchand

[PATCH v2 0/1] multiple representors in one device

[Harman Kalra]

Following series adds support to enable creation of multiple representors
under one base device. There may be scenarios where port representors for
multiple PFs or VFs under PF are required and all these representor ports
created under a single pci device. Marvell CNXK port representor solution
is designed around this scenario where all representors are backed by a
single switch device.

Earlier this change was implemented as part of the Marvell CNXK port
representor series but after suggestions from Thomas we would like
to propose these changes in common code.
https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785

V2:
- Updated the multiple representor devarg pattern to list
i.e. representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
- Introduced size of array as third argument to rte_eth_devargs_parse()
to avoid array corruption
- Squashed separate document patch 

Harman Kalra (1):
  ethdev: parsing multiple representor devargs string

 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 105 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 119 insertions(+), 36 deletions(-)

-- 
2.18.0

[PATCH v2 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Adding support for parsing multiple representor devargs strings
passed to a PCI BDF. There may be scenario where port representors
for various PFs or VFs under PFs are required and all these are
representor ports shall be backed by single pci device. In such
case port representors can be created using devargs string:
<PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 105 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 119 insertions(+), 36 deletions(-)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..5008b41c60 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -376,7 +376,7 @@ parameters to those ports.
 
 * ``representor`` for a device which supports the creation of representor ports
   this argument allows user to specify which switch ports to enable port
-  representors for. Multiple representors in one device argument is invalid::
+  representors for::
 
    -a DBDF,representor=vf0
    -a DBDF,representor=vf[0,4,6,9]
@@ -389,6 +389,8 @@ parameters to those ports.
    -a DBDF,representor=pf1vf0
    -a DBDF,representor=pf[0-1]sf[0-127]
    -a DBDF,representor=pf1
+   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
+   (Multiple representors in one device argument can be represented as a list)
 
 Note: PMDs are not required to support the standard device arguments and users
 should consult the relevant PMD documentation to see support devargs.
diff --git a/doc/guides/prog_guide/switch_representation.rst b/doc/guides/prog_guide/switch_representation.rst
index 6fd7b98bdc..46e0ca85a5 100644
--- a/doc/guides/prog_guide/switch_representation.rst
+++ b/doc/guides/prog_guide/switch_representation.rst
@@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for applications.
    -a pci:dbdf,representor=sf1
    -a pci:dbdf,representor=sf[0-1023]
    -a pci:dbdf,representor=sf[0,2-1023]
+   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
 
 - As virtual devices, they may be more limited than their physical
   counterparts, for instance by exposing only a subset of device
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index acf7e6e46e..5d4f599044 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-					    &eth_da);
-		if (ret)
+					    &eth_da, 1);
+		if (ret < 0)
 			return ret;
 	}
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index b04b6c9aa1..33d96ec07a 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1317,8 +1317,8 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	ENICPMD_FUNC_TRACE();
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 	if (eth_da.nb_representor_ports > 0 &&
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..4d21341382 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -646,8 +646,8 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 5d845bba31..0e991fe4b8 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -2041,8 +2041,8 @@ eth_ice_dcf_pci_probe(__rte_unused struct rte_pci_driver *pci_drv,
 	if (!ice_devargs_check(pci_dev->device.devargs, ICE_DCF_DEVARG_CAP))
 		return 1;
 
-	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-	if (ret)
+	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+	if (ret < 0)
 		return ret;
 
 	ret = rte_eth_dev_pci_generic_probe(pci_dev,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d6cf00317e..98e9b8a031 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1765,8 +1765,8 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	} else
 		memset(&eth_da, 0, sizeof(eth_da));
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index ae82e1e5d8..17061126d5 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2733,16 +2733,16 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	memset(eth_da, 0, sizeof(*eth_da));
 	/* Parse representor information first from class argument. */
 	if (dev->devargs->cls_str)
-		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
-	if (ret != 0) {
+		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da, 1);
+	if (ret < 0) {
 		DRV_LOG(ERR, "failed to parse device arguments: %s",
 			dev->devargs->cls_str);
 		return -rte_errno;
 	}
 	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev->devargs->args) {
 		/* Parse legacy device argument */
-		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
-		if (ret) {
+		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da, 1);
+		if (ret < 0) {
 			DRV_LOG(ERR, "failed to parse device arguments: %s",
 				dev->devargs->args);
 			return -rte_errno;
diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 5f7c1fa737..63fe37c8d7 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower *app_fw_flower)
 
 	/* Now parse PCI device args passed for representor info */
 	if (pci_dev->device.devargs != NULL) {
-		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-		if (ret != 0) {
+		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+		if (ret < 0) {
 			PMD_INIT_LOG(ERR, "devarg parse failed");
 			return -EINVAL;
 		}
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6d57b2ba26..0bbcb80365 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3305,8 +3305,8 @@ sfc_parse_rte_devargs(const char *args, struct rte_eth_devargs *devargs)
 	int rc;
 
 	if (args != NULL) {
-		rc = rte_eth_devargs_parse(args, &eth_da);
-		if (rc != 0) {
+		rc = rte_eth_devargs_parse(args, &eth_da, 1);
+		if (rc < 0) {
 			SFC_GENERIC_LOG(ERR,
 					"Failed to parse generic devargs '%s'",
 					args);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..208252f6c5 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2022 Intel Corporation
  */
 
+#include <ctype.h>
 #include <stdlib.h>
 #include <pthread.h>
 
@@ -459,9 +460,23 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 			break;
 
 		case 3: /* Parsing list */
-			if (*letter == ']')
-				state = 2;
-			else if (*letter == '\0')
+			if (*letter == ']') {
+				/* Multiple representor case has ']' dual meaning, first end of
+				 * individual pfvf list and other end of consolidated list of
+				 * representors.
+				 * Complete multiple representors list to be considered as one
+				 * pair value.
+				 */
+				if ((strcmp("representor", pair->key) == 0) &&
+				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 's' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
+				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
+					state = 3;
+				else
+					state = 2;
+			} else if (*letter == '\0')
 				return -EINVAL;
 			break;
 		}
@@ -469,16 +484,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
+static int
+eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
+				  uint8_t nb_da)
+{
+	struct rte_eth_devargs *eth_da;
+	char da_val[BUFSIZ];
+	char delim[] = "]";
+	int devargs = 0;
+	int result = 0;
+	char *token;
+
+	token = strtok(&p_val[1], delim);
+	while (token != NULL) {
+		eth_da = &eth_devargs[devargs];
+		memset(eth_da, 0, sizeof(*eth_da));
+		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token : token, ']');
+		/* Parse the tokenised devarg value */
+		result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
+		if (result < 0)
+			goto parse_cleanup;
+		devargs++;
+		if (devargs > nb_da) {
+			RTE_ETHDEV_LOG_LINE(ERR,
+					    "Devargs parsed %d > max array size %d",
+					    devargs, nb_da);
+			result = -1;
+			goto parse_cleanup;
+		}
+		token = strtok(NULL, delim);
+	}
+
+	result = devargs;
+
+parse_cleanup:
+	return result;
+
+}
+
 int
-rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
+rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
+		      uint8_t nb_da)
 {
-	struct rte_kvargs args;
+	struct rte_eth_devargs *eth_da;
 	struct rte_kvargs_pair *pair;
+	struct rte_kvargs args;
+	static bool dup_rep;
+	int devargs = 0;
 	unsigned int i;
 	int result = 0;
 
-	memset(eth_da, 0, sizeof(*eth_da));
-
 	result = eth_dev_devargs_tokenise(&args, dargs);
 	if (result < 0)
 		goto parse_cleanup;
@@ -486,18 +541,40 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
-					dargs);
+			if (dup_rep) {
+				RTE_ETHDEV_LOG_LINE(ERR, "Duplicated representor key: %s",
+						    pair->value);
 				result = -1;
 				goto parse_cleanup;
 			}
-			result = rte_eth_devargs_parse_representor_ports(
-					pair->value, eth_da);
-			if (result < 0)
-				goto parse_cleanup;
+
+			if (pair->value[0] == '[' && !isdigit(pair->value[1])) {
+				/* Multiple representor list case */
+				devargs = eth_dev_tokenise_representor_list(pair->value,
+									    eth_devargs, nb_da);
+				if (devargs < 0)
+					goto parse_cleanup;
+			} else {
+				/* Single representor case */
+				eth_da = &eth_devargs[devargs];
+				memset(eth_da, 0, sizeof(*eth_da));
+				result =
+					rte_eth_devargs_parse_representor_ports(pair->value,
+										eth_da);
+				if (result < 0)
+					goto parse_cleanup;
+				devargs++;
+				if (devargs > nb_da) {
+					RTE_ETHDEV_LOG_LINE(ERR, "Devargs parsed %d > max array size %d",
+							    devargs, nb_da);
+					result = -1;
+					goto parse_cleanup;
+				}
+			}
+			dup_rep = true;
 		}
 	}
+	result = devargs;
 
 parse_cleanup:
 	free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index b482cd12bb..e4b99448dd 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1800,14 +1800,17 @@ rte_eth_representor_id_get(uint16_t port_id,
  * @param devargs
  *  device arguments
  * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.
+ * @param nb_da
+ *  size of eth_devargs array passed
  *
  * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
  */
 __rte_internal
 int
-rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs);
+rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs,
+		      uint8_t nb_da);
 
 
 typedef int (*ethdev_init_t)(struct rte_eth_dev *ethdev, void *init_params);
-- 
2.18.0

RE: [EXT] Re: [PATCH 1/2] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi David

Thanks for review
Please find responses inline

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, January 12, 2024 6:12 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>;
> dev@dpdk.org
> Subject: [EXT] Re: [PATCH 1/2] ethdev: parsing multiple representor devargs
> string
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Jan 11, 2024 at 7:45 AM Harman Kalra <hkalra@marvell.com>
> wrote:
> >
> > Adding support for parsing multiple representor devargs strings passed
> > to a PCI BDF. There may be scenario where port representors for
> > various PFs or VFs under PFs are required and all these are
> > representor ports shall be backed by single pci device. In such case
> > port representors can be created using devargs string:
> > <PCI
> > BDF>,representor=pf[0-1],representor=pf2vf[1,2-3],representor=[4-5]
> 
> Is it possible to make the representor devargs value a list?
> Like: <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]

Thanks for suggestion, even I was not so happy with repeated representor keywork.
Please review the changes:
https://patches.dpdk.org/project/dpdk/patch/20240115155715.111154-2-hkalra@marvell.com/

Thanks
Harman


> 
> 
> --
> David Marchand

RE: [EXT] Re: [PATCH 2/2] doc: multiple representors in one device

[Harman Kalra]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, January 12, 2024 12:56 PM
> To: Harman Kalra <hkalra@marvell.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: dev@dpdk.org
> Subject: [EXT] Re: [PATCH 2/2] doc: multiple representors in one device
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 1/11/24 09:44, Harman Kalra wrote:
> > Updating the documentation with the pattern format for enabling
> > multiple representors in one device
> >
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> 
> IMHO it should be squashed into previous patch.

Done, kindly review V2

[PATCH v3 0/1] multiple representors in one device

[Harman Kalra]

Following series adds support to enable creation of multiple representors
under one base device. There may be scenarios where port representors for
multiple PFs or VFs under PF are required and all these representor ports
created under a single pci device. Marvell CNXK port representor solution
is designed around this scenario where all representors are backed by a
single switch device.

Earlier this change was implemented as part of the Marvell CNXK port
representor series but after suggestions from Thomas we would like
to propose these changes in common code.
https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785

V3:
- Fix duplicate representor devarg key handling logic

V2:
- Updated the multiple representor devarg pattern to list
i.e. representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
- Introduced size of array as third argument to rte_eth_devargs_parse()
to avoid array corruption
- Squashed separate document patch 

Harman Kalra (1):
  ethdev: parsing multiple representor devargs string

 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 105 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 119 insertions(+), 36 deletions(-)

-- 
2.18.0

[PATCH v3 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Adding support for parsing multiple representor devargs strings
passed to a PCI BDF. There may be scenario where port representors
for various PFs or VFs under PFs are required and all these are
representor ports shall be backed by single pci device. In such
case port representors can be created using devargs string:
<PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 105 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 119 insertions(+), 36 deletions(-)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..5008b41c60 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -376,7 +376,7 @@ parameters to those ports.
 
 * ``representor`` for a device which supports the creation of representor ports
   this argument allows user to specify which switch ports to enable port
-  representors for. Multiple representors in one device argument is invalid::
+  representors for::
 
    -a DBDF,representor=vf0
    -a DBDF,representor=vf[0,4,6,9]
@@ -389,6 +389,8 @@ parameters to those ports.
    -a DBDF,representor=pf1vf0
    -a DBDF,representor=pf[0-1]sf[0-127]
    -a DBDF,representor=pf1
+   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
+   (Multiple representors in one device argument can be represented as a list)
 
 Note: PMDs are not required to support the standard device arguments and users
 should consult the relevant PMD documentation to see support devargs.
diff --git a/doc/guides/prog_guide/switch_representation.rst b/doc/guides/prog_guide/switch_representation.rst
index 6fd7b98bdc..46e0ca85a5 100644
--- a/doc/guides/prog_guide/switch_representation.rst
+++ b/doc/guides/prog_guide/switch_representation.rst
@@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for applications.
    -a pci:dbdf,representor=sf1
    -a pci:dbdf,representor=sf[0-1023]
    -a pci:dbdf,representor=sf[0,2-1023]
+   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
 
 - As virtual devices, they may be more limited than their physical
   counterparts, for instance by exposing only a subset of device
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index acf7e6e46e..5d4f599044 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-					    &eth_da);
-		if (ret)
+					    &eth_da, 1);
+		if (ret < 0)
 			return ret;
 	}
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index b04b6c9aa1..33d96ec07a 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1317,8 +1317,8 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	ENICPMD_FUNC_TRACE();
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 	if (eth_da.nb_representor_ports > 0 &&
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..4d21341382 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -646,8 +646,8 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 5d845bba31..0e991fe4b8 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -2041,8 +2041,8 @@ eth_ice_dcf_pci_probe(__rte_unused struct rte_pci_driver *pci_drv,
 	if (!ice_devargs_check(pci_dev->device.devargs, ICE_DCF_DEVARG_CAP))
 		return 1;
 
-	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-	if (ret)
+	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+	if (ret < 0)
 		return ret;
 
 	ret = rte_eth_dev_pci_generic_probe(pci_dev,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d6cf00317e..98e9b8a031 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1765,8 +1765,8 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	} else
 		memset(&eth_da, 0, sizeof(eth_da));
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index ae82e1e5d8..17061126d5 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2733,16 +2733,16 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	memset(eth_da, 0, sizeof(*eth_da));
 	/* Parse representor information first from class argument. */
 	if (dev->devargs->cls_str)
-		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
-	if (ret != 0) {
+		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da, 1);
+	if (ret < 0) {
 		DRV_LOG(ERR, "failed to parse device arguments: %s",
 			dev->devargs->cls_str);
 		return -rte_errno;
 	}
 	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev->devargs->args) {
 		/* Parse legacy device argument */
-		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
-		if (ret) {
+		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da, 1);
+		if (ret < 0) {
 			DRV_LOG(ERR, "failed to parse device arguments: %s",
 				dev->devargs->args);
 			return -rte_errno;
diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 5f7c1fa737..63fe37c8d7 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower *app_fw_flower)
 
 	/* Now parse PCI device args passed for representor info */
 	if (pci_dev->device.devargs != NULL) {
-		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-		if (ret != 0) {
+		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+		if (ret < 0) {
 			PMD_INIT_LOG(ERR, "devarg parse failed");
 			return -EINVAL;
 		}
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6d57b2ba26..0bbcb80365 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3305,8 +3305,8 @@ sfc_parse_rte_devargs(const char *args, struct rte_eth_devargs *devargs)
 	int rc;
 
 	if (args != NULL) {
-		rc = rte_eth_devargs_parse(args, &eth_da);
-		if (rc != 0) {
+		rc = rte_eth_devargs_parse(args, &eth_da, 1);
+		if (rc < 0) {
 			SFC_GENERIC_LOG(ERR,
 					"Failed to parse generic devargs '%s'",
 					args);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..8a49511516 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2022 Intel Corporation
  */
 
+#include <ctype.h>
 #include <stdlib.h>
 #include <pthread.h>
 
@@ -459,9 +460,23 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 			break;
 
 		case 3: /* Parsing list */
-			if (*letter == ']')
-				state = 2;
-			else if (*letter == '\0')
+			if (*letter == ']') {
+				/* Multiple representor case has ']' dual meaning, first end of
+				 * individual pfvf list and other end of consolidated list of
+				 * representors.
+				 * Complete multiple representors list to be considered as one
+				 * pair value.
+				 */
+				if ((strcmp("representor", pair->key) == 0) &&
+				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 's' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
+				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
+					state = 3;
+				else
+					state = 2;
+			} else if (*letter == '\0')
 				return -EINVAL;
 			break;
 		}
@@ -469,16 +484,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
+static int
+eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
+				  uint8_t nb_da)
+{
+	struct rte_eth_devargs *eth_da;
+	char da_val[BUFSIZ];
+	char delim[] = "]";
+	int devargs = 0;
+	int result = 0;
+	char *token;
+
+	token = strtok(&p_val[1], delim);
+	while (token != NULL) {
+		eth_da = &eth_devargs[devargs];
+		memset(eth_da, 0, sizeof(*eth_da));
+		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token : token, ']');
+		/* Parse the tokenised devarg value */
+		result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
+		if (result < 0)
+			goto parse_cleanup;
+		devargs++;
+		if (devargs > nb_da) {
+			RTE_ETHDEV_LOG_LINE(ERR,
+					    "Devargs parsed %d > max array size %d",
+					    devargs, nb_da);
+			result = -1;
+			goto parse_cleanup;
+		}
+		token = strtok(NULL, delim);
+	}
+
+	result = devargs;
+
+parse_cleanup:
+	return result;
+
+}
+
 int
-rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
+rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
+		      uint8_t nb_da)
 {
-	struct rte_kvargs args;
+	struct rte_eth_devargs *eth_da;
 	struct rte_kvargs_pair *pair;
+	struct rte_kvargs args;
+	bool dup_rep = false;
+	int devargs = 0;
 	unsigned int i;
 	int result = 0;
 
-	memset(eth_da, 0, sizeof(*eth_da));
-
 	result = eth_dev_devargs_tokenise(&args, dargs);
 	if (result < 0)
 		goto parse_cleanup;
@@ -486,18 +541,40 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
-					dargs);
+			if (dup_rep) {
+				RTE_ETHDEV_LOG_LINE(ERR, "Duplicated representor key: %s",
+						    pair->value);
 				result = -1;
 				goto parse_cleanup;
 			}
-			result = rte_eth_devargs_parse_representor_ports(
-					pair->value, eth_da);
-			if (result < 0)
-				goto parse_cleanup;
+
+			if (pair->value[0] == '[' && !isdigit(pair->value[1])) {
+				/* Multiple representor list case */
+				devargs = eth_dev_tokenise_representor_list(pair->value,
+									    eth_devargs, nb_da);
+				if (devargs < 0)
+					goto parse_cleanup;
+			} else {
+				/* Single representor case */
+				eth_da = &eth_devargs[devargs];
+				memset(eth_da, 0, sizeof(*eth_da));
+				result =
+					rte_eth_devargs_parse_representor_ports(pair->value,
+										eth_da);
+				if (result < 0)
+					goto parse_cleanup;
+				devargs++;
+				if (devargs > nb_da) {
+					RTE_ETHDEV_LOG_LINE(ERR, "Devargs parsed %d > max array size %d",
+							    devargs, nb_da);
+					result = -1;
+					goto parse_cleanup;
+				}
+			}
+			dup_rep = true;
 		}
 	}
+	result = devargs;
 
 parse_cleanup:
 	free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index b482cd12bb..e4b99448dd 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1800,14 +1800,17 @@ rte_eth_representor_id_get(uint16_t port_id,
  * @param devargs
  *  device arguments
  * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.
+ * @param nb_da
+ *  size of eth_devargs array passed
  *
  * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
  */
 __rte_internal
 int
-rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs);
+rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs,
+		      uint8_t nb_da);
 
 
 typedef int (*ethdev_init_t)(struct rte_eth_dev *ethdev, void *init_params);
-- 
2.18.0

Re: [PATCH v3 1/1] ethdev: parsing multiple representor devargs string

[Andrew Rybchenko]

On 1/16/24 12:55, Harman Kalra wrote:
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>

see below

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index bd917a15fc..8a49511516 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -2,6 +2,7 @@
>    * Copyright(c) 2022 Intel Corporation
>    */
>   
> +#include <ctype.h>
>   #include <stdlib.h>
>   #include <pthread.h>
>   
> @@ -459,9 +460,23 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   			break;
>   
>   		case 3: /* Parsing list */
> -			if (*letter == ']')
> -				state = 2;
> -			else if (*letter == '\0')
> +			if (*letter == ']') {
> +				/* Multiple representor case has ']' dual meaning, first end of
> +				 * individual pfvf list and other end of consolidated list of
> +				 * representors.
> +				 * Complete multiple representors list to be considered as one
> +				 * pair value.
> +				 */
> +				if ((strcmp("representor", pair->key) == 0) &&
> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')   ||

Sorry, but it is unclear why it is not out-of-bound access.

> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')   ||
> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')   ||

may be it is better to use strncmp() instead? IMHO it is a bit hard to
follow.

> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
> +				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
> +					state = 3;
> +				else
> +					state = 2;
> +			} else if (*letter == '\0')
>   				return -EINVAL;
>   			break;
>   		}
> @@ -469,16 +484,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   	}
>   }
>   
> +static int
> +eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
> +				  uint8_t nb_da)
> +{
> +	struct rte_eth_devargs *eth_da;
> +	char da_val[BUFSIZ];
> +	char delim[] = "]";
> +	int devargs = 0;
> +	int result = 0;
> +	char *token;
> +
> +	token = strtok(&p_val[1], delim);

since strtok() is MT-unsafe, I'd recommend to use strtok_r()

> +	while (token != NULL) {
> +		eth_da = &eth_devargs[devargs];
> +		memset(eth_da, 0, sizeof(*eth_da));
> +		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token : token, ']');
> +		/* Parse the tokenised devarg value */
> +		result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
> +		if (result < 0)
> +			goto parse_cleanup;
> +		devargs++;
> +		if (devargs > nb_da) {
> +			RTE_ETHDEV_LOG_LINE(ERR,
> +					    "Devargs parsed %d > max array size %d",
> +					    devargs, nb_da);
> +			result = -1;
> +			goto parse_cleanup;
> +		}
> +		token = strtok(NULL, delim);
> +	}
> +
> +	result = devargs;
> +
> +parse_cleanup:
> +	return result;
> +
> +}
> +
>   int
> -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
> +		      uint8_t nb_da)

I see no single reason to limit nb_da to uint8_t type. IMHO it should be
'unsigned int' as an unsigned number of default type.
'unsigned int' is used for number of stats and ptypes in array.

[snip]

[PATCH v4 0/1] multiple representors in one device

[Harman Kalra]

Following series adds support to enable creation of multiple representors
under one base device. There may be scenarios where port representors for
multiple PFs or VFs under PF are required and all these representor ports
created under a single pci device. Marvell CNXK port representor solution
is designed around this scenario where all representors are backed by a
single switch device.

Earlier this change was implemented as part of the Marvell CNXK port
representor series but after suggestions from Thomas we would like
to propose these changes in common code.
https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785

V4:
- Used MT safe strtok_r in place of strtok
- Reworded some comments

V3:
- Fix duplicate representor devarg key handling logic

V2:
- Updated the multiple representor devarg pattern to list
i.e. representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
- Introduced size of array as third argument to rte_eth_devargs_parse()
to avoid array corruption
- Squashed separate document patch 

Harman Kalra (1):
  ethdev: parsing multiple representor devargs string

 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 108 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 122 insertions(+), 36 deletions(-)

-- 
2.18.0

[PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Adding support for parsing multiple representor devargs strings
passed to a PCI BDF. There may be scenario where port representors
for various PFs or VFs under PFs are required and all these are
representor ports shall be backed by single pci device. In such
case port representors can be created using devargs string:
<PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 108 +++++++++++++++---
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 122 insertions(+), 36 deletions(-)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..5008b41c60 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -376,7 +376,7 @@ parameters to those ports.
 
 * ``representor`` for a device which supports the creation of representor ports
   this argument allows user to specify which switch ports to enable port
-  representors for. Multiple representors in one device argument is invalid::
+  representors for::
 
    -a DBDF,representor=vf0
    -a DBDF,representor=vf[0,4,6,9]
@@ -389,6 +389,8 @@ parameters to those ports.
    -a DBDF,representor=pf1vf0
    -a DBDF,representor=pf[0-1]sf[0-127]
    -a DBDF,representor=pf1
+   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
+   (Multiple representors in one device argument can be represented as a list)
 
 Note: PMDs are not required to support the standard device arguments and users
 should consult the relevant PMD documentation to see support devargs.
diff --git a/doc/guides/prog_guide/switch_representation.rst b/doc/guides/prog_guide/switch_representation.rst
index 6fd7b98bdc..46e0ca85a5 100644
--- a/doc/guides/prog_guide/switch_representation.rst
+++ b/doc/guides/prog_guide/switch_representation.rst
@@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for applications.
    -a pci:dbdf,representor=sf1
    -a pci:dbdf,representor=sf[0-1023]
    -a pci:dbdf,representor=sf[0,2-1023]
+   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
 
 - As virtual devices, they may be more limited than their physical
   counterparts, for instance by exposing only a subset of device
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index acf7e6e46e..5d4f599044 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-					    &eth_da);
-		if (ret)
+					    &eth_da, 1);
+		if (ret < 0)
 			return ret;
 	}
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index b04b6c9aa1..33d96ec07a 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1317,8 +1317,8 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	ENICPMD_FUNC_TRACE();
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 	if (eth_da.nb_representor_ports > 0 &&
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..4d21341382 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -646,8 +646,8 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 5d845bba31..0e991fe4b8 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -2041,8 +2041,8 @@ eth_ice_dcf_pci_probe(__rte_unused struct rte_pci_driver *pci_drv,
 	if (!ice_devargs_check(pci_dev->device.devargs, ICE_DCF_DEVARG_CAP))
 		return 1;
 
-	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-	if (ret)
+	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+	if (ret < 0)
 		return ret;
 
 	ret = rte_eth_dev_pci_generic_probe(pci_dev,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d6cf00317e..98e9b8a031 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1765,8 +1765,8 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	} else
 		memset(&eth_da, 0, sizeof(eth_da));
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index ae82e1e5d8..17061126d5 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2733,16 +2733,16 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	memset(eth_da, 0, sizeof(*eth_da));
 	/* Parse representor information first from class argument. */
 	if (dev->devargs->cls_str)
-		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
-	if (ret != 0) {
+		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da, 1);
+	if (ret < 0) {
 		DRV_LOG(ERR, "failed to parse device arguments: %s",
 			dev->devargs->cls_str);
 		return -rte_errno;
 	}
 	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev->devargs->args) {
 		/* Parse legacy device argument */
-		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
-		if (ret) {
+		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da, 1);
+		if (ret < 0) {
 			DRV_LOG(ERR, "failed to parse device arguments: %s",
 				dev->devargs->args);
 			return -rte_errno;
diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 5f7c1fa737..63fe37c8d7 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower *app_fw_flower)
 
 	/* Now parse PCI device args passed for representor info */
 	if (pci_dev->device.devargs != NULL) {
-		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-		if (ret != 0) {
+		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+		if (ret < 0) {
 			PMD_INIT_LOG(ERR, "devarg parse failed");
 			return -EINVAL;
 		}
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6d57b2ba26..0bbcb80365 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3305,8 +3305,8 @@ sfc_parse_rte_devargs(const char *args, struct rte_eth_devargs *devargs)
 	int rc;
 
 	if (args != NULL) {
-		rc = rte_eth_devargs_parse(args, &eth_da);
-		if (rc != 0) {
+		rc = rte_eth_devargs_parse(args, &eth_da, 1);
+		if (rc < 0) {
 			SFC_GENERIC_LOG(ERR,
 					"Failed to parse generic devargs '%s'",
 					args);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..4c0d22f838 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2022 Intel Corporation
  */
 
+#include <ctype.h>
 #include <stdlib.h>
 #include <pthread.h>
 
@@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 			break;
 
 		case 3: /* Parsing list */
-			if (*letter == ']')
-				state = 2;
-			else if (*letter == '\0')
+			if (*letter == ']') {
+				/* For devargs having singles lists move to state 2 once letter
+				 * becomes ']' so each can be considered as different pair key
+				 * value. But in nested lists case e.g. multiple representors
+				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete nested list should
+				 * be considered as one pair value, hence checking if end of outer
+				 * list ']' is reached else stay on state 3.
+				 */
+				if ((strcmp("representor", pair->key) == 0)	    &&
+				    (*(letter + 1) != '\0' && *(letter + 2) != '\0' &&
+				     *(letter + 3) != '\0')			    &&
+				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f') ||
+				     (*(letter + 2) == 'v' && *(letter + 3) == 'f') ||
+				     (*(letter + 2) == 's' && *(letter + 3) == 'f') ||
+				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
+				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
+					state = 3;
+				else
+					state = 2;
+			} else if (*letter == '\0')
 				return -EINVAL;
 			break;
 		}
@@ -469,16 +487,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 	}
 }
 
+static int
+eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
+				  unsigned int nb_da)
+{
+	struct rte_eth_devargs *eth_da;
+	char da_val[BUFSIZ];
+	char *token, *sptr;
+	char delim[] = "]";
+	int devargs = 0;
+	int result = 0;
+
+	token = strtok_r(&p_val[1], delim, &sptr);
+	while (token != NULL) {
+		eth_da = &eth_devargs[devargs];
+		memset(eth_da, 0, sizeof(*eth_da));
+		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token : token, ']');
+		/* Parse the tokenised devarg value */
+		result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
+		if (result < 0)
+			goto parse_cleanup;
+		devargs++;
+		if (devargs > (int)nb_da) {
+			RTE_ETHDEV_LOG_LINE(ERR,
+					    "Devargs parsed %d > max array size %d",
+					    devargs, nb_da);
+			result = -1;
+			goto parse_cleanup;
+		}
+		token = strtok_r(NULL, delim, &sptr);
+	}
+
+	result = devargs;
+
+parse_cleanup:
+	return result;
+
+}
+
 int
-rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
+rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
+		      unsigned int nb_da)
 {
-	struct rte_kvargs args;
+	struct rte_eth_devargs *eth_da;
 	struct rte_kvargs_pair *pair;
+	struct rte_kvargs args;
+	static bool dup_rep;
+	int devargs = 0;
 	unsigned int i;
 	int result = 0;
 
-	memset(eth_da, 0, sizeof(*eth_da));
-
 	result = eth_dev_devargs_tokenise(&args, dargs);
 	if (result < 0)
 		goto parse_cleanup;
@@ -486,18 +544,40 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
-					dargs);
+			if (dup_rep) {
+				RTE_ETHDEV_LOG_LINE(ERR, "Duplicated representor key: %s",
+						    pair->value);
 				result = -1;
 				goto parse_cleanup;
 			}
-			result = rte_eth_devargs_parse_representor_ports(
-					pair->value, eth_da);
-			if (result < 0)
-				goto parse_cleanup;
+
+			if (pair->value[0] == '[' && !isdigit(pair->value[1])) {
+				/* Multiple representor list case */
+				devargs = eth_dev_tokenise_representor_list(pair->value,
+									    eth_devargs, nb_da);
+				if (devargs < 0)
+					goto parse_cleanup;
+			} else {
+				/* Single representor case */
+				eth_da = &eth_devargs[devargs];
+				memset(eth_da, 0, sizeof(*eth_da));
+				result =
+					rte_eth_devargs_parse_representor_ports(pair->value,
+										eth_da);
+				if (result < 0)
+					goto parse_cleanup;
+				devargs++;
+				if (devargs > (int)nb_da) {
+					RTE_ETHDEV_LOG_LINE(ERR, "Devargs parsed %d > max array size %d",
+							    devargs, nb_da);
+					result = -1;
+					goto parse_cleanup;
+				}
+			}
+			dup_rep = true;
 		}
 	}
+	result = devargs;
 
 parse_cleanup:
 	free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index b482cd12bb..fd6f404e9a 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1800,14 +1800,17 @@ rte_eth_representor_id_get(uint16_t port_id,
  * @param devargs
  *  device arguments
  * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.
+ * @param nb_da
+ *  size of eth_devargs array passed
  *
  * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
  */
 __rte_internal
 int
-rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs);
+rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs,
+		      unsigned int nb_da);
 
 
 typedef int (*ethdev_init_t)(struct rte_eth_dev *ethdev, void *init_params);
-- 
2.18.0

RE: [EXT] Re: [PATCH v3 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi Andrew,

Thanks for the review comments.
Please see responses inline.
Kindly review V4 as well.

> -----Original Message-----


<snip>

> > @@ -459,9 +460,23 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
> >   			break;
> >
> >   		case 3: /* Parsing list */
> > -			if (*letter == ']')
> > -				state = 2;
> > -			else if (*letter == '\0')
> > +			if (*letter == ']') {
> > +				/* Multiple representor case has ']' dual
> meaning, first end of
> > +				 * individual pfvf list and other end of
> consolidated list of
> > +				 * representors.
> > +				 * Complete multiple representors list to be
> considered as one
> > +				 * pair value.
> > +				 */
> > +				if ((strcmp("representor", pair->key) == 0)
> &&
> > +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
> ||
> 
> Sorry, but it is unclear why it is not out-of-bound access.

Sorry I missed that, added in V4

> 
> > +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
> ||
> > +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
> ||
> 
> may be it is better to use strncmp() instead?.

Yes strncmp can be used but I kept as is for symmetry with other comparisons.
Moreover I needed 2nd and 3rd letter comparison from current position, so just
for ease I kept as is.

> IMHO it is a bit hard to follow
I reworded the comment in V4 to explain the changes, I hope it is making sense now.


> 
> > +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
> ||
> > +				     (*(letter + 2) == '[' && isdigit(*(letter +
> 3)))))
> > +					state = 3;
> > +				else
> > +					state = 2;
> > +			} else if (*letter == '\0')
> >   				return -EINVAL;
> >   			break;
> >   		}
> > @@ -469,16 +484,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
> >   	}
> >   }
> >
> > +static int
> > +eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs
> *eth_devargs,
> > +				  uint8_t nb_da)
> > +{
> > +	struct rte_eth_devargs *eth_da;
> > +	char da_val[BUFSIZ];
> > +	char delim[] = "]";
> > +	int devargs = 0;
> > +	int result = 0;
> > +	char *token;
> > +
> > +	token = strtok(&p_val[1], delim);
> 
> since strtok() is MT-unsafe, I'd recommend to use strtok_r()

Thanks, changed in V4

> 
> > +	while (token != NULL) {
> > +		eth_da = &eth_devargs[devargs];
> > +		memset(eth_da, 0, sizeof(*eth_da));
> > +		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token :
> token, ']');
> > +		/* Parse the tokenised devarg value */
> > +		result = rte_eth_devargs_parse_representor_ports(da_val,
> eth_da);
> > +		if (result < 0)
> > +			goto parse_cleanup;
> > +		devargs++;
> > +		if (devargs > nb_da) {
> > +			RTE_ETHDEV_LOG_LINE(ERR,
> > +					    "Devargs parsed %d > max array
> size %d",
> > +					    devargs, nb_da);
> > +			result = -1;
> > +			goto parse_cleanup;
> > +		}
> > +		token = strtok(NULL, delim);
> > +	}
> > +
> > +	result = devargs;
> > +
> > +parse_cleanup:
> > +	return result;
> > +
> > +}
> > +
> >   int
> > -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs
> > *eth_da)
> > +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs
> *eth_devargs,
> > +		      uint8_t nb_da)
> 
> I see no single reason to limit nb_da to uint8_t type. IMHO it should be
> 'unsigned int' as an unsigned number of default type.
> 'unsigned int' is used for number of stats and ptypes in array.

Ack, changed in V4

Thanks
Harman

> 
> [snip]

RE: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Chaoyong He]

> -----Original Message-----
> From: Harman Kalra <hkalra@marvell.com>
> Sent: Monday, January 22, 2024 3:19 AM
> To: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@amd.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
> Youb Kim <hyonkim@cisco.com>; Yuying Zhang <Yuying.Zhang@intel.com>;
> Beilei Xing <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> Zhang <qi.z.zhang@intel.com>; Wenjun Wu <wenjun1.wu@intel.com>;
> Dariusz Sosnowski <dsosnowski@nvidia.com>; Viacheslav Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Matan Azrad <matan@nvidia.com>; Chaoyong
> He <chaoyong.he@corigine.com>
> Cc: Harman Kalra <hkalra@marvell.com>
> Subject: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string
> 
> [You don't often get email from hkalra@marvell.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Adding support for parsing multiple representor devargs strings passed to a
> PCI BDF. There may be scenario where port representors for various PFs or VFs
> under PFs are required and all these are representor ports shall be backed by
> single pci device. In such case port representors can be created using devargs
> string:
> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> ---
>  doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
>  .../prog_guide/switch_representation.rst      |   1 +
>  drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
>  drivers/net/enic/enic_ethdev.c                |   4 +-
>  drivers/net/i40e/i40e_ethdev.c                |   4 +-
>  drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
>  drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
>  .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
>  drivers/net/sfc/sfc_ethdev.c                  |   4 +-
>  lib/ethdev/ethdev_driver.c                    | 108 +++++++++++++++---
>  lib/ethdev/ethdev_driver.h                    |   9 +-
>  12 files changed, 122 insertions(+), 36 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
> b/doc/guides/prog_guide/poll_mode_drv.rst
> index c145a9066c..5008b41c60 100644
> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> @@ -376,7 +376,7 @@ parameters to those ports.
> 
>  * ``representor`` for a device which supports the creation of representor ports
>    this argument allows user to specify which switch ports to enable port
> -  representors for. Multiple representors in one device argument is invalid::
> +  representors for::
> 
>     -a DBDF,representor=vf0
>     -a DBDF,representor=vf[0,4,6,9]
> @@ -389,6 +389,8 @@ parameters to those ports.
>     -a DBDF,representor=pf1vf0
>     -a DBDF,representor=pf[0-1]sf[0-127]
>     -a DBDF,representor=pf1
> +   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
> +   (Multiple representors in one device argument can be represented as
> + a list)
> 
>  Note: PMDs are not required to support the standard device arguments and
> users  should consult the relevant PMD documentation to see support
> devargs.
> diff --git a/doc/guides/prog_guide/switch_representation.rst
> b/doc/guides/prog_guide/switch_representation.rst
> index 6fd7b98bdc..46e0ca85a5 100644
> --- a/doc/guides/prog_guide/switch_representation.rst
> +++ b/doc/guides/prog_guide/switch_representation.rst
> @@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for
> applications.
>     -a pci:dbdf,representor=sf1
>     -a pci:dbdf,representor=sf[0-1023]
>     -a pci:dbdf,representor=sf[0,2-1023]
> +   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
> 
>  - As virtual devices, they may be more limited than their physical
>    counterparts, for instance by exposing only a subset of device diff --git
> a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c index
> acf7e6e46e..5d4f599044 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver
> *pci_drv __rte_unused,
> 
>         if (pci_dev->device.devargs) {
>                 ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> -                                           &eth_da);
> -               if (ret)
> +                                           &eth_da, 1);
> +               if (ret < 0)
>                         return ret;
>         }
> 
...
> a/drivers/net/nfp/flower/nfp_flower_representor.c
> b/drivers/net/nfp/flower/nfp_flower_representor.c
> index 5f7c1fa737..63fe37c8d7 100644
> --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> @@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower
> *app_fw_flower)
> 
>         /* Now parse PCI device args passed for representor info */
>         if (pci_dev->device.devargs != NULL) {
> -               ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da);
> -               if (ret != 0) {
> +               ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da, 1);
> +               if (ret < 0) {
>                         PMD_INIT_LOG(ERR, "devarg parse failed");
>                         return -EINVAL;
>                 }

Sorry, I don't quite understand why change 'ret != 0' to 'ret < 0'?
Compare return value with 0 or NULL is the specification for our PMD, we prefer not to change it if don't have a good reason.
Thanks.

> --
> 2.18.0

RE: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi Chaoyong,

Please see responses inline


<snip>

> >                         return ret;
> >         }
> >
> ...
> > a/drivers/net/nfp/flower/nfp_flower_representor.c
> > b/drivers/net/nfp/flower/nfp_flower_representor.c
> > index 5f7c1fa737..63fe37c8d7 100644
> > --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> > +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> > @@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower
> > *app_fw_flower)
> >
> >         /* Now parse PCI device args passed for representor info */
> >         if (pci_dev->device.devargs != NULL) {
> > -               ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> > &eth_da);
> > -               if (ret != 0) {
> > +               ret =
> > + rte_eth_devargs_parse(pci_dev->device.devargs->args,
> > &eth_da, 1);
> > +               if (ret < 0) {
> >                         PMD_INIT_LOG(ERR, "devarg parse failed");
> >                         return -EINVAL;
> >                 }
> 
> Sorry, I don't quite understand why change 'ret != 0' to 'ret < 0'?
> Compare return value with 0 or NULL is the specification for our PMD, we
> prefer not to change it if don't have a good reason.
> Thanks.

To support multiple representors under one PCI BDF, "eth_devargs args" parameter
passed to rte_eth_devargs_parse() is now an array which gets populated with multiple
"struct rte_eth_devargs" elements viz different pfvf representor devargs. 

We are proposing a change to the return value of this API, I.e. negative means error condition
and positive value refers to no of "struct rte_eth_devargs" elements populated. So it can't be
zero, else we wont know how many elements were populated in the array.

Thanks
Harman

> 
> > --
> > 2.18.0

RE: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Chaoyong He]

> 
> Hi Chaoyong,
> 
> Please see responses inline
> 
> 
> <snip>
> 
> > >                         return ret;
> > >         }
> > >
> > ...
> > > a/drivers/net/nfp/flower/nfp_flower_representor.c
> > > b/drivers/net/nfp/flower/nfp_flower_representor.c
> > > index 5f7c1fa737..63fe37c8d7 100644
> > > --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> > > +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> > > @@ -792,8 +792,8 @@ nfp_flower_repr_create(struct
> nfp_app_fw_flower
> > > *app_fw_flower)
> > >
> > >         /* Now parse PCI device args passed for representor info */
> > >         if (pci_dev->device.devargs != NULL) {
> > > -               ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> > > &eth_da);
> > > -               if (ret != 0) {
> > > +               ret =
> > > + rte_eth_devargs_parse(pci_dev->device.devargs->args,
> > > &eth_da, 1);
> > > +               if (ret < 0) {
> > >                         PMD_INIT_LOG(ERR, "devarg parse failed");
> > >                         return -EINVAL;
> > >                 }
> >
> > Sorry, I don't quite understand why change 'ret != 0' to 'ret < 0'?
> > Compare return value with 0 or NULL is the specification for our PMD,
> > we prefer not to change it if don't have a good reason.
> > Thanks.
> 
> To support multiple representors under one PCI BDF, "eth_devargs args"
> parameter passed to rte_eth_devargs_parse() is now an array which gets
> populated with multiple "struct rte_eth_devargs" elements viz different pfvf
> representor devargs.
> 
> We are proposing a change to the return value of this API, I.e. negative means
> error condition and positive value refers to no of "struct rte_eth_devargs"
> elements populated. So it can't be zero, else we wont know how many
> elements were populated in the array.
> 
> Thanks
> Harman
> 

Got it, thanks to make it clear.
Then it looks good to me.

> >
> > > --
> > > 2.18.0

RE: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Ping...
Request for review comments please.

Thanks
Harman

> -----Original Message-----
> From: Harman Kalra <hkalra@marvell.com>
> Sent: Monday, January 22, 2024 12:49 AM
> To: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@amd.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>
> Cc: Harman Kalra <hkalra@marvell.com>
> Subject: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string
> 
> Adding support for parsing multiple representor devargs strings passed to a
> PCI BDF. There may be scenario where port representors for various PFs or
> VFs under PFs are required and all these are representor ports shall be
> backed by single pci device. In such case port representors can be created
> using devargs string:
> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> ---
>  doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
>  .../prog_guide/switch_representation.rst      |   1 +
>  drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
>  drivers/net/enic/enic_ethdev.c                |   4 +-
>  drivers/net/i40e/i40e_ethdev.c                |   4 +-
>  drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
>  drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
>  .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
>  drivers/net/sfc/sfc_ethdev.c                  |   4 +-
>  lib/ethdev/ethdev_driver.c                    | 108 +++++++++++++++---
>  lib/ethdev/ethdev_driver.h                    |   9 +-
>  12 files changed, 122 insertions(+), 36 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
> b/doc/guides/prog_guide/poll_mode_drv.rst
> index c145a9066c..5008b41c60 100644
> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> @@ -376,7 +376,7 @@ parameters to those ports.
> 
>  * ``representor`` for a device which supports the creation of representor
> ports
>    this argument allows user to specify which switch ports to enable port
> -  representors for. Multiple representors in one device argument is invalid::
> +  representors for::
> 
>     -a DBDF,representor=vf0
>     -a DBDF,representor=vf[0,4,6,9]
> @@ -389,6 +389,8 @@ parameters to those ports.
>     -a DBDF,representor=pf1vf0
>     -a DBDF,representor=pf[0-1]sf[0-127]
>     -a DBDF,representor=pf1
> +   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
> +   (Multiple representors in one device argument can be represented as
> + a list)
> 
>  Note: PMDs are not required to support the standard device arguments and
> users  should consult the relevant PMD documentation to see support
> devargs.
> diff --git a/doc/guides/prog_guide/switch_representation.rst
> b/doc/guides/prog_guide/switch_representation.rst
> index 6fd7b98bdc..46e0ca85a5 100644
> --- a/doc/guides/prog_guide/switch_representation.rst
> +++ b/doc/guides/prog_guide/switch_representation.rst
> @@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for
> applications.
>     -a pci:dbdf,representor=sf1
>     -a pci:dbdf,representor=sf[0-1023]
>     -a pci:dbdf,representor=sf[0,2-1023]
> +   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
> 
>  - As virtual devices, they may be more limited than their physical
>    counterparts, for instance by exposing only a subset of device diff --git
> a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c index
> acf7e6e46e..5d4f599044 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver
> *pci_drv __rte_unused,
> 
>  	if (pci_dev->device.devargs) {
>  		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> -					    &eth_da);
> -		if (ret)
> +					    &eth_da, 1);
> +		if (ret < 0)
>  			return ret;
>  	}
> 
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index b04b6c9aa1..33d96ec07a 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -1317,8 +1317,8 @@ static int eth_enic_pci_probe(struct rte_pci_driver
> *pci_drv __rte_unused,
>  	ENICPMD_FUNC_TRACE();
>  	if (pci_dev->device.devargs) {
>  		retval = rte_eth_devargs_parse(pci_dev->device.devargs-
> >args,
> -				&eth_da);
> -		if (retval)
> +				&eth_da, 1);
> +		if (retval < 0)
>  			return retval;
>  	}
>  	if (eth_da.nb_representor_ports > 0 && diff --git
> a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index
> 3ca226156b..4d21341382 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -646,8 +646,8 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> 
>  	if (pci_dev->device.devargs) {
>  		retval = rte_eth_devargs_parse(pci_dev->device.devargs-
> >args,
> -				&eth_da);
> -		if (retval)
> +				&eth_da, 1);
> +		if (retval < 0)
>  			return retval;
>  	}
> 
> diff --git a/drivers/net/ice/ice_dcf_ethdev.c
> b/drivers/net/ice/ice_dcf_ethdev.c
> index 5d845bba31..0e991fe4b8 100644
> --- a/drivers/net/ice/ice_dcf_ethdev.c
> +++ b/drivers/net/ice/ice_dcf_ethdev.c
> @@ -2041,8 +2041,8 @@ eth_ice_dcf_pci_probe(__rte_unused struct
> rte_pci_driver *pci_drv,
>  	if (!ice_devargs_check(pci_dev->device.devargs,
> ICE_DCF_DEVARG_CAP))
>  		return 1;
> 
> -	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da);
> -	if (ret)
> +	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da, 1);
> +	if (ret < 0)
>  		return ret;
> 
>  	ret = rte_eth_dev_pci_generic_probe(pci_dev,
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index d6cf00317e..98e9b8a031 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1765,8 +1765,8 @@ eth_ixgbe_pci_probe(struct rte_pci_driver
> *pci_drv __rte_unused,
> 
>  	if (pci_dev->device.devargs) {
>  		retval = rte_eth_devargs_parse(pci_dev->device.devargs-
> >args,
> -				&eth_da);
> -		if (retval)
> +				&eth_da, 1);
> +		if (retval < 0)
>  			return retval;
>  	} else
>  		memset(&eth_da, 0, sizeof(eth_da));
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> b/drivers/net/mlx5/linux/mlx5_os.c
> index ae82e1e5d8..17061126d5 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -2733,16 +2733,16 @@ mlx5_os_parse_eth_devargs(struct rte_device
> *dev,
>  	memset(eth_da, 0, sizeof(*eth_da));
>  	/* Parse representor information first from class argument. */
>  	if (dev->devargs->cls_str)
> -		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
> -	if (ret != 0) {
> +		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da,
> 1);
> +	if (ret < 0) {
>  		DRV_LOG(ERR, "failed to parse device arguments: %s",
>  			dev->devargs->cls_str);
>  		return -rte_errno;
>  	}
>  	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev-
> >devargs->args) {
>  		/* Parse legacy device argument */
> -		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
> -		if (ret) {
> +		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da, 1);
> +		if (ret < 0) {
>  			DRV_LOG(ERR, "failed to parse device arguments:
> %s",
>  				dev->devargs->args);
>  			return -rte_errno;
> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c
> b/drivers/net/nfp/flower/nfp_flower_representor.c
> index 5f7c1fa737..63fe37c8d7 100644
> --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> @@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower
> *app_fw_flower)
> 
>  	/* Now parse PCI device args passed for representor info */
>  	if (pci_dev->device.devargs != NULL) {
> -		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da);
> -		if (ret != 0) {
> +		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
> &eth_da, 1);
> +		if (ret < 0) {
>  			PMD_INIT_LOG(ERR, "devarg parse failed");
>  			return -EINVAL;
>  		}
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c index
> 6d57b2ba26..0bbcb80365 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -3305,8 +3305,8 @@ sfc_parse_rte_devargs(const char *args, struct
> rte_eth_devargs *devargs)
>  	int rc;
> 
>  	if (args != NULL) {
> -		rc = rte_eth_devargs_parse(args, &eth_da);
> -		if (rc != 0) {
> +		rc = rte_eth_devargs_parse(args, &eth_da, 1);
> +		if (rc < 0) {
>  			SFC_GENERIC_LOG(ERR,
>  					"Failed to parse generic devargs '%s'",
>  					args);
> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c index
> bd917a15fc..4c0d22f838 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2022 Intel Corporation
>   */
> 
> +#include <ctype.h>
>  #include <stdlib.h>
>  #include <pthread.h>
> 
> @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
>  			break;
> 
>  		case 3: /* Parsing list */
> -			if (*letter == ']')
> -				state = 2;
> -			else if (*letter == '\0')
> +			if (*letter == ']') {
> +				/* For devargs having singles lists move to
> state 2 once letter
> +				 * becomes ']' so each can be considered as
> different pair key
> +				 * value. But in nested lists case e.g. multiple
> representors
> +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete
> nested list should
> +				 * be considered as one pair value, hence
> checking if end of outer
> +				 * list ']' is reached else stay on state 3.
> +				 */
> +				if ((strcmp("representor", pair->key) == 0)
> 	    &&
> +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0'
> &&
> +				     *(letter + 3) != '\0')			    &&
> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
> ||
> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f') ||
> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f') ||
> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
> ||
> +				     (*(letter + 2) == '[' && isdigit(*(letter +
> 3)))))
> +					state = 3;
> +				else
> +					state = 2;
> +			} else if (*letter == '\0')
>  				return -EINVAL;
>  			break;
>  		}
> @@ -469,16 +487,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
>  	}
>  }
> 
> +static int
> +eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs
> *eth_devargs,
> +				  unsigned int nb_da)
> +{
> +	struct rte_eth_devargs *eth_da;
> +	char da_val[BUFSIZ];
> +	char *token, *sptr;
> +	char delim[] = "]";
> +	int devargs = 0;
> +	int result = 0;
> +
> +	token = strtok_r(&p_val[1], delim, &sptr);
> +	while (token != NULL) {
> +		eth_da = &eth_devargs[devargs];
> +		memset(eth_da, 0, sizeof(*eth_da));
> +		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token :
> token, ']');
> +		/* Parse the tokenised devarg value */
> +		result = rte_eth_devargs_parse_representor_ports(da_val,
> eth_da);
> +		if (result < 0)
> +			goto parse_cleanup;
> +		devargs++;
> +		if (devargs > (int)nb_da) {
> +			RTE_ETHDEV_LOG_LINE(ERR,
> +					    "Devargs parsed %d > max array
> size %d",
> +					    devargs, nb_da);
> +			result = -1;
> +			goto parse_cleanup;
> +		}
> +		token = strtok_r(NULL, delim, &sptr);
> +	}
> +
> +	result = devargs;
> +
> +parse_cleanup:
> +	return result;
> +
> +}
> +
>  int
> -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs
> *eth_devargs,
> +		      unsigned int nb_da)
>  {
> -	struct rte_kvargs args;
> +	struct rte_eth_devargs *eth_da;
>  	struct rte_kvargs_pair *pair;
> +	struct rte_kvargs args;
> +	static bool dup_rep;
> +	int devargs = 0;
>  	unsigned int i;
>  	int result = 0;
> 
> -	memset(eth_da, 0, sizeof(*eth_da));
> -
>  	result = eth_dev_devargs_tokenise(&args, dargs);
>  	if (result < 0)
>  		goto parse_cleanup;
> @@ -486,18 +544,40 @@ rte_eth_devargs_parse(const char *dargs, struct
> rte_eth_devargs *eth_da)
>  	for (i = 0; i < args.count; i++) {
>  		pair = &args.pairs[i];
>  		if (strcmp("representor", pair->key) == 0) {
> -			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
> -				RTE_ETHDEV_LOG_LINE(ERR, "duplicated
> representor key: %s",
> -					dargs);
> +			if (dup_rep) {
> +				RTE_ETHDEV_LOG_LINE(ERR, "Duplicated
> representor key: %s",
> +						    pair->value);
>  				result = -1;
>  				goto parse_cleanup;
>  			}
> -			result = rte_eth_devargs_parse_representor_ports(
> -					pair->value, eth_da);
> -			if (result < 0)
> -				goto parse_cleanup;
> +
> +			if (pair->value[0] == '[' && !isdigit(pair->value[1])) {
> +				/* Multiple representor list case */
> +				devargs =
> eth_dev_tokenise_representor_list(pair->value,
> +
> eth_devargs, nb_da);
> +				if (devargs < 0)
> +					goto parse_cleanup;
> +			} else {
> +				/* Single representor case */
> +				eth_da = &eth_devargs[devargs];
> +				memset(eth_da, 0, sizeof(*eth_da));
> +				result =
> +
> 	rte_eth_devargs_parse_representor_ports(pair->value,
> +
> 	eth_da);
> +				if (result < 0)
> +					goto parse_cleanup;
> +				devargs++;
> +				if (devargs > (int)nb_da) {
> +					RTE_ETHDEV_LOG_LINE(ERR,
> "Devargs parsed %d > max array size %d",
> +							    devargs, nb_da);
> +					result = -1;
> +					goto parse_cleanup;
> +				}
> +			}
> +			dup_rep = true;
>  		}
>  	}
> +	result = devargs;
> 
>  parse_cleanup:
>  	free(args.str);
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h index
> b482cd12bb..fd6f404e9a 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1800,14 +1800,17 @@ rte_eth_representor_id_get(uint16_t port_id,
>   * @param devargs
>   *  device arguments
>   * @param eth_devargs
> - *  parsed ethdev specific arguments.
> + *  contiguous memory populated with parsed ethdev specific arguments.
> + * @param nb_da
> + *  size of eth_devargs array passed
>   *
>   * @return
> - *   Negative errno value on error, 0 on success.
> + *   Negative errno value on error, no of devargs parsed on success.
>   */
>  __rte_internal
>  int
> -rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs
> *eth_devargs);
> +rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs
> *eth_devargs,
> +		      unsigned int nb_da);
> 
> 
>  typedef int (*ethdev_init_t)(struct rte_eth_dev *ethdev, void *init_params);
> --
> 2.18.0

Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Ferruh Yigit]

On 1/21/2024 7:19 PM, Harman Kalra wrote:
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> 

This patch is to be able to parse multiple representor device argument,
but I am concerned how it is used.

When there are multiple representor ports backed up by same device,
can't it cause syncronization issues, like if two representor interfaces
used for conflicting configurations. Or if datapath will be supported,
what if two representator used simultaneously.



Meanwhile please check some comments below related to the parser code.

<...>

> @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>  			break;
>  
>  		case 3: /* Parsing list */
> -			if (*letter == ']')
> -				state = 2;
> -			else if (*letter == '\0')
> +			if (*letter == ']') {
> +				/* For devargs having singles lists move to state 2 once letter
> +				 * becomes ']' so each can be considered as different pair key
> +				 * value. But in nested lists case e.g. multiple representors
> +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete nested list should
> +				 * be considered as one pair value, hence checking if end of outer
> +				 * list ']' is reached else stay on state 3.
> +				 */
> +				if ((strcmp("representor", pair->key) == 0)	    &&
> +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0' &&
> +				     *(letter + 3) != '\0')			    &&
> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f') ||
> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f') ||
> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f') ||
> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
> +				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
>

Above is hard to understand but there are some assumptions in the input,
can we list supported syntax in the comment to make it more clear.

For example following seems not support, can you please check if
intentional:
[vf0,vf1] // I am aware this can be put as vf[0,1] too
[vf[0,1],3]
[vf[0],vf[1]]

I am not saying above should be supported, but syntax should be clear
what is supported what is not.


Also I can't check but is the redundant input verified, like:
[vf[0-3],vf[3,4]]

RE: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi Ferruh

Thanks for the review
Please find response inline


> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Friday, January 26, 2024 7:13 PM
> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>
> Subject: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor
> devargs string
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 1/21/2024 7:19 PM, Harman Kalra wrote:
> > Adding support for parsing multiple representor devargs strings passed
> > to a PCI BDF. There may be scenario where port representors for
> > various PFs or VFs under PFs are required and all these are
> > representor ports shall be backed by single pci device. In such case
> > port representors can be created using devargs string:
> > <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> >
> 
> This patch is to be able to parse multiple representor device argument, but I
> am concerned how it is used.

In Marvell CNXK port representor solution all representors are backed by a single 
PCI device (we call it as eswitch device). Eswitch device is not DPDK ethdev device
but an internal device with NIC capabilities and is configured with as many no of
Rxqs and Txqs as port representor required.
Hence each port representor will have dedicated RxQ/TxQ pair to communicate with
respective represented PF or VF. 


> 
> When there are multiple representor ports backed up by same device, can't
> it cause syncronization issues, like if two representor interfaces used for
> conflicting configurations. Or if datapath will be supported, what if two
> representator used simultaneously.

As I mentioned above, each port representor will have dedicated RxQ and TxQ,
worker cores can poll on respective queues without any synchronization issue.
I hope I am able to explain the use case.

> 
> 




> 
> Meanwhile please check some comments below related to the parser code.
> 
> <...>
> 
> > @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> *arglist, const char *str_in)
> >  			break;
> >
> >  		case 3: /* Parsing list */
> > -			if (*letter == ']')
> > -				state = 2;
> > -			else if (*letter == '\0')
> > +			if (*letter == ']') {
> > +				/* For devargs having singles lists move to
> state 2 once letter
> > +				 * becomes ']' so each can be considered as
> different pair key
> > +				 * value. But in nested lists case e.g. multiple
> representors
> > +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete
> nested list should
> > +				 * be considered as one pair value, hence
> checking if end of outer
> > +				 * list ']' is reached else stay on state 3.
> > +				 */
> > +				if ((strcmp("representor", pair->key) == 0)
> 	    &&
> > +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0'
> &&
> > +				     *(letter + 3) != '\0')			    &&
> > +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
> ||
> > +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
> ||
> > +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
> ||
> > +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
> ||
> > +				     (*(letter + 2) == '[' && isdigit(*(letter +
> 3)))))
> >
> 
> Above is hard to understand but there are some assumptions in the input,
> can we list supported syntax in the comment to make it more clear.
> 
> For example following seems not support, can you please check if
> intentional:
> [vf0,vf1] // I am aware this can be put as vf[0,1] too [vf[0,1],3] [vf[0],vf[1]]

Intention behind above change is just to detect if its nested list i.e. multiple
devargs (constituting lists as well) or a single devarg with a list.

pf0vf[1-4] is a single list devarg example, while [pf[0-3],pfvf[3,4-6]] is
nested list example, where multiple devargs pf[0-3] and pfvf[3,4-6]
(which are also lists) are bind to a list of devargs.

And as I mentioned in the comment, complete "nested list should be considered 
as one pair value", hence entire list i.e. [vf[0,1],3] [vf[0],vf[1]] will be one
value of arglist and later eth_dev_tokenise_representor_list() will tokenize
and feed to rte_eth_devargs_parse_representor_ports().

Whether any format is supported or not should be handled by 
rte_eth_devargs_parse_representor_ports()


> 
> I am not saying above should be supported, but syntax should be clear what
> is supported what is not.
> 
> 
> Also I can't check but is the redundant input verified, like:
> [vf[0-3],vf[3,4]]

IMO, this should be handled by PMD, if any representor already created should
return an error.

Thanks
Harman

> 

Re: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Ferruh Yigit]

On 1/29/2024 6:20 PM, Harman Kalra wrote:
> Hi Ferruh
> 
> Thanks for the review
> Please find response inline
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Friday, January 26, 2024 7:13 PM
>> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org; Thomas Monjalon
>> <thomas@monjalon.net>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
>> Hyong Youb Kim <hyonkim@cisco.com>; Yuying Zhang
>> <Yuying.Zhang@intel.com>; Beilei Xing <beilei.xing@intel.com>; Qiming Yang
>> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
>> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
>> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
>> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
>> Azrad <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>
>> Subject: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor
>> devargs string
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 1/21/2024 7:19 PM, Harman Kalra wrote:
>>> Adding support for parsing multiple representor devargs strings passed
>>> to a PCI BDF. There may be scenario where port representors for
>>> various PFs or VFs under PFs are required and all these are
>>> representor ports shall be backed by single pci device. In such case
>>> port representors can be created using devargs string:
>>> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
>>>
>>
>> This patch is to be able to parse multiple representor device argument, but I
>> am concerned how it is used.
> 
> In Marvell CNXK port representor solution all representors are backed by a single 
> PCI device (we call it as eswitch device). Eswitch device is not DPDK ethdev device
> but an internal device with NIC capabilities and is configured with as many no of
> Rxqs and Txqs as port representor required.
> Hence each port representor will have dedicated RxQ/TxQ pair to communicate with
> respective represented PF or VF. 
> 

ack, thanks for clarification.

Just to double check, is the cnxk driver support of new syntax planned
for this release?
It helps if the RFC is out before merging this patch.

> 
>>
>> When there are multiple representor ports backed up by same device, can't
>> it cause syncronization issues, like if two representor interfaces used for
>> conflicting configurations. Or if datapath will be supported, what if two
>> representator used simultaneously.
> 
> As I mentioned above, each port representor will have dedicated RxQ and TxQ,
> worker cores can poll on respective queues without any synchronization issue.
> I hope I am able to explain the use case.
> 

ack

>>
>>
> 
> 
> 
> 
>>
>> Meanwhile please check some comments below related to the parser code.
>>
>> <...>
>>
>>> @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs
>> *arglist, const char *str_in)
>>>  			break;
>>>
>>>  		case 3: /* Parsing list */
>>> -			if (*letter == ']')
>>> -				state = 2;
>>> -			else if (*letter == '\0')
>>> +			if (*letter == ']') {
>>> +				/* For devargs having singles lists move to
>> state 2 once letter
>>> +				 * becomes ']' so each can be considered as
>> different pair key
>>> +				 * value. But in nested lists case e.g. multiple
>> representors
>>> +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete
>> nested list should
>>> +				 * be considered as one pair value, hence
>> checking if end of outer
>>> +				 * list ']' is reached else stay on state 3.
>>> +				 */
>>> +				if ((strcmp("representor", pair->key) == 0)
>> 	    &&
>>> +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0'
>> &&
>>> +				     *(letter + 3) != '\0')			    &&
>>> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
>> ||
>>> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
>> ||
>>> +				     (*(letter + 2) == '[' && isdigit(*(letter +
>> 3)))))
>>>
>>
>> Above is hard to understand but there are some assumptions in the input,
>> can we list supported syntax in the comment to make it more clear.
>>
>> For example following seems not support, can you please check if
>> intentional:
>> [vf0,vf1] // I am aware this can be put as vf[0,1] too [vf[0,1],3] [vf[0],vf[1]]
> 
> Intention behind above change is just to detect if its nested list i.e. multiple
> devargs (constituting lists as well) or a single devarg with a list.
> 
> pf0vf[1-4] is a single list devarg example, while [pf[0-3],pfvf[3,4-6]] is
> nested list example, where multiple devargs pf[0-3] and pfvf[3,4-6]
> (which are also lists) are bind to a list of devargs.
> 
> And as I mentioned in the comment, complete "nested list should be considered 
> as one pair value", hence entire list i.e. [vf[0,1],3] [vf[0],vf[1]] will be one
> value of arglist and later eth_dev_tokenise_representor_list() will tokenize
> and feed to rte_eth_devargs_parse_representor_ports().
> 
> Whether any format is supported or not should be handled by 
> rte_eth_devargs_parse_representor_ports()
> 

Agree but this is something user facing, user should know the syntax to
use it but some corner cases are not documented well and not trivial to
grasp from above code.

What do you think about adding some unit tests for parser, it can be
some pointer to the intention of what should be supported and what not,
also increases our confidence to the code?

> 
>>
>> I am not saying above should be supported, but syntax should be clear what
>> is supported what is not.
>>
>>
>> Also I can't check but is the redundant input verified, like:
>> [vf[0-3],vf[3,4]]
> 
> IMO, this should be handled by PMD, if any representor already created should
> return an error.
> 

fair enough

[PATCH v5 0/2] multiple representors in one device

[Harman Kalra]

Following series adds support to enable creation of multiple representors
under one base device. There may be scenarios where port representors for
multiple PFs or VFs under PF are required and all these representor ports
created under a single pci device. Marvell CNXK port representor solution
is designed around this scenario where all representors are backed by a
single switch device.

Earlier this change was implemented as part of the Marvell CNXK port
representor series but after suggestions from Thomas we would like
to propose these changes in common code.
https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785

V5:
- Added test cases to demonstrate valid and invalid cases
- changed the tokenizing logic to address all valid cases

V4:
- Used MT safe strtok_r in place of strtok
- Reworded some comments

V3:
- Fix duplicate representor devarg key handling logic

V2:
- Updated the multiple representor devarg pattern to list
i.e. representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
- Introduced size of array as third argument to rte_eth_devargs_parse()
to avoid array corruption
- Squashed separate document patch 


Harman Kalra (2):
  ethdev: parsing multiple representor devargs string
  test/devargs: add eth devargs parse cases

 app/test/test_devargs.c                       | 107 +++++++++++
 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 175 ++++++++++++++++--
 lib/ethdev/ethdev_driver.h                    |   9 +-
 13 files changed, 296 insertions(+), 36 deletions(-)

-- 
2.18.0

[PATCH v5 1/2] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Adding support for parsing multiple representor devargs strings
passed to a PCI BDF. There may be scenario where port representors
for various PFs or VFs under PFs are required and all these are
representor ports shall be backed by single pci device. In such
case port representors can be created using devargs string:
<PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst       |   4 +-
 .../prog_guide/switch_representation.rst      |   1 +
 drivers/net/bnxt/bnxt_ethdev.c                |   4 +-
 drivers/net/enic/enic_ethdev.c                |   4 +-
 drivers/net/i40e/i40e_ethdev.c                |   4 +-
 drivers/net/ice/ice_dcf_ethdev.c              |   4 +-
 drivers/net/ixgbe/ixgbe_ethdev.c              |   4 +-
 drivers/net/mlx5/linux/mlx5_os.c              |   8 +-
 .../net/nfp/flower/nfp_flower_representor.c   |   4 +-
 drivers/net/sfc/sfc_ethdev.c                  |   4 +-
 lib/ethdev/ethdev_driver.c                    | 175 ++++++++++++++++--
 lib/ethdev/ethdev_driver.h                    |   9 +-
 12 files changed, 189 insertions(+), 36 deletions(-)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..5008b41c60 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -376,7 +376,7 @@ parameters to those ports.
 
 * ``representor`` for a device which supports the creation of representor ports
   this argument allows user to specify which switch ports to enable port
-  representors for. Multiple representors in one device argument is invalid::
+  representors for::
 
    -a DBDF,representor=vf0
    -a DBDF,representor=vf[0,4,6,9]
@@ -389,6 +389,8 @@ parameters to those ports.
    -a DBDF,representor=pf1vf0
    -a DBDF,representor=pf[0-1]sf[0-127]
    -a DBDF,representor=pf1
+   -a DBDF,representor=[pf[0-1],pf2vf[0-2],pf3[3,5-8]]
+   (Multiple representors in one device argument can be represented as a list)
 
 Note: PMDs are not required to support the standard device arguments and users
 should consult the relevant PMD documentation to see support devargs.
diff --git a/doc/guides/prog_guide/switch_representation.rst b/doc/guides/prog_guide/switch_representation.rst
index 6fd7b98bdc..46e0ca85a5 100644
--- a/doc/guides/prog_guide/switch_representation.rst
+++ b/doc/guides/prog_guide/switch_representation.rst
@@ -77,6 +77,7 @@ thought as a software "patch panel" front-end for applications.
    -a pci:dbdf,representor=sf1
    -a pci:dbdf,representor=sf[0-1023]
    -a pci:dbdf,representor=sf[0,2-1023]
+   -a pci:dbdf,representor=[pf[0-1],pf2vf[0-2],pf3[3,5]]
 
 - As virtual devices, they may be more limited than their physical
   counterparts, for instance by exposing only a subset of device
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index acf7e6e46e..5d4f599044 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -6383,8 +6383,8 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-					    &eth_da);
-		if (ret)
+					    &eth_da, 1);
+		if (ret < 0)
 			return ret;
 	}
 
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index b04b6c9aa1..33d96ec07a 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1317,8 +1317,8 @@ static int eth_enic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	ENICPMD_FUNC_TRACE();
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 	if (eth_da.nb_representor_ports > 0 &&
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3ca226156b..4d21341382 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -646,8 +646,8 @@ eth_i40e_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	}
 
diff --git a/drivers/net/ice/ice_dcf_ethdev.c b/drivers/net/ice/ice_dcf_ethdev.c
index 5d845bba31..0e991fe4b8 100644
--- a/drivers/net/ice/ice_dcf_ethdev.c
+++ b/drivers/net/ice/ice_dcf_ethdev.c
@@ -2041,8 +2041,8 @@ eth_ice_dcf_pci_probe(__rte_unused struct rte_pci_driver *pci_drv,
 	if (!ice_devargs_check(pci_dev->device.devargs, ICE_DCF_DEVARG_CAP))
 		return 1;
 
-	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-	if (ret)
+	ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+	if (ret < 0)
 		return ret;
 
 	ret = rte_eth_dev_pci_generic_probe(pci_dev,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d6cf00317e..98e9b8a031 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1765,8 +1765,8 @@ eth_ixgbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 	if (pci_dev->device.devargs) {
 		retval = rte_eth_devargs_parse(pci_dev->device.devargs->args,
-				&eth_da);
-		if (retval)
+				&eth_da, 1);
+		if (retval < 0)
 			return retval;
 	} else
 		memset(&eth_da, 0, sizeof(eth_da));
diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index ae82e1e5d8..17061126d5 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -2733,16 +2733,16 @@ mlx5_os_parse_eth_devargs(struct rte_device *dev,
 	memset(eth_da, 0, sizeof(*eth_da));
 	/* Parse representor information first from class argument. */
 	if (dev->devargs->cls_str)
-		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da);
-	if (ret != 0) {
+		ret = rte_eth_devargs_parse(dev->devargs->cls_str, eth_da, 1);
+	if (ret < 0) {
 		DRV_LOG(ERR, "failed to parse device arguments: %s",
 			dev->devargs->cls_str);
 		return -rte_errno;
 	}
 	if (eth_da->type == RTE_ETH_REPRESENTOR_NONE && dev->devargs->args) {
 		/* Parse legacy device argument */
-		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da);
-		if (ret) {
+		ret = rte_eth_devargs_parse(dev->devargs->args, eth_da, 1);
+		if (ret < 0) {
 			DRV_LOG(ERR, "failed to parse device arguments: %s",
 				dev->devargs->args);
 			return -rte_errno;
diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 5f7c1fa737..63fe37c8d7 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -792,8 +792,8 @@ nfp_flower_repr_create(struct nfp_app_fw_flower *app_fw_flower)
 
 	/* Now parse PCI device args passed for representor info */
 	if (pci_dev->device.devargs != NULL) {
-		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da);
-		if (ret != 0) {
+		ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, &eth_da, 1);
+		if (ret < 0) {
 			PMD_INIT_LOG(ERR, "devarg parse failed");
 			return -EINVAL;
 		}
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6d57b2ba26..0bbcb80365 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3305,8 +3305,8 @@ sfc_parse_rte_devargs(const char *args, struct rte_eth_devargs *devargs)
 	int rc;
 
 	if (args != NULL) {
-		rc = rte_eth_devargs_parse(args, &eth_da);
-		if (rc != 0) {
+		rc = rte_eth_devargs_parse(args, &eth_da, 1);
+		if (rc < 0) {
 			SFC_GENERIC_LOG(ERR,
 					"Failed to parse generic devargs '%s'",
 					args);
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..7e683bcaa1 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2022 Intel Corporation
  */
 
+#include <ctype.h>
 #include <stdlib.h>
 #include <pthread.h>
 
@@ -459,26 +460,157 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
 			break;
 
 		case 3: /* Parsing list */
-			if (*letter == ']')
-				state = 2;
-			else if (*letter == '\0')
+			if (*letter == ']') {
+				/* For devargs having singles lists move to state 2 once letter
+				 * becomes ']' so each can be considered as different pair key
+				 * value. But in nested lists case e.g. multiple representors
+				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete nested list should
+				 * be considered as one pair value, hence checking if end of outer
+				 * list ']' is reached else stay on state 3.
+				 */
+				if ((strcmp("representor", pair->key) == 0) &&
+				    (*(letter + 1) != '\0' && *(letter + 2) != '\0' &&
+				     *(letter + 3) != '\0')			    &&
+				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 's' && *(letter + 3) == 'f')   ||
+				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
+				     (*(letter + 2) == '[' && isdigit(*(letter + 3))) ||
+				     (isdigit(*(letter + 2)))))
+					state = 3;
+				else
+					state = 2;
+			} else if (*letter == '\0') {
 				return -EINVAL;
+			}
 			break;
 		}
 		letter++;
 	}
 }
 
+static int
+devargs_parse_representor_ports(struct rte_eth_devargs *eth_devargs, char
+				*da_val, unsigned int da_idx, unsigned int nb_da)
+{
+	struct rte_eth_devargs *eth_da;
+	int result = 0;
+
+	if (da_idx + 1 > nb_da) {
+		RTE_ETHDEV_LOG_LINE(ERR, "Devargs parsed %d > max array size %d",
+			       da_idx + 1, nb_da);
+		result = -1;
+		goto parse_cleanup;
+	}
+	eth_da = &eth_devargs[da_idx];
+	memset(eth_da, 0, sizeof(*eth_da));
+	RTE_ETHDEV_LOG_LINE(DEBUG, "	  Devargs idx %d value %s", da_idx, da_val);
+	result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
+
+parse_cleanup:
+	return result;
+}
+
+static int
+eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
+				  unsigned int nb_da)
+{
+	char da_val[BUFSIZ], str[BUFSIZ];
+	bool is_rep_portid_list = true;
+	unsigned int devargs = 0;
+	int result = 0, len = 0;
+	int i = 0, j = 0;
+	char *pos;
+
+	pos = p_val;
+	/* Length of consolidated list */
+	while (*pos++ != '\0') {
+		len++;
+		if (isalpha(*pos))
+			is_rep_portid_list = false;
+	}
+
+	/* List of representor portIDs i.e.[1,2,3] should be considered as single representor case*/
+	if (is_rep_portid_list) {
+		result = devargs_parse_representor_ports(eth_devargs, p_val, 0, 1);
+		if (result < 0)
+			return result;
+
+		devargs++;
+		return devargs;
+	}
+
+	memset(str, 0, BUFSIZ);
+	/* Remove the exterior [] of the consolidated list */
+	strncpy(str, &p_val[1], len - 2);
+	while (1) {
+		if (str[i] == '\0') {
+			if (da_val[0] != '\0') {
+				result = devargs_parse_representor_ports(eth_devargs, da_val,
+									 devargs, nb_da);
+				if (result < 0)
+					goto parse_cleanup;
+
+				devargs++;
+			}
+			break;
+		}
+		if (str[i] == ',' || str[i] == '[') {
+			if (str[i] == ',') {
+				if (da_val[0] != '\0') {
+					da_val[j + 1] = '\0';
+					result = devargs_parse_representor_ports(eth_devargs,
+										 da_val, devargs,
+										 nb_da);
+					if (result < 0)
+						goto parse_cleanup;
+
+					devargs++;
+					j = 0;
+					memset(da_val, 0, BUFSIZ);
+				}
+			}
+
+			if (str[i] == '[') {
+				while (str[i] != ']' || isalpha(str[i + 1])) {
+					da_val[j] = str[i];
+					j++;
+					i++;
+				}
+				da_val[j] = ']';
+				da_val[j + 1] = '\0';
+				result = devargs_parse_representor_ports(eth_devargs, da_val,
+									 devargs, nb_da);
+				if (result < 0)
+					goto parse_cleanup;
+
+				devargs++;
+				j = 0;
+				memset(da_val, 0, BUFSIZ);
+			}
+		} else {
+			da_val[j] = str[i];
+			j++;
+		}
+		i++;
+	}
+	result = devargs;
+
+parse_cleanup:
+	return result;
+}
+
 int
-rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
+rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
+		      unsigned int nb_da)
 {
-	struct rte_kvargs args;
 	struct rte_kvargs_pair *pair;
+	struct rte_kvargs args;
+	bool dup_rep = false;
+	int devargs = 0;
 	unsigned int i;
 	int result = 0;
 
-	memset(eth_da, 0, sizeof(*eth_da));
-
 	result = eth_dev_devargs_tokenise(&args, dargs);
 	if (result < 0)
 		goto parse_cleanup;
@@ -486,18 +618,33 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-				RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor key: %s",
-					dargs);
+			if (dup_rep) {
+				RTE_ETHDEV_LOG_LINE(ERR, "Duplicated representor key: %s",
+						    pair->value);
 				result = -1;
 				goto parse_cleanup;
 			}
-			result = rte_eth_devargs_parse_representor_ports(
-					pair->value, eth_da);
-			if (result < 0)
-				goto parse_cleanup;
+
+			RTE_ETHDEV_LOG_LINE(DEBUG, "Devarg pattern: %s", pair->value);
+			if (pair->value[0] == '[') {
+				/* Multiple representor list case */
+				devargs = eth_dev_tokenise_representor_list(pair->value,
+									    eth_devargs, nb_da);
+				if (devargs < 0)
+					goto parse_cleanup;
+			} else {
+				/* Single representor case */
+				devargs = devargs_parse_representor_ports(eth_devargs, pair->value,
+									  0, 1);
+				if (devargs < 0)
+					goto parse_cleanup;
+				devargs++;
+			}
+			dup_rep = true;
 		}
 	}
+	RTE_ETHDEV_LOG_LINE(DEBUG, "Total devargs parsed %d", devargs);
+	result = devargs;
 
 parse_cleanup:
 	free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index f05f68a67c..23df06d289 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1802,14 +1802,17 @@ rte_eth_representor_id_get(uint16_t port_id,
  * @param devargs
  *  device arguments
  * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.
+ * @param nb_da
+ *  size of eth_devargs array passed
  *
  * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
  */
 __rte_internal
 int
-rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs);
+rte_eth_devargs_parse(const char *devargs, struct rte_eth_devargs *eth_devargs,
+		      unsigned int nb_da);
 
 
 typedef int (*ethdev_init_t)(struct rte_eth_dev *ethdev, void *init_params);
-- 
2.18.0

[PATCH v5 2/2] test/devargs: add eth devargs parse cases

[Harman Kalra]

Adding new eth devargs parsing test cases which can demonstrate
valid and invalid usage of devargs patterns.

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
 app/test/test_devargs.c | 107 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index f977d44448..4166b2bea2 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -6,6 +6,7 @@
 #include <stdio.h>
 #include <string.h>
 
+#include <ethdev_driver.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_kvargs.h>
@@ -201,6 +202,106 @@ test_invalid_devargs(void)
 	return fail;
 }
 
+struct devargs_parse_case {
+	const char *devargs;
+	uint8_t devargs_count;
+};
+
+static int
+test_valid_devargs_parsing(void)
+{
+	static const struct devargs_parse_case list[] = {
+		/* Single representors patterns */
+		{"representor=1", 1},
+		{"representor=[1,2,3]", 1},
+		{"representor=[0-3,5,7]", 1},
+		{"representor=vf[0-1]", 1},
+		{"representor=sf[0-1]", 1},
+		{"representor=pf1vf[0-1]", 1},
+		{"representor=pf[0-1]vf[1-2]", 1},
+		{"representor=pf[0-1]sf[1-2]", 1},
+		{"representor=c0pf[0-1]", 1},
+		{"representor=sf[1,2]vf[2]", 1},  /* Only SF ports will be represented */
+		{"representor=vf2sf[1-2]", 1},    /* Only VF ports will be represented */
+		{"representor=c[0-1]pf[0-1]vf[1-2]", 1},
+		/* Single devarg inside multiple representor pattern */
+		{"representor=[c[0-1]pf[0-1]vf[1-2]]", 1},
+		/* Multiple representors patterns */
+		{"representor=[vf0,3]", 2},
+		{"representor=[vf0,vf1]", 2},
+		{"representor=[vf[0],vf[1]]", 2},
+		{"representor=[vf[0,1],3]", 2},
+		{"representor=[vf[0,1],[3]]", 2},
+		{"representor=[pf1vf[0-1],3]", 2},
+		{"representor=[pf1vf[0-1],pf[0-1]]", 2},
+		{"representor=[pf1,pf3,pf1vf[0-1],vf[0],vf[1],vf0,vf1,vf2]", 8},
+		{"representor=[1,3,5,pf1,pf2,pf3,pf1vf[0-1],vf[0],vf[1],vf0,vf1,vf2]", 12},
+		{"representor=[[1,3,5],pf1,pf2,pf3,pf1vf[0-1],vf[0],vf[1],vf0,vf1,vf2]", 10},
+		{"representor=[c[0,2-4]pf[1,6]vf[0-1],vf[0],vf4,[3-5,7],pf1vf[0-1,6],pf[2,4,6]]", 6}
+	};
+	struct rte_eth_devargs eth_da[RTE_MAX_ETHPORTS];
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(eth_da, 0, RTE_MAX_ETHPORTS * sizeof(*eth_da));
+		ret = rte_eth_devargs_parse(list[i].devargs, eth_da, RTE_MAX_ETHPORTS);
+		if (ret <= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			fail = ret;
+			break;
+		}
+
+		/* Check the return value, count should be equivalent to no of devargs. */
+		if (ret != list[i].devargs_count) {
+			printf("Devargs returned count %d != expected count %d\n", ret,
+			       list[i].devargs_count);
+			fail = -1;
+			break;
+		}
+	}
+	return fail;
+}
+
+static int
+test_invalid_devargs_parsing(void)
+{
+	static const char * const list[] = {
+		"representor=1,2,3,4",			/* Out [] missing */
+		"representor=[1 2 3]",			/* , missing */
+		"representor=c[1,2]",			/* Only controller no valid */
+		"representor=c0vf[0-1]",		/* Controller with vf alone not valid */
+		"representor=c0sf[0-1]",		/* Controller with sf alone not valid */
+		"representor=vf[0-1],vf3",		/* Out [] missing */
+		"representor=[[vf0,3]]",		/* Double [] is invalid */
+		"representor=pfvf[1-2]",		/* No PF index provided */
+		"representor=pf1[vf[1-2]]",		/* Additional [] across vf */
+		"representor=c0[pf1vf[1-2]]",		/* Additional [] across pfvf */
+		"representor=c0[pf1[vf[1-2]]]",		/* Additional [] across pfvf */
+		"representor=[pf1vf[0-1], pf[0-1]]",	/* Space between two devargs is invalid */
+		"representor=[pf1vf[0-1], 3]",		/* Space between two devargs is invalid */
+		"representor=pf1vf[1-2],representor=1", /* Multiple representor keys */
+	};
+	struct rte_eth_devargs eth_da[RTE_MAX_ETHPORTS];
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(eth_da, 0, RTE_MAX_ETHPORTS * sizeof(*eth_da));
+		ret = rte_eth_devargs_parse(list[i], eth_da, RTE_MAX_ETHPORTS);
+		if (ret > 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+			break;
+		}
+	}
+	return fail;
+}
+
 static int
 test_devargs(void)
 {
@@ -210,6 +311,12 @@ test_devargs(void)
 	printf("== test invalid case ==\n");
 	if (test_invalid_devargs() < 0)
 		return -1;
+	printf("== test devargs parsing valid case ==\n");
+	if (test_valid_devargs_parsing() < 0)
+		return -1;
+	printf("== test devargs parsing invalid case ==\n");
+	if (test_invalid_devargs_parsing() < 0)
+		return -1;
 	return 0;
 }
 
-- 
2.18.0

RE: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor devargs string

[Harman Kalra]

Hi Ferruh

Please find response inline

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, January 31, 2024 4:40 AM
> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org; Thomas Monjalon
> <thomas@monjalon.net>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
> Youb Kim <hyonkim@cisco.com>; Yuying Zhang <Yuying.Zhang@intel.com>;
> Beilei Xing <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> Zhang <qi.z.zhang@intel.com>; Wenjun Wu <wenjun1.wu@intel.com>;
> Dariusz Sosnowski <dsosnowski@nvidia.com>; Viacheslav Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Matan Azrad <matan@nvidia.com>; Chaoyong
> He <chaoyong.he@corigine.com>
> Subject: Re: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple representor
> devargs string
> 
> On 1/29/2024 6:20 PM, Harman Kalra wrote:
> > Hi Ferruh
> >
> > Thanks for the review
> > Please find response inline
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Friday, January 26, 2024 7:13 PM
> >> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org; Thomas
> Monjalon
> >> <thomas@monjalon.net>; Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>; Ajit Khaparde
> >> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> Hyong
> >> Youb Kim <hyonkim@cisco.com>; Yuying Zhang
> <Yuying.Zhang@intel.com>;
> >> Beilei Xing <beilei.xing@intel.com>; Qiming Yang
> >> <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Wenjun Wu
> >> <wenjun1.wu@intel.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>;
> >> Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Ori Kam
> >> <orika@nvidia.com>; Suanming Mou <suanmingm@nvidia.com>; Matan
> Azrad
> >> <matan@nvidia.com>; Chaoyong He <chaoyong.he@corigine.com>
> >> Subject: [EXT] Re: [PATCH v4 1/1] ethdev: parsing multiple
> >> representor devargs string
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - On 1/21/2024 7:19 PM, Harman Kalra wrote:
> >>> Adding support for parsing multiple representor devargs strings
> >>> passed to a PCI BDF. There may be scenario where port representors
> >>> for various PFs or VFs under PFs are required and all these are
> >>> representor ports shall be backed by single pci device. In such case
> >>> port representors can be created using devargs string:
> >>> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> >>>
> >>
> >> This patch is to be able to parse multiple representor device
> >> argument, but I am concerned how it is used.
> >
> > In Marvell CNXK port representor solution all representors are backed
> > by a single PCI device (we call it as eswitch device). Eswitch device
> > is not DPDK ethdev device but an internal device with NIC capabilities
> > and is configured with as many no of Rxqs and Txqs as port representor
> required.
> > Hence each port representor will have dedicated RxQ/TxQ pair to
> > communicate with respective represented PF or VF.
> >
> 
> ack, thanks for clarification.
> 
> Just to double check, is the cnxk driver support of new syntax planned for this
> release?
> It helps if the RFC is out before merging this patch.
 
Yes, it is planned for this release. We have already pushed 2 versions and received
comments. V3 is ready with all V2 comments addressed, I will push by EOD.


> 
> >
> >>
> >> When there are multiple representor ports backed up by same device,
> >> can't it cause syncronization issues, like if two representor
> >> interfaces used for conflicting configurations. Or if datapath will
> >> be supported, what if two representator used simultaneously.
> >
> > As I mentioned above, each port representor will have dedicated RxQ
> > and TxQ, worker cores can poll on respective queues without any
> synchronization issue.
> > I hope I am able to explain the use case.
> >
> 
> ack
> 
> >>
> >>
> >
> >
> >
> >
> >>
> >> Meanwhile please check some comments below related to the parser code.
> >>
> >> <...>
> >>
> >>> @@ -459,9 +460,26 @@ eth_dev_devargs_tokenise(struct rte_kvargs
> >> *arglist, const char *str_in)
> >>>  			break;
> >>>
> >>>  		case 3: /* Parsing list */
> >>> -			if (*letter == ']')
> >>> -				state = 2;
> >>> -			else if (*letter == '\0')
> >>> +			if (*letter == ']') {
> >>> +				/* For devargs having singles lists move to
> >> state 2 once letter
> >>> +				 * becomes ']' so each can be considered as
> >> different pair key
> >>> +				 * value. But in nested lists case e.g. multiple
> >> representors
> >>> +				 * case i.e. [pf[0-3],pfvf[3,4-6]], complete
> >> nested list should
> >>> +				 * be considered as one pair value, hence
> >> checking if end of outer
> >>> +				 * list ']' is reached else stay on state 3.
> >>> +				 */
> >>> +				if ((strcmp("representor", pair->key) == 0)
> >> 	    &&
> >>> +				    (*(letter + 1) != '\0' && *(letter + 2) != '\0'
> >> &&
> >>> +				     *(letter + 3) != '\0')			    &&
> >>> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')
> >> ||
> >>> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
> >> ||
> >>> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
> >> ||
> >>> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3)))
> >> ||
> >>> +				     (*(letter + 2) == '[' && isdigit(*(letter +
> >> 3)))))
> >>>
> >>
> >> Above is hard to understand but there are some assumptions in the
> >> input, can we list supported syntax in the comment to make it more clear.
> >>
> >> For example following seems not support, can you please check if
> >> intentional:
> >> [vf0,vf1] // I am aware this can be put as vf[0,1] too [vf[0,1],3]
> >> [vf[0],vf[1]]
> >
> > Intention behind above change is just to detect if its nested list
> > i.e. multiple devargs (constituting lists as well) or a single devarg with a list.
> >
> > pf0vf[1-4] is a single list devarg example, while
> > [pf[0-3],pfvf[3,4-6]] is nested list example, where multiple devargs
> > pf[0-3] and pfvf[3,4-6] (which are also lists) are bind to a list of devargs.
> >
> > And as I mentioned in the comment, complete "nested list should be
> > considered as one pair value", hence entire list i.e. [vf[0,1],3]
> > [vf[0],vf[1]] will be one value of arglist and later
> > eth_dev_tokenise_representor_list() will tokenize and feed to
> rte_eth_devargs_parse_representor_ports().
> >
> > Whether any format is supported or not should be handled by
> > rte_eth_devargs_parse_representor_ports()
> >
> 
> Agree but this is something user facing, user should know the syntax to
> use it but some corner cases are not documented well and not trivial to
> grasp from above code.
> 
> What do you think about adding some unit tests for parser, it can be
> some pointer to the intention of what should be supported and what not,
> also increases our confidence to the code?

Thanks for the suggestion, I have updated devargs UT with valid/invalid devarg
patterns. Kindly review V5. I tried to cover most of the use case, please let me
know if any important case I missed.

Thanks
Harman


> 
> >
> >>
> >> I am not saying above should be supported, but syntax should be clear
> what
> >> is supported what is not.
> >>
> >>
> >> Also I can't check but is the redundant input verified, like:
> >> [vf[0-3],vf[3,4]]
> >
> > IMO, this should be handled by PMD, if any representor already created
> should
> > return an error.
> >
> 
> fair enough

Re: [PATCH v5 0/2] multiple representors in one device

[Ferruh Yigit]

On 2/1/2024 10:02 AM, Harman Kalra wrote:
> Following series adds support to enable creation of multiple representors
> under one base device. There may be scenarios where port representors for
> multiple PFs or VFs under PF are required and all these representor ports
> created under a single pci device. Marvell CNXK port representor solution
> is designed around this scenario where all representors are backed by a
> single switch device.
> 
> Earlier this change was implemented as part of the Marvell CNXK port
> representor series but after suggestions from Thomas we would like
> to propose these changes in common code.
> https://patches.dpdk.org/project/dpdk/patch/20231219174003.72901-25-hkalra@marvell.com/#166785
> 
> V5:
> - Added test cases to demonstrate valid and invalid cases
> - changed the tokenizing logic to address all valid cases
> 
> V4:
> - Used MT safe strtok_r in place of strtok
> - Reworded some comments
> 
> V3:
> - Fix duplicate representor devarg key handling logic
> 
> V2:
> - Updated the multiple representor devarg pattern to list
> i.e. representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> - Introduced size of array as third argument to rte_eth_devargs_parse()
> to avoid array corruption
> - Squashed separate document patch 
> 
> 
> Harman Kalra (2):
>   ethdev: parsing multiple representor devargs string
>   test/devargs: add eth devargs parse cases
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>


Squashed patches while merging,
Series applied to dpdk-next-net/main, thanks.