[v2] net/gve: check driver compatibility

[Rushil Gupta]

Thanks for reviewing. Here is v3:
http://patchwork.dpdk.org/project/dpdk/patch/20230518174005.1377467-1-rushilg@google.com/

On Wed, May 17, 2023 at 9:59 AM Ferruh Yigit <ferruh.yigit at amd.com> wrote:

> On 5/8/2023 8:15 PM, Rushil Gupta wrote:
> > Change gve_driver_info fields to report DPDK as OS type and DPDK RTE
> > version as OS version, reserving driver_version fields for GVE driver
> > version based on features.
> >
> > Signed-off-by: Rushil Gupta <rushilg at google.com>
> > Signed-off-by: Joshua Washington <joshwash at google.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
> > Signed-off-by: Jeroen de Borst <jeroendb at google.com>
> > ---
> >  drivers/net/gve/base/gve.h        |  3 --
> >  drivers/net/gve/base/gve_adminq.c | 19 ++++++++++
> >  drivers/net/gve/base/gve_adminq.h | 46 ++++++++++++++++++++++-
> >  drivers/net/gve/base/gve_osdep.h  | 24 ++++++++++++
> >  drivers/net/gve/gve_ethdev.c      | 61 +++++++++++++++++++++++++------
> >  drivers/net/gve/gve_ethdev.h      |  2 +-
> >  drivers/net/gve/gve_version.c     | 14 +++++++
> >  drivers/net/gve/gve_version.h     | 25 +++++++++++++
> >  drivers/net/gve/meson.build       |  7 ++++
> >  9 files changed, 185 insertions(+), 16 deletions(-)
> >  create mode 100644 drivers/net/gve/gve_version.c
> >  create mode 100644 drivers/net/gve/gve_version.h
> >
>
> 'doc/guides/nics/gve.rst' has following reference to Linux kernel version:
>
> "
> The base code is under MIT license and based on GVE kernel driver v1.3.0.
> "
>
> Previously you mentioned that Linux kernel code has nothing to do with
> DPDK version, should above note removed or updated?
>
> I will address this in the license patch 23.11. We will port all of our
MIT licenses to BSD.
http://patchwork.dpdk.org/project/dpdk/patch/20230411045908.844901-2-rushilg@google.com/
For now let's keep this change separate from license update/removal.

> <...>
>
> > +static int
> > +gve_verify_driver_compatibility(struct gve_priv *priv)
> > +{
> > +     struct gve_driver_info *driver_info;
> > +     int err;
> > +
> > +     driver_info = rte_zmalloc("driver info", sizeof(struct
> gve_driver_info), 0);
>
> Can use calloc(), no need to use DPDK memory APIs.
> This memory is not for datapath, and only used for short time.
>
Done

>
> > +     if (driver_info == NULL) {
> > +             PMD_DRV_LOG(ERR,
> > +                         "Could not alloc for driver compatibility");
>
> Message can be misleading, adding 'verify' may help.
>
Done

>
> > +             return -ENOMEM;
> > +     }
> > +     *driver_info = (struct gve_driver_info) {
> > +             .os_type = 5, /* DPDK */
> > +             .driver_major = GVE_VERSION_MAJOR,
> > +             .driver_minor = GVE_VERSION_MINOR,
> > +             .driver_sub = GVE_VERSION_SUB,
> > +             .os_version_major = cpu_to_be32(DPDK_VERSION_MAJOR),
> > +             .os_version_minor = cpu_to_be32(DPDK_VERSION_MINOR),
> > +             .os_version_sub = cpu_to_be32(DPDK_VERSION_SUB),
> > +             .driver_capability_flags = {
> > +                     cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS1),
> > +                     cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS2),
> > +                     cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS3),
> > +                     cpu_to_be64(GVE_DRIVER_CAPABILITY_FLAGS4),
> > +             },
> > +     };
> > +
> > +     populate_driver_version_strings((char
> *)driver_info->os_version_str1,
> > +                     (char *)driver_info->os_version_str2);
> > +
>
> Is it accepted by adminq_verify if 'os_version_str1' & 'os_version_str2'
> is not set? If not 'populate_driver_version_strings()' should return
> success/error value.
>
It is acceptable if  'os_version_str1' & 'os_version_str2' are not set.

>
> > +     err = gve_adminq_verify_driver_compatibility(priv,
> > +             sizeof(struct gve_driver_info), (dma_addr_t)driver_info);
> > +
> > +     /* It's ok if the device doesn't support this */
> > +     if (err == -EOPNOTSUPP)
> > +             err = 0;
> > +
> > +     rte_free(driver_info);
> > +     return err;
> > +}
> > +
> >  static int
> >  gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info)
> >  {
> > @@ -672,6 +706,11 @@ gve_init_priv(struct gve_priv *priv, bool
> skip_describe_device)
> >               PMD_DRV_LOG(ERR, "Failed to alloc admin queue: err=%d",
> err);
> >               return err;
> >       }
> > +     err = gve_verify_driver_compatibility(priv);
> > +     if (err) {
> > +             PMD_DRV_LOG(ERR, "Could not verify driver compatibility:
> err=%d", err);
> > +             goto free_adminq;
> > +     }
> >
> >       if (skip_describe_device)
> >               goto setup_device;
> > diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
> > index 42a02cf5d4..23ccff37d3 100644
> > --- a/drivers/net/gve/gve_ethdev.h
> > +++ b/drivers/net/gve/gve_ethdev.h
> > @@ -222,7 +222,7 @@ struct gve_priv {
> >       uint32_t adminq_report_stats_cnt;
> >       uint32_t adminq_report_link_speed_cnt;
> >       uint32_t adminq_get_ptype_map_cnt;
> > -
> > +     uint32_t adminq_verify_driver_compatibility_cnt;
> >       volatile uint32_t state_flags;
> >
> >       /* Gvnic device link speed from hypervisor. */
> > diff --git a/drivers/net/gve/gve_version.c
> b/drivers/net/gve/gve_version.c
> > new file mode 100644
> > index 0000000000..f02b4f696f
> > --- /dev/null
> > +++ b/drivers/net/gve/gve_version.c
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Google Virtual Ethernet (gve) driver
>
> Is above line added intentionally?
> Normally SPDX tag only contains license and copyright information,
> please check 'gve_ethdev.c' for sample tag.
>
Fixed.

>
> > + * Copyright (C) 2015-2023 Google, Inc.
> > + */
> > +#include "gve_version.h"
> > +
> > +const char *gve_version_string(void)
> > +{
> > +     static char gve_version[10];
>
> DPDK-1.0.0 => 10 chars, it doesn't left room for terminating NULL, but
> at least 'gve_write_version()' seems expecting to have one.
>
Great catch! Fixed.

>
> > +     snprintf(gve_version, sizeof(gve_version), "%s%d.%d.%d",
> > +             GVE_VERSION_PREFIX, GVE_VERSION_MAJOR, GVE_VERSION_MINOR,
> > +             GVE_VERSION_SUB);
>
> 'snprintf()' adds terminating NULL, truncating version string, need more
> space in the 'gve_version' array.
>
> > +     return gve_version;
> > +}
> > diff --git a/drivers/net/gve/gve_version.h
> b/drivers/net/gve/gve_version.h
> > new file mode 100644
> > index 0000000000..eca717d64b
> > --- /dev/null
> > +++ b/drivers/net/gve/gve_version.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Google Virtual Ethernet (gve) driver
>
> Ditto
>
Fixed.

>
> > + * Copyright (C) 2015-2023 Google, Inc.
> > + */
> > +
> > +#ifndef _GVE_VERSION_H_
> > +#define _GVE_VERSION_H_
> > +
> > +#include <rte_version.h>
> > +
> > +#define GVE_VERSION_PREFIX "DPDK-"
> > +#define GVE_VERSION_MAJOR 1
> > +#define GVE_VERSION_MINOR 0
> > +#define GVE_VERSION_SUB 0
> > +
> > +#define DPDK_VERSION_MAJOR (100 * RTE_VER_YEAR + RTE_VER_MONTH)
> > +#define DPDK_VERSION_MINOR RTE_VER_MINOR
> > +#define DPDK_VERSION_SUB RTE_VER_RELEASE
> > +
> > +
> > +const char *
> > +gve_version_string(void);
> > +
> > +
> > +#endif /* GVE_VERSION_H */
> > diff --git a/drivers/net/gve/meson.build b/drivers/net/gve/meson.build
> > index af0010c01c..62ab02d0dc 100644
> > --- a/drivers/net/gve/meson.build
> > +++ b/drivers/net/gve/meson.build
> > @@ -7,10 +7,17 @@ if is_windows
> >      subdir_done()
> >  endif
> >
> > +if is_freebsd
> > +    build = false
> > +    reason = 'not supported on FreeBSD'
> > +    subdir_done()
> > +endif
> > +
>
> instead of both 'is_windows' & 'is_freebsd' checks,
> can check as "not is_linux":
>
> if not is_linux
>     build = false
>     reason = 'only supported on Linux'
> endif
>
Fixed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20230518/d473e085/attachment-0001.htm>