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@google.com>
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> Signed-off-by: Jeroen de Borst <jeroendb@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?

<...>

> +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.

> +     if (driver_info == NULL) {
> +             PMD_DRV_LOG(ERR,
> +                         "Could not alloc for driver compatibility");

Message can be misleading, adding 'verify' may help.

> +             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.

> +     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.

> + * 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.

> +     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

> + * 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