Thanks for reviewing. Here is v3: http://patchwork.dpdk.org/project/dpdk/patch/20230518174005.1377467-1-rush...@google.com/
On Wed, May 17, 2023 at 9:59 AM Ferruh Yigit <ferruh.yi...@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 <rush...@google.com> > > Signed-off-by: Joshua Washington <joshw...@google.com> > > Signed-off-by: Junfeng Guo <junfeng....@intel.com> > > Signed-off-by: Jeroen de Borst <jeroe...@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-rush...@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.
