<div dir="ltr"><div dir="ltr">Thanks for reviewing. Here is v3: <a href="http://patchwork.dpdk.org/project/dpdk/patch/20230518174005.1377467-1-rushilg@google.com/">http://patchwork.dpdk.org/project/dpdk/patch/20230518174005.1377467-1-rushilg@google.com/</a></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 17, 2023 at 9:59 AM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a>> > Signed-off-by: Joshua Washington <<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a>> > Signed-off-by: Junfeng Guo <<a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a>> > Signed-off-by: Jeroen de Borst <<a href="mailto:jeroendb@google.com" target="_blank">jeroendb@google.com</a>> > --- > 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? </blockquote><div>I will address this in the license patch 23.11. We will port all of our MIT licenses to BSD.</div><div><a href="http://patchwork.dpdk.org/project/dpdk/patch/20230411045908.844901-2-rushilg@google.com/">http://patchwork.dpdk.org/project/dpdk/patch/20230411045908.844901-2-rushilg@google.com/</a> </div><div>For now let's keep this change separate from license update/removal.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <...> > +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. </blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + if (driver_info == NULL) { > + PMD_DRV_LOG(ERR, > + "Could not alloc for driver compatibility"); Message can be misleading, adding 'verify' may help. </blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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. </blockquote><div>It is acceptable if 'os_version_str1' & 'os_version_str2' are not set.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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. </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + * 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. </blockquote><div>Great catch! Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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 </blockquote><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + * 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 </blockquote><div>Fixed. </div></div></div>