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.yi...@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 <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 | 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') >
