From: Rushil Gupta <rushilg@google.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: qi.z.zhang@intel.com, jingjing.wu@intel.com,
junfeng.guo@intel.com, joshwash@google.com, dev@dpdk.org,
Jeroen de Borst <jeroendb@google.com>
Subject: Re: [v2] net/gve: check driver compatibility
Date: Thu, 18 May 2023 10:41:34 -0700 [thread overview]
Message-ID: <CANzqiF5-aDewEn0WXdXLontxs5D7pNu4t_fFM1sqnOBN9HaggA@mail.gmail.com> (raw)
In-Reply-To: <cda1f438-b59e-3f89-3f9e-b9272ce613cd@amd.com>
[-- Attachment #1: Type: text/plain, Size: 8116 bytes --]
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@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@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?
>
> 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.
[-- Attachment #2: Type: text/html, Size: 11398 bytes --]
next prev parent reply other threads:[~2023-05-18 17:41 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-14 5:14 [PATCH] net/gve: Check whether the driver is compatible with the device presented Rushil Gupta
2023-04-14 9:05 ` Guo, Junfeng
2023-04-14 15:40 ` Ferruh Yigit
2023-04-18 5:30 ` Guo, Junfeng
2023-04-26 21:37 ` Rushil Gupta
2023-05-04 15:06 ` Ferruh Yigit
2023-05-08 6:23 ` Rushil Gupta
2023-05-08 16:00 ` Ferruh Yigit
2023-05-08 19:22 ` Rushil Gupta
2023-05-08 19:15 ` [v2] net/gve: check driver compatibility Rushil Gupta
2023-05-17 16:58 ` Ferruh Yigit
2023-05-18 17:41 ` Rushil Gupta [this message]
2023-05-19 7:15 ` [v3] " Rushil Gupta
2023-05-19 7:23 ` [PATCH] " Rushil Gupta
2023-05-19 7:26 ` Rushil Gupta
2023-05-19 7:41 ` Rushil Gupta
2023-05-19 10:04 ` Ferruh Yigit
2023-05-19 16:15 ` Rushil Gupta
2023-05-19 19:54 ` [v4] " Rushil Gupta
2023-05-19 20:46 ` Rushil Gupta
2023-05-19 20:56 ` Stephen Hemminger
2023-05-20 16:44 ` Rushil Gupta
2023-05-22 8:52 ` Ferruh Yigit
2023-05-22 15:45 ` Rushil Gupta
2023-05-23 10:20 ` Ferruh Yigit
2023-05-23 10:21 ` Ferruh Yigit
2023-05-24 17:14 ` Rushil Gupta
2023-05-31 16:40 ` Rushil Gupta
2023-05-24 17:13 ` [v5] " Rushil Gupta
2023-05-31 17:04 ` Ferruh Yigit
2023-05-18 17:40 ` [v3] " Rushil Gupta
2023-05-08 19:14 ` [v2] " Rushil Gupta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANzqiF5-aDewEn0WXdXLontxs5D7pNu4t_fFM1sqnOBN9HaggA@mail.gmail.com \
--to=rushilg@google.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=jeroendb@google.com \
--cc=jingjing.wu@intel.com \
--cc=joshwash@google.com \
--cc=junfeng.guo@intel.com \
--cc=qi.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).