[1/2] ethdev: parsing multiple representor devargs string
Checks
Commit Message
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(-)
Comments
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 = ð_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
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 = ð_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
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]]
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
@@ -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,
ð_da);
- if (ret)
+ if (ret < 0)
return ret;
}
@@ -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,
ð_da);
- if (retval)
+ if (retval < 0)
return retval;
}
if (eth_da.nb_representor_ports > 0 &&
@@ -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,
ð_da);
- if (retval)
+ if (retval < 0)
return retval;
}
@@ -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, ð_da);
- if (ret)
+ if (ret < 0)
return ret;
ret = rte_eth_dev_pci_generic_probe(pci_dev,
@@ -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,
ð_da);
- if (retval)
+ if (retval < 0)
return retval;
} else
memset(ð_da, 0, sizeof(eth_da));
@@ -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;
@@ -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, ð_da);
- if (ret != 0) {
+ if (ret < 0) {
PMD_INIT_LOG(ERR, "devarg parse failed");
return -EINVAL;
}
@@ -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, ð_da);
- if (rc != 0) {
+ if (rc < 0) {
SFC_GENERIC_LOG(ERR,
"Failed to parse generic devargs '%s'",
args);
@@ -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 = ð_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);
@@ -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