[PATCH] net/gve: Check whether the driver is compatible with the device presented.
Rushil Gupta
rushilg at google.com
Mon May 8 08:23:22 CEST 2023
Thanks for reviewing Ferruh Yigit! I wanted to answer your queries
before sending the next patch.
Please find answers below:
On Thu, May 4, 2023 at 8:06 AM Ferruh Yigit <ferruh.yigit at amd.com> wrote:
>
> On 4/26/2023 10:37 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.
> >
>
> './devtools/check-git-log.sh' is giving some warnings, can you please
> check them?
>
> Contribution guide documentation may help to fix reported issues:
> https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-subject-line
>
I did run ./devtools/checkpatches.sh but not
./devtools/check-git-log.sh. I will run both before sending the next
patch.
> > Depends-on: series-27687
> >
> > 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 | 49 ++++++++++++++++++++++-
> > drivers/net/gve/base/gve_osdep.h | 36 +++++++++++++++++
> > drivers/net/gve/gve_ethdev.c | 65 +++++++++++++++++++++++++------
> > 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 | 1 +
> > 9 files changed, 198 insertions(+), 16 deletions(-)
> > create mode 100644 drivers/net/gve/gve_version.c
> > create mode 100644 drivers/net/gve/gve_version.h
> >
> > diff --git a/drivers/net/gve/base/gve.h b/drivers/net/gve/base/gve.h
> > index 2dc4507acb..89f9654a72 100644
> > --- a/drivers/net/gve/base/gve.h
> > +++ b/drivers/net/gve/base/gve.h
> > @@ -8,9 +8,6 @@
> >
> > #include "gve_desc.h"
> >
> > -#define GVE_VERSION "1.3.0"
> > -#define GVE_VERSION_PREFIX "GVE-"
> > -
> > #ifndef GOOGLE_VENDOR_ID
> > #define GOOGLE_VENDOR_ID 0x1ae0
> > #endif
> > diff --git a/drivers/net/gve/base/gve_adminq.c b/drivers/net/gve/base/gve_adminq.c
> > index e745b709b2..2e5099a5b0 100644
> > --- a/drivers/net/gve/base/gve_adminq.c
> > +++ b/drivers/net/gve/base/gve_adminq.c
> > @@ -401,6 +401,9 @@ static int gve_adminq_issue_cmd(struct gve_priv *priv,
> > case GVE_ADMINQ_GET_PTYPE_MAP:
> > priv->adminq_get_ptype_map_cnt++;
> > break;
> > + case GVE_ADMINQ_VERIFY_DRIVER_COMPATIBILITY:
> > + priv->adminq_verify_driver_compatibility_cnt++;
> > + break;
> > default:
> > PMD_DRV_LOG(ERR, "unknown AQ command opcode %d", opcode);
> > }
> > @@ -465,6 +468,22 @@ int gve_adminq_configure_device_resources(struct gve_priv *priv,
> > return gve_adminq_execute_cmd(priv, &cmd);
> > }
> >
> > +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> > + u64 driver_info_len,
> > + dma_addr_t driver_info_addr)
> > +{
> > + union gve_adminq_command cmd;
> > +
> > + memset(&cmd, 0, sizeof(cmd));
> > + cmd.opcode = cpu_to_be32(GVE_ADMINQ_VERIFY_DRIVER_COMPATIBILITY);
> > + cmd.verify_driver_compatibility = (struct gve_adminq_verify_driver_compatibility) {
> > + .driver_info_len = cpu_to_be64(driver_info_len),
> > + .driver_info_addr = cpu_to_be64(driver_info_addr),
> > + };
> > +
> > + return gve_adminq_execute_cmd(priv, &cmd);
> > +}
> > +
> > int gve_adminq_deconfigure_device_resources(struct gve_priv *priv)
> > {
> > union gve_adminq_command cmd;
> > diff --git a/drivers/net/gve/base/gve_adminq.h b/drivers/net/gve/base/gve_adminq.h
> > index 05550119de..edac32f031 100644
> > --- a/drivers/net/gve/base/gve_adminq.h
> > +++ b/drivers/net/gve/base/gve_adminq.h
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: MIT
> > * Google Virtual Ethernet (gve) driver
> > - * Copyright (C) 2015-2022 Google, Inc.
> > + * Copyright (C) 2015-2023 Google, Inc.
> > */
> >
> > #ifndef _GVE_ADMINQ_H
> > @@ -23,6 +23,7 @@ enum gve_adminq_opcodes {
> > GVE_ADMINQ_REPORT_STATS = 0xC,
> > GVE_ADMINQ_REPORT_LINK_SPEED = 0xD,
> > GVE_ADMINQ_GET_PTYPE_MAP = 0xE,
> > + GVE_ADMINQ_VERIFY_DRIVER_COMPATIBILITY = 0xF,
> > };
> >
> > /* Admin queue status codes */
> > @@ -145,6 +146,47 @@ enum gve_sup_feature_mask {
> > };
> >
> > #define GVE_DEV_OPT_LEN_GQI_RAW_ADDRESSING 0x0
> > +enum gve_driver_capbility {
> > + gve_driver_capability_gqi_qpl = 0,
> > + gve_driver_capability_gqi_rda = 1,
> > + gve_driver_capability_dqo_qpl = 2, /* reserved for future use */
> > + gve_driver_capability_dqo_rda = 3,
> > +};
> > +
> > +#define GVE_CAP1(a) BIT((int)a)
> > +#define GVE_CAP2(a) BIT(((int)a) - 64)
> > +#define GVE_CAP3(a) BIT(((int)a) - 128)
> > +#define GVE_CAP3(a) BIT(((int)a) - 192)
>
> GVE_CAP2, GVE_CAP3 & GVE_CAP4 seems not used, do we need to add them now?
> And I guess '(((int)a) - 64)' usage is to unset some bits, will it be
> better to '(a & ~(1 << 6))' ?
Sure; I will remove GVE_CAP2, GVE_CAP3 & GVE_CAP4.
>
> > +
> > +#define GVE_DRIVER_CAPABILITY_FLAGS1 \
> > + (GVE_CAP1(gve_driver_capability_gqi_qpl) | \
> > + GVE_CAP1(gve_driver_capability_gqi_rda) | \
> > + GVE_CAP1(gve_driver_capability_dqo_rda))
> > +
> > +#define GVE_DRIVER_CAPABILITY_FLAGS2 0x0
> > +#define GVE_DRIVER_CAPABILITY_FLAGS3 0x0
> > +#define GVE_DRIVER_CAPABILITY_FLAGS4 0x0
> > +
> > +struct gve_driver_info {
> > + u8 os_type; /* 0x05 = DPDK */
> > + u8 driver_major;
> > + u8 driver_minor;
> > + u8 driver_sub;
> > + __be32 os_version_major;
> > + __be32 os_version_minor;
> > + __be32 os_version_sub;
> > + __be64 driver_capability_flags[4];
> > + u8 os_version_str1[OS_VERSION_STRLEN];
> > + u8 os_version_str2[OS_VERSION_STRLEN];
> > +};
> > +
> > +struct gve_adminq_verify_driver_compatibility {
> > + __be64 driver_info_len;
> > + __be64 driver_info_addr;
> > +};
> > +
> > +GVE_CHECK_STRUCT_LEN(16, gve_adminq_verify_driver_compatibility);
> > +
>
> It looks like we need a static assert helper for DPDK, I will try to
> send something.
>
> >
> > struct gve_adminq_configure_device_resources {
> > __be64 counter_array;
> > @@ -345,6 +387,8 @@ union gve_adminq_command {
> > struct gve_adminq_report_stats report_stats;
> > struct gve_adminq_report_link_speed report_link_speed;
> > struct gve_adminq_get_ptype_map get_ptype_map;
> > + struct gve_adminq_verify_driver_compatibility
> > + verify_driver_compatibility;
> > };
> > };
> > u8 reserved[64];
> > @@ -378,4 +422,7 @@ struct gve_ptype_lut;
> > int gve_adminq_get_ptype_map_dqo(struct gve_priv *priv,
> > struct gve_ptype_lut *ptype_lut);
> >
> > +int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
> > + u64 driver_info_len,
> > + dma_addr_t driver_info_addr);
> > #endif /* _GVE_ADMINQ_H */
> > diff --git a/drivers/net/gve/base/gve_osdep.h b/drivers/net/gve/base/gve_osdep.h
> > index 7cb73002f4..a5efa8543e 100644
> > --- a/drivers/net/gve/base/gve_osdep.h
> > +++ b/drivers/net/gve/base/gve_osdep.h
> > @@ -21,9 +21,14 @@
> > #include <rte_malloc.h>
> > #include <rte_memcpy.h>
> > #include <rte_memzone.h>
> > +#include <rte_version.h>
> >
> > #include "../gve_logs.h"
> >
> > +#ifdef __linux__
> > +#include <sys/utsname.h>
> > +#endif
> > +
> > typedef uint8_t u8;
> > typedef uint16_t u16;
> > typedef uint32_t u32;
> > @@ -69,6 +74,12 @@ typedef rte_iova_t dma_addr_t;
> >
> > #define msleep(ms) rte_delay_ms(ms)
> >
> > +#define OS_VERSION_STRLEN 128
> > +struct os_version_string {
> > + char os_version_str1[OS_VERSION_STRLEN];
> > + char os_version_str2[OS_VERSION_STRLEN];
> > +};
> > +
> > /* These macros are used to generate compilation errors if a struct/union
> > * is not exactly the correct length. It gives a divide by zero error if
> > * the struct/union is not of the correct size, otherwise it creates an
> > @@ -79,6 +90,12 @@ typedef rte_iova_t dma_addr_t;
> > #define GVE_CHECK_UNION_LEN(n, X) enum gve_static_asset_enum_##X \
> > { gve_static_assert_##X = (n) / ((sizeof(union X) == (n)) ? 1 : 0) }
> >
> > +#ifndef LINUX_VERSION_MAJOR
> > +#define LINUX_VERSION_MAJOR (((LINUX_VERSION_CODE) >> 16) & 0xff)
> > +#define LINUX_VERSION_SUBLEVEL (((LINUX_VERSION_CODE) >> 8) & 0xff)
> > +#define LINUX_VERSION_PATCHLEVEL ((LINUX_VERSION_CODE) & 0xff)
> > +#endif
> > +
>
> Do we need Linux version macros in DPDK? They are not used at all.
You are right. Will remove.
>
> > static __rte_always_inline u8
> > readb(volatile void *addr)
> > {
> > @@ -156,4 +173,23 @@ gve_free_dma_mem(struct gve_dma_mem *mem)
> > mem->pa = 0;
> > }
> >
> > +static inline void
> > +populate_driver_version_strings(struct os_version_string *os_version_str)
> > +{
> > +#ifdef __linux__
> > + struct utsname uts;
> > + if (uname(&uts) >= 0) {
> > + /* release */
> > + rte_strscpy(os_version_str->os_version_str1, uts.release,
> > + sizeof(os_version_str->os_version_str1));
> > + /* version */
> > + rte_strscpy(os_version_str->os_version_str2, uts.version,
> > + sizeof(os_version_str->os_version_str2));
> > + }
>
> Since OS type is reported as DPDK, do we need underlying Linux release
> information to the FW?
>
> If not we can remove #ifdef and 'uname()' to reduce platform specific code.
Yes it is a GCE business requirement (to look at DPDK usage by linux
kernel version).
>
> > +#else
> > + /* gVNIC is currently not supported on OS like FreeBSD */
>
> meson file only disable for Windows, for FreeBSD driver is compiled.
> If driver is only supported on Linux, can you change the meson file to
> reflec this?
>
Ok will do.
> > + RTE_SET_USED(os_version_str);
> > +#endif
> > +}
> > +
> > #endif /* _GVE_OSDEP_H_ */
> > diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
> > index cf28a4a3b7..1d6e134fff 100644
> > --- a/drivers/net/gve/gve_ethdev.c
> > +++ b/drivers/net/gve/gve_ethdev.c
> > @@ -5,21 +5,13 @@
> > #include "gve_ethdev.h"
> > #include "base/gve_adminq.h"
> > #include "base/gve_register.h"
> > -
> > -const char gve_version_str[] = GVE_VERSION;
> > -static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
> > +#include "base/gve_osdep.h"
> > +#include "gve_version.h"
> >
> > static void
> > gve_write_version(uint8_t *driver_version_register)
> > {
> > - const char *c = gve_version_prefix;
> > -
> > - while (*c) {
> > - writeb(*c, driver_version_register);
> > - c++;
> > - }
> > -
> > - c = gve_version_str;
> > + const char *c = gve_version_string();
> > while (*c) {
> > writeb(*c, driver_version_register);
> > c++;
> > @@ -265,6 +257,52 @@ gve_dev_close(struct rte_eth_dev *dev)
> > return err;
> > }
> >
> > +static int
> > +gve_verify_driver_compatibility(struct gve_priv *priv)
> > +{
> > + const struct rte_memzone *driver_info_bus;
>
> What does '_bus' indicates in the variable name?
This seems like a typo. Will correct.
>
> > + struct gve_driver_info *driver_info;
> > + int err;
> > +
> > + driver_info_bus = rte_memzone_reserve_aligned("verify_driver_compatibility",
> > + sizeof(struct gve_driver_info),
> > + rte_socket_id(),
> > + RTE_MEMZONE_IOVA_CONTIG, PAGE_SIZE);
>
> There is no benefit to use DPDK memory APIs for this usage, a 'malloc()'
> will do the work, and maybe better as compilers/static analyzers
> recognizes it more.
Makes sense.
>
> > + if (driver_info_bus == NULL) {
> > + PMD_DRV_LOG(ERR,
> > + "Could not alloc memzone for driver compatibility");
> > + return -ENOMEM;
> > + }
> > + driver_info = (struct gve_driver_info *)driver_info_bus->addr;
> > + *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((struct os_version_string *)&driver_info->os_version_str1);
> > +
>
> If you pass two strings as parameter to
> 'populate_driver_version_strings()' can you get rid of interim type
> "struct os_version_string" and above casting?
Ok.
>
> > + err = gve_adminq_verify_driver_compatibility(priv,
> > + sizeof(struct gve_driver_info), (dma_addr_t)driver_info_bus);
> > +
> > + /* It's ok if the device doesn't support this */
> > + if (err == -EOPNOTSUPP)
> > + err = 0;
>
> Is this error saying DPDK version is not supported?
> Where a change should go to be able to identify and verify DPDK platform
> in GVE? Does it require continious update as DPDK get updated?
>
This means vNIC doesn't support verify and log adminq information
(because of older hypervisor).
Does it require continuous update : No. This is to make sure we return
proper error code for older vNIC.
> > +
> > + rte_memzone_free(driver_info_bus);
> > + return err;
> > +}
> > +
> > static int
> > gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> > {
> > @@ -672,6 +710,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..7eaa689d90
> > --- /dev/null
> > +++ b/drivers/net/gve/gve_version.c
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: MIT
> > + * Google Virtual Ethernet (gve) driver
> > + * Copyright (C) 2015-2023 Google, Inc.
> > + */
>
> This is not base code, but DPDK layer, license should be BSD-3.
>
Ok
> > +#include "gve_version.h"
> > +
> > +const char *gve_version_string(void)
> > +{
> > + static char gve_version[10];
> > + snprintf(gve_version, sizeof(gve_version), "%s%d.%d.%d",
> > + GVE_VERSION_PREFIX, GVE_VERSION_MAJOR, GVE_VERSION_MINOR,
> > + GVE_VERSION_SUB);
> > + 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..9af6a00568
> > --- /dev/null
> > +++ b/drivers/net/gve/gve_version.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: MIT
> > + * Google Virtual Ethernet (gve) driver
> > + * Copyright (C) 2015-2023 Google, Inc.
> > + */
> > +
>
> This is not base code, but DPDK layer, license should be BSD-3.
>
Ok
> > +#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
> > +
>
> Is this GVE base version, or DPDK gve version?
> Previous version information was "GVE-1.3.0", now it is becoming
> "DPDK-1.0.0",
> is this breaking the version link with GVE base version and creating a
> new versioning scheme for DPDK GVE driver?
>
> Btw, documentation still has "v1.3.0. GVE base code", should it be
> updated as well?
>
DPDK-1.0.0 is the DPDK driver version. GVE driver versioning only
applies for linux kernel Gvnic driver.
Similarly Windows Gvnic driver has different versioning system.
>
> > +#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
> > +
>
> RTE_VER_RELEASE is 1 (-rc1), 2 (-rc2), etc... And if it is not release
> candidate it is always 99, so it may not be very usefull for the
> official releases and changes frequently for the development tree, are
> you sure to use this value?
>
It is ok for our usecase.
>
> > +
> > +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..a37c4bafa2 100644
> > --- a/drivers/net/gve/meson.build
> > +++ b/drivers/net/gve/meson.build
> > @@ -12,5 +12,6 @@ sources = files(
> > 'gve_rx.c',
> > 'gve_tx.c',
> > 'gve_ethdev.c',
> > + 'gve_version.c',
> > )
> > includes += include_directories('base')
>
More information about the dev
mailing list