From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2C24D42B4B; Fri, 19 May 2023 18:15:44 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9011740E25; Fri, 19 May 2023 18:15:43 +0200 (CEST) Received: from mail-vs1-f42.google.com (mail-vs1-f42.google.com [209.85.217.42]) by mails.dpdk.org (Postfix) with ESMTP id A495540E09 for ; Fri, 19 May 2023 18:15:41 +0200 (CEST) Received: by mail-vs1-f42.google.com with SMTP id ada2fe7eead31-4301281573aso1071954137.3 for ; Fri, 19 May 2023 09:15:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1684512941; x=1687104941; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=uOH4Z3c+/NQ26/Gdfzg/BW6x7hS2wX4kWmPsKKr7C7Y=; b=feLpkFhpfJ2wGhT0gJzBrfNgEeHJbLhUFVzPC2ZXSPAM7Ev9IDuu/iQk0j5BJV457g fcnL6bwKRJpRpdF53dHuU31XHPhNdgJ3Qxlc0NGAqOT2mdb9+fQQ3RnWG0fLZ8xGgi9Z nzZkJYibgNLOjJM7chABMjOBw3GNnLDfTcZUaTl0z44Z2eQOhQSkdcCXCC/LbMlbm0px QxI8YsvVutnaMldn1YYc70RaGUSK5sB6TsxsLcIAS11GvjZqscNWkj7+Y5IEDCu10VsR ierC5zqI6K3Uj9WRyNZpvycubm3sIGh5GvN5wtsjyxrhWprhT4cl6PILXp1yBkfNUV5n iRFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684512941; x=1687104941; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=uOH4Z3c+/NQ26/Gdfzg/BW6x7hS2wX4kWmPsKKr7C7Y=; b=iFlhb9eGB3fDGX36XxxbJkt1HxofERVA9mh9dHIphzM15uxAVg5I35/BVwQ9n/tQGM n61ZF895JCqybPpTrQ6tsQRcN1ClPhZml8tJOlcfjpzRQnjSEOkBBobMRKmhJeYzYnOh z2DKvmXcpXNJIgIIFWO5/24JjrsVyQsjCtsmd+RyegeagICZdfCp+SCZx0f8OWmx4AMg ddIoX01pak2DcmbkaGSQLkO87z36k5ybNO0b8h2UXKPdzv8it/DT9tvWuygxcdz2n2up JUuzhc8RmYy1nMfA7pU46lE97e6sbmQmC6gdwpLNBGppql0pnl/gPKX+16GDCqhEApqe oF9A== X-Gm-Message-State: AC+VfDytNebfX4eTs4P0oodhj5AsKTgYh61QT+8DXLwz9tqTVwZ85XIW TX1rEOSLCNmF9e+ymu2Bb8DUQygTrqQ6QXy5Yyn6CA== X-Google-Smtp-Source: ACHHUZ4MR6uRJEOsGDeQRvqnpfSxXvuUhkAHZ590wIGaXWDdkkZ/qDUdjWkDjJNWoseHgPNpiQojs1hISq5MFizdARw= X-Received: by 2002:a05:6102:538:b0:436:4dda:ee63 with SMTP id m24-20020a056102053800b004364ddaee63mr1135515vsa.0.1684512940748; Fri, 19 May 2023 09:15:40 -0700 (PDT) MIME-Version: 1.0 References: <20230508191552.104540-1-rushilg@google.com> <20230519072600.1444309-1-rushilg@google.com> <7ed74e1f-11ed-7fc4-2ee5-1e646cbb3699@amd.com> In-Reply-To: <7ed74e1f-11ed-7fc4-2ee5-1e646cbb3699@amd.com> From: Rushil Gupta Date: Fri, 19 May 2023 09:15:29 -0700 Message-ID: Subject: Re: [PATCH] net/gve: check driver compatibility To: Ferruh Yigit Cc: qi.z.zhang@intel.com, jingjing.wu@intel.com, junfeng.guo@intel.com, joshwash@google.com, dev@dpdk.org, Jeroen de Borst Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org I agree. Other adminq commands like gve_adminq_report_link_speed use gve_alloc_dma_mem and gve_free_dma_mem (which use rte_memzone_reserve_aligned under the hood) so let's stick to that. On Fri, May 19, 2023 at 3:04=E2=80=AFAM Ferruh Yigit = wrote: > > On 5/19/2023 8:41 AM, Rushil Gupta wrote: > > Hi Ferruh > > I have updated the latest patch here: > > http://patchwork.dpdk.org/project/dpdk/patch/20230519072600.1444309-1-r= ushilg@google.com/ > > However, using calloc causes issue while executing verify-compatibility= command. > > > > sudo dpdk-testpmd -a 00:04.0 -l 0-32 -- --forward-mode=3Drxonly --txq= =3D16 > > --rxq=3D16 --nb-cores=3D16 --stats-period 5 > > ... > > EAL: Using IOMMU type 8 (No-IOMMU) > > EAL: Probe PCI driver: net_gve (1ae0:42) device: 0000:00:04.0 (socket -= 1) > > gve_adminq_parse_err(): AQ command failed with status -9 > > gve_init_priv(): Could not verify driver compatibility: err=3D-22 > > EAL: Releasing PCI mapped resource for 0000:00:04.0 > > ... > > EAL: Error - exiting with code: 1 > > > > > > This is probably because the adminq command is sharing memory to > > report driver-info to the gvnic device and that needs to be in dpdk > > memory. > > > > > > Hi Rushil, > > That is OK, I missed this requirement. > > Admin command passes pointers for device to process, what is the address > type requirement here? > Some other commands pass pysical address (iova address) via the command. > > Both 'calloc()' and 'rte_zmalloc()' returns (guest) virtual address, why > one doesn't work but other does? > > Initial version was passing 'rte_memzone' pointer, are you updating the > hyperviser side based on changes on dpdk side? > > > And perhaps better to switch to 'rte_memzone_reserve_aligned()' as done > initial version and pass (guest) pysical address via adminq, if that is > the requirement? > > > > > > On Fri, May 19, 2023 at 12:26=E2=80=AFAM 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 > >> Signed-off-by: Joshua Washington > >> Signed-off-by: Junfeng Guo > >> Signed-off-by: Jeroen de Borst > >> --- > >> 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 | 13 +++++++ > >> drivers/net/gve/gve_version.h | 24 ++++++++++++ > >> drivers/net/gve/meson.build | 5 ++- > >> 9 files changed, 179 insertions(+), 18 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 2b7cf7d99b..f7b297e759 100644 > >> --- a/drivers/net/gve/base/gve.h > >> +++ b/drivers/net/gve/base/gve.h > >> @@ -9,9 +9,6 @@ > >> #include "gve_desc.h" > >> #include "gve_desc_dqo.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 e963f910a0..41202725e6 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 *p= riv, > >> 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", opcod= e); > >> } > >> @@ -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 =3D cpu_to_be32(GVE_ADMINQ_VERIFY_DRIVER_COMPATIBIL= ITY); > >> + cmd.verify_driver_compatibility =3D (struct gve_adminq_verify_= driver_compatibility) { > >> + .driver_info_len =3D cpu_to_be64(driver_info_len), > >> + .driver_info_addr =3D 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..e30b184913 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 =3D 0xC, > >> GVE_ADMINQ_REPORT_LINK_SPEED =3D 0xD, > >> GVE_ADMINQ_GET_PTYPE_MAP =3D 0xE, > >> + GVE_ADMINQ_VERIFY_DRIVER_COMPATIBILITY =3D 0xF, > >> }; > >> > >> /* Admin queue status codes */ > >> @@ -145,6 +146,44 @@ enum gve_sup_feature_mask { > >> }; > >> > >> #define GVE_DEV_OPT_LEN_GQI_RAW_ADDRESSING 0x0 > >> +enum gve_driver_capbility { > >> + gve_driver_capability_gqi_qpl =3D 0, > >> + gve_driver_capability_gqi_rda =3D 1, > >> + gve_driver_capability_dqo_qpl =3D 2, /* reserved for future us= e */ > >> + gve_driver_capability_dqo_rda =3D 3, > >> +}; > >> + > >> +#define GVE_CAP1(a) BIT((int)a) > >> + > >> +#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 =3D 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); > >> + > >> > >> struct gve_adminq_configure_device_resources { > >> __be64 counter_array; > >> @@ -345,6 +384,8 @@ union gve_adminq_command { > >> struct gve_adminq_report_stats report_stats; > >> struct gve_adminq_report_link_speed report_lin= k_speed; > >> struct gve_adminq_get_ptype_map get_ptype_map; > >> + struct gve_adminq_verify_driver_compatibility > >> + verify_driver_compatibility; > >> }; > >> }; > >> u8 reserved[64]; > >> @@ -378,4 +419,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/g= ve_osdep.h > >> index abf3d379ae..5e8ae1eac6 100644 > >> --- a/drivers/net/gve/base/gve_osdep.h > >> +++ b/drivers/net/gve/base/gve_osdep.h > >> @@ -21,9 +21,14 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include "../gve_logs.h" > >> > >> +#ifdef __linux__ > >> +#include > >> +#endif > >> + > >> typedef uint8_t u8; > >> typedef uint16_t u16; > >> typedef uint32_t u32; > >> @@ -73,6 +78,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/u= nion > >> * 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 > >> @@ -160,4 +171,17 @@ gve_free_dma_mem(struct gve_dma_mem *mem) > >> mem->pa =3D 0; > >> } > >> > >> +static inline void > >> +populate_driver_version_strings(char *str1, char *str2) > >> +{ > >> + struct utsname uts; > >> + if (uname(&uts) >=3D 0) { > >> + /* release */ > >> + rte_strscpy(str1, uts.release, > >> + OS_VERSION_STRLEN); > >> + /* version */ > >> + rte_strscpy(str2, uts.version, > >> + OS_VERSION_STRLEN); > >> + } > >> +} > >> #endif /* _GVE_OSDEP_H_ */ > >> diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev= .c > >> index 1dcb3b3a01..71d131fb41 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[] =3D GVE_VERSION; > >> -static const char gve_version_prefix[] =3D 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 =3D gve_version_prefix; > >> - > >> - while (*c) { > >> - writeb(*c, driver_version_register); > >> - c++; > >> - } > >> - > >> - c =3D gve_version_str; > >> + const char *c =3D gve_version_string(); > >> while (*c) { > >> writeb(*c, driver_version_register); > >> c++; > >> @@ -245,6 +237,48 @@ gve_dev_close(struct rte_eth_dev *dev) > >> return err; > >> } > >> > >> +static int > >> +gve_verify_driver_compatibility(struct gve_priv *priv) > >> +{ > >> + struct gve_driver_info *driver_info; > >> + int err; > >> + > >> + driver_info =3D rte_zmalloc("driver info", > >> + sizeof(struct gve_driver_info), 0); > >> + if (driver_info =3D=3D NULL) { > >> + PMD_DRV_LOG(ERR, > >> + "Could not alloc for verify driver compati= bility"); > >> + return -ENOMEM; > >> + } > >> + *driver_info =3D (struct gve_driver_info) { > >> + .os_type =3D 5, /* DPDK */ > >> + .driver_major =3D GVE_VERSION_MAJOR, > >> + .driver_minor =3D GVE_VERSION_MINOR, > >> + .driver_sub =3D GVE_VERSION_SUB, > >> + .os_version_major =3D cpu_to_be32(DPDK_VERSION_MAJOR), > >> + .os_version_minor =3D cpu_to_be32(DPDK_VERSION_MINOR), > >> + .os_version_sub =3D cpu_to_be32(DPDK_VERSION_SUB), > >> + .driver_capability_flags =3D { > >> + 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_versio= n_str1, > >> + (char *)driver_info->os_version_str2); > >> + > >> + err =3D gve_adminq_verify_driver_compatibility(priv, > >> + sizeof(struct gve_driver_info), (dma_addr_t)driver_inf= o); > >> + /* It's ok if the device doesn't support this */ > >> + if (err =3D=3D -EOPNOTSUPP) > >> + err =3D 0; > >> + > >> + rte_free(driver_info); > >> + return err; > >> +} > >> + > >> static int > >> gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *de= v_info) > >> { > >> @@ -679,6 +713,11 @@ gve_init_priv(struct gve_priv *priv, bool skip_de= scribe_device) > >> PMD_DRV_LOG(ERR, "Failed to alloc admin queue: err=3D%= d", err); > >> return err; > >> } > >> + err =3D gve_verify_driver_compatibility(priv); > >> + if (err) { > >> + PMD_DRV_LOG(ERR, "Could not verify driver compatibilit= y: err=3D%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 cd62debd22..c9bcfa553c 100644 > >> --- a/drivers/net/gve/gve_ethdev.h > >> +++ b/drivers/net/gve/gve_ethdev.h > >> @@ -268,7 +268,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_versi= on.c > >> new file mode 100644 > >> index 0000000000..5fe34dc179 > >> --- /dev/null > >> +++ b/drivers/net/gve/gve_version.c > >> @@ -0,0 +1,13 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright (C) 2015-2023 Google, Inc. > >> + */ > >> +#include "gve_version.h" > >> + > >> +const char *gve_version_string(void) > >> +{ > >> + static char gve_version[20]; > >> + snprintf(gve_version, sizeof(gve_version), "%s%d.%d.%d", > >> + GVE_VERSION_PREFIX, GVE_VERSION_MAJOR, GVE_VERSION_MIN= OR, > >> + GVE_VERSION_SUB); > >> + return gve_version; > >> +} > >> diff --git a/drivers/net/gve/gve_version.h b/drivers/net/gve/gve_versi= on.h > >> new file mode 100644 > >> index 0000000000..4dd998dca1 > >> --- /dev/null > >> +++ b/drivers/net/gve/gve_version.h > >> @@ -0,0 +1,24 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright (C) 2015-2023 Google, Inc. > >> + */ > >> + > >> +#ifndef _GVE_VERSION_H_ > >> +#define _GVE_VERSION_H_ > >> + > >> +#include > >> + > >> +#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 c9d87903f9..61d195009c 100644 > >> --- a/drivers/net/gve/meson.build > >> +++ b/drivers/net/gve/meson.build > >> @@ -1,9 +1,9 @@ > >> # SPDX-License-Identifier: BSD-3-Clause > >> # Copyright(C) 2022 Intel Corporation > >> > >> -if is_windows > >> +if not is_linux > >> build =3D false > >> - reason =3D 'not supported on Windows' > >> + reason =3D 'only supported on Linux' > >> subdir_done() > >> endif > >> > >> @@ -14,5 +14,6 @@ sources =3D files( > >> 'gve_rx_dqo.c', > >> 'gve_tx_dqo.c', > >> 'gve_ethdev.c', > >> + 'gve_version.c', > >> ) > >> includes +=3D include_directories('base') > >> -- > >> 2.40.1.698.g37aff9b760-goog > >> >