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 = ð_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]
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 = ð_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]
@@ -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.
@@ -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
@@ -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,
- ð_da);
- if (ret)
+ ð_da, 1);
+ if (ret < 0)
return ret;
}
@@ -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,
- ð_da);
- if (retval)
+ ð_da, 1);
+ if (retval < 0)
return retval;
}
if (eth_da.nb_representor_ports > 0 &&
@@ -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,
- ð_da);
- if (retval)
+ ð_da, 1);
+ if (retval < 0)
return retval;
}
@@ -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, ð_da);
- if (ret)
+ ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, ð_da, 1);
+ if (ret < 0)
return ret;
ret = rte_eth_dev_pci_generic_probe(pci_dev,
@@ -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,
- ð_da);
- if (retval)
+ ð_da, 1);
+ if (retval < 0)
return retval;
} else
memset(ð_da, 0, sizeof(eth_da));
@@ -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;
@@ -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, ð_da);
- if (ret != 0) {
+ ret = rte_eth_devargs_parse(pci_dev->device.devargs->args, ð_da, 1);
+ if (ret < 0) {
PMD_INIT_LOG(ERR, "devarg parse failed");
return -EINVAL;
}
@@ -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, ð_da);
- if (rc != 0) {
+ rc = rte_eth_devargs_parse(args, ð_da, 1);
+ if (rc < 0) {
SFC_GENERIC_LOG(ERR,
"Failed to parse generic devargs '%s'",
args);
@@ -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 = ð_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 = ð_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);
@@ -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);