[PATCH net-next v3 0/2] gve: Handle alternate miss-completions

[PATCH net-next v3 0/2] gve: Handle alternate miss-completions

[Jeroen de Borst]

Some versions of the virtual NIC present miss-completions in
an alternative way. Let the diver handle these alternate completions
and announce this capability to the device.

Changed in v3:
- Rewording cover letter
- Added 'Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>'
Changes in v2:
- Changed the subject to include 'gve:'


Jeroen de Borst (2):
  gve: Adding a new AdminQ command to verify driver
  gve: Handle alternate miss completions

 drivers/net/ethernet/google/gve/gve.h         |  1 +
 drivers/net/ethernet/google/gve/gve_adminq.c  | 19 +++++++
 drivers/net/ethernet/google/gve/gve_adminq.h  | 51 ++++++++++++++++++
 .../net/ethernet/google/gve/gve_desc_dqo.h    |  5 ++
 drivers/net/ethernet/google/gve/gve_main.c    | 52 +++++++++++++++++++
 drivers/net/ethernet/google/gve/gve_tx_dqo.c  | 18 ++++---
 6 files changed, 140 insertions(+), 6 deletions(-)

-- 
2.38.1.431.g37b22c650d-goog

[PATCH net-next v3 1/2] gve: Adding a new AdminQ command to verify driver

[Jeroen de Borst]

Check whether the driver is compatible with the device
presented.

Signed-off-by: Jeroen de Borst <jeroendb@google.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/google/gve/gve.h        |  1 +
 drivers/net/ethernet/google/gve/gve_adminq.c | 19 +++++++
 drivers/net/ethernet/google/gve/gve_adminq.h | 49 ++++++++++++++++++
 drivers/net/ethernet/google/gve/gve_main.c   | 52 ++++++++++++++++++++
 4 files changed, 121 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 5655da9cd236..64eb0442c82f 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -563,6 +563,7 @@ struct gve_priv {
 	u32 adminq_report_stats_cnt;
 	u32 adminq_report_link_speed_cnt;
 	u32 adminq_get_ptype_map_cnt;
+	u32 adminq_verify_driver_compatibility_cnt;
 
 	/* Global stats */
 	u32 interface_up_cnt; /* count of times interface turned up since last reset */
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index f7621ab672b9..6a12b30a9f87 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -407,6 +407,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:
 		dev_err(&priv->pdev->dev, "unknown AQ command opcode %d\n", opcode);
 	}
@@ -878,6 +881,22 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
 	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_report_link_speed(struct gve_priv *priv)
 {
 	union gve_adminq_command gvnic_cmd;
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index 83c0b40cd2d9..b9ee8be73f96 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -24,6 +24,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 */
@@ -146,6 +147,49 @@ enum gve_sup_feature_mask {
 
 #define GVE_DEV_OPT_LEN_GQI_RAW_ADDRESSING 0x0
 
+#define GVE_VERSION_STR_LEN 128
+
+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_CAP4(a) BIT(((int)a) - 192)
+
+#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;	/* 0x01 = Linux */
+	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[GVE_VERSION_STR_LEN];
+	u8 os_version_str2[GVE_VERSION_STR_LEN];
+};
+
+struct gve_adminq_verify_driver_compatibility {
+	__be64 driver_info_len;
+	__be64 driver_info_addr;
+};
+
+static_assert(sizeof(struct gve_adminq_verify_driver_compatibility) == 16);
+
 struct gve_adminq_configure_device_resources {
 	__be64 counter_array;
 	__be64 irq_db_addr;
@@ -345,6 +389,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];
@@ -372,6 +418,9 @@ int gve_adminq_unregister_page_list(struct gve_priv *priv, u32 page_list_id);
 int gve_adminq_set_mtu(struct gve_priv *priv, u64 mtu);
 int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
 			    dma_addr_t stats_report_addr, u64 interval);
+int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
+					   u64 driver_info_len,
+					   dma_addr_t driver_info_addr);
 int gve_adminq_report_link_speed(struct gve_priv *priv);
 
 struct gve_ptype_lut;
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 5a229a01f49d..5b40f9c53196 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -12,6 +12,8 @@
 #include <linux/sched.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <linux/utsname.h>
+#include <linux/version.h>
 #include <net/sch_generic.h>
 #include "gve.h"
 #include "gve_dqo.h"
@@ -30,6 +32,49 @@
 const char gve_version_str[] = GVE_VERSION;
 static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
 
+static int gve_verify_driver_compatibility(struct gve_priv *priv)
+{
+	int err;
+	struct gve_driver_info *driver_info;
+	dma_addr_t driver_info_bus;
+
+	driver_info = dma_alloc_coherent(&priv->pdev->dev,
+					 sizeof(struct gve_driver_info),
+					 &driver_info_bus, GFP_KERNEL);
+	if (!driver_info)
+		return -ENOMEM;
+
+	*driver_info = (struct gve_driver_info) {
+		.os_type = 1, /* Linux */
+		.os_version_major = cpu_to_be32(LINUX_VERSION_MAJOR),
+		.os_version_minor = cpu_to_be32(LINUX_VERSION_SUBLEVEL),
+		.os_version_sub = cpu_to_be32(LINUX_VERSION_PATCHLEVEL),
+		.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),
+		},
+	};
+	strscpy(driver_info->os_version_str1, utsname()->release,
+		sizeof(driver_info->os_version_str1));
+	strscpy(driver_info->os_version_str2, utsname()->version,
+		sizeof(driver_info->os_version_str2));
+
+	err = gve_adminq_verify_driver_compatibility(priv,
+						     sizeof(struct gve_driver_info),
+						     driver_info_bus);
+
+	/* It's ok if the device doesn't support this */
+	if (err == -EOPNOTSUPP)
+		err = 0;
+
+	dma_free_coherent(&priv->pdev->dev,
+			  sizeof(struct gve_driver_info),
+			  driver_info, driver_info_bus);
+	return err;
+}
+
 static netdev_tx_t gve_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct gve_priv *priv = netdev_priv(dev);
@@ -1368,6 +1413,13 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
 		return err;
 	}
 
+	err = gve_verify_driver_compatibility(priv);
+	if (err) {
+		dev_err(&priv->pdev->dev,
+			"Could not verify driver compatibility: err=%d\n", err);
+		goto err;
+	}
+
 	if (skip_describe_device)
 		goto setup_device;
 
-- 
2.38.1.431.g37b22c650d-goog

[PATCH net-next v3 2/2] gve: Handle alternate miss completions

[Jeroen de Borst]

The virtual NIC has 2 ways of indicating a miss-path
completion. This handles the alternate.

Signed-off-by: Jeroen de Borst <jeroendb@google.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/net/ethernet/google/gve/gve_adminq.h   |  4 +++-
 drivers/net/ethernet/google/gve/gve_desc_dqo.h |  5 +++++
 drivers/net/ethernet/google/gve/gve_tx_dqo.c   | 18 ++++++++++++------
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
index b9ee8be73f96..cf29662e6ad1 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.h
+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
@@ -154,6 +154,7 @@ enum gve_driver_capbility {
 	gve_driver_capability_gqi_rda = 1,
 	gve_driver_capability_dqo_qpl = 2, /* reserved for future use */
 	gve_driver_capability_dqo_rda = 3,
+	gve_driver_capability_alt_miss_compl = 4,
 };
 
 #define GVE_CAP1(a) BIT((int)a)
@@ -164,7 +165,8 @@ enum gve_driver_capbility {
 #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))
+	 GVE_CAP1(gve_driver_capability_dqo_rda) | \
+	 GVE_CAP1(gve_driver_capability_alt_miss_compl))
 
 #define GVE_DRIVER_CAPABILITY_FLAGS2 0x0
 #define GVE_DRIVER_CAPABILITY_FLAGS3 0x0
diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
index e8fe9adef7f2..f79cd0591110 100644
--- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
+++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
@@ -176,6 +176,11 @@ static_assert(sizeof(struct gve_tx_compl_desc) == 8);
 #define GVE_COMPL_TYPE_DQO_MISS 0x1 /* Miss path completion */
 #define GVE_COMPL_TYPE_DQO_REINJECTION 0x3 /* Re-injection completion */
 
+/* The most significant bit in the completion tag can change the completion
+ * type from packet completion to miss path completion.
+ */
+#define GVE_ALT_MISS_COMPL_BIT BIT(15)
+
 /* Descriptor to post buffers to HW on buffer queue. */
 struct gve_rx_desc_dqo {
 	__le16 buf_id; /* ID returned in Rx completion descriptor */
diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index 588d64819ed5..762915c6063b 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -953,12 +953,18 @@ int gve_clean_tx_done_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
 			atomic_set_release(&tx->dqo_compl.hw_tx_head, tx_head);
 		} else if (type == GVE_COMPL_TYPE_DQO_PKT) {
 			u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
-
-			gve_handle_packet_completion(priv, tx, !!napi,
-						     compl_tag,
-						     &pkt_compl_bytes,
-						     &pkt_compl_pkts,
-						     /*is_reinjection=*/false);
+			if (compl_tag & GVE_ALT_MISS_COMPL_BIT) {
+				compl_tag &= ~GVE_ALT_MISS_COMPL_BIT;
+				gve_handle_miss_completion(priv, tx, compl_tag,
+							   &miss_compl_bytes,
+							   &miss_compl_pkts);
+			} else {
+				gve_handle_packet_completion(priv, tx, !!napi,
+							     compl_tag,
+							     &pkt_compl_bytes,
+							     &pkt_compl_pkts,
+							     /*is_reinjection=*/false);
+			}
 		} else if (type == GVE_COMPL_TYPE_DQO_MISS) {
 			u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
 
-- 
2.38.1.431.g37b22c650d-goog

Re: [PATCH net-next v3 2/2] gve: Handle alternate miss completions

[Saeed Mahameed]

On 14 Nov 15:35, Jeroen de Borst wrote:
>The virtual NIC has 2 ways of indicating a miss-path
>completion. This handles the alternate.
>
>Signed-off-by: Jeroen de Borst <jeroendb@google.com>
>Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>---
> drivers/net/ethernet/google/gve/gve_adminq.h   |  4 +++-
> drivers/net/ethernet/google/gve/gve_desc_dqo.h |  5 +++++
> drivers/net/ethernet/google/gve/gve_tx_dqo.c   | 18 ++++++++++++------
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
>index b9ee8be73f96..cf29662e6ad1 100644
>--- a/drivers/net/ethernet/google/gve/gve_adminq.h
>+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
>@@ -154,6 +154,7 @@ enum gve_driver_capbility {
> 	gve_driver_capability_gqi_rda = 1,
> 	gve_driver_capability_dqo_qpl = 2, /* reserved for future use */
> 	gve_driver_capability_dqo_rda = 3,
>+	gve_driver_capability_alt_miss_compl = 4,
> };
>
> #define GVE_CAP1(a) BIT((int)a)
>@@ -164,7 +165,8 @@ enum gve_driver_capbility {
> #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))
>+	 GVE_CAP1(gve_driver_capability_dqo_rda) | \
>+	 GVE_CAP1(gve_driver_capability_alt_miss_compl))
>
> #define GVE_DRIVER_CAPABILITY_FLAGS2 0x0
> #define GVE_DRIVER_CAPABILITY_FLAGS3 0x0
>diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
>index e8fe9adef7f2..f79cd0591110 100644
>--- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
>+++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
>@@ -176,6 +176,11 @@ static_assert(sizeof(struct gve_tx_compl_desc) == 8);
> #define GVE_COMPL_TYPE_DQO_MISS 0x1 /* Miss path completion */
> #define GVE_COMPL_TYPE_DQO_REINJECTION 0x3 /* Re-injection completion */
>
>+/* The most significant bit in the completion tag can change the completion
>+ * type from packet completion to miss path completion.
>+ */
>+#define GVE_ALT_MISS_COMPL_BIT BIT(15)
>+
> /* Descriptor to post buffers to HW on buffer queue. */
> struct gve_rx_desc_dqo {
> 	__le16 buf_id; /* ID returned in Rx completion descriptor */
>diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
>index 588d64819ed5..762915c6063b 100644
>--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
>+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
>@@ -953,12 +953,18 @@ int gve_clean_tx_done_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
> 			atomic_set_release(&tx->dqo_compl.hw_tx_head, tx_head);
> 		} else if (type == GVE_COMPL_TYPE_DQO_PKT) {
> 			u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
>-
>-			gve_handle_packet_completion(priv, tx, !!napi,
>-						     compl_tag,
>-						     &pkt_compl_bytes,
>-						     &pkt_compl_pkts,
>-						     /*is_reinjection=*/false);
>+			if (compl_tag & GVE_ALT_MISS_COMPL_BIT) {
>+				compl_tag &= ~GVE_ALT_MISS_COMPL_BIT;

nit: __test_and_clear_bit() and reduce to oneline. also you can drop the
braces in the if else statements once you squashed the two lines.

>+				gve_handle_miss_completion(priv, tx, compl_tag,
>+							   &miss_compl_bytes,
>+							   &miss_compl_pkts);
>+			} else {
>+				gve_handle_packet_completion(priv, tx, !!napi,
>+							     compl_tag,
>+							     &pkt_compl_bytes,
>+							     &pkt_compl_pkts,
>+							     /*is_reinjection=*/false);
>+			}
> 		} else if (type == GVE_COMPL_TYPE_DQO_MISS) {
> 			u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
>
>-- 
>2.38.1.431.g37b22c650d-goog
>

Re: [PATCH net-next v3 1/2] gve: Adding a new AdminQ command to verify driver

[Saeed Mahameed]

On 14 Nov 15:35, Jeroen de Borst wrote:
>Check whether the driver is compatible with the device
>presented.
>
>Signed-off-by: Jeroen de Borst <jeroendb@google.com>
>Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>---
> drivers/net/ethernet/google/gve/gve.h        |  1 +
> drivers/net/ethernet/google/gve/gve_adminq.c | 19 +++++++
> drivers/net/ethernet/google/gve/gve_adminq.h | 49 ++++++++++++++++++
> drivers/net/ethernet/google/gve/gve_main.c   | 52 ++++++++++++++++++++
> 4 files changed, 121 insertions(+)
>
>diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
>index 5655da9cd236..64eb0442c82f 100644
>--- a/drivers/net/ethernet/google/gve/gve.h
>+++ b/drivers/net/ethernet/google/gve/gve.h
>@@ -563,6 +563,7 @@ struct gve_priv {
> 	u32 adminq_report_stats_cnt;
> 	u32 adminq_report_link_speed_cnt;
> 	u32 adminq_get_ptype_map_cnt;
>+	u32 adminq_verify_driver_compatibility_cnt;
>
> 	/* Global stats */
> 	u32 interface_up_cnt; /* count of times interface turned up since last reset */
>diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
>index f7621ab672b9..6a12b30a9f87 100644
>--- a/drivers/net/ethernet/google/gve/gve_adminq.c
>+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
>@@ -407,6 +407,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:
> 		dev_err(&priv->pdev->dev, "unknown AQ command opcode %d\n", opcode);
> 	}
>@@ -878,6 +881,22 @@ int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
> 	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_report_link_speed(struct gve_priv *priv)
> {
> 	union gve_adminq_command gvnic_cmd;
>diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
>index 83c0b40cd2d9..b9ee8be73f96 100644
>--- a/drivers/net/ethernet/google/gve/gve_adminq.h
>+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
>@@ -24,6 +24,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 */
>@@ -146,6 +147,49 @@ enum gve_sup_feature_mask {
>
> #define GVE_DEV_OPT_LEN_GQI_RAW_ADDRESSING 0x0
>
>+#define GVE_VERSION_STR_LEN 128
>+
>+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_CAP4(a) BIT(((int)a) - 192)
>+
>+#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;	/* 0x01 = Linux */
>+	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[GVE_VERSION_STR_LEN];
>+	u8 os_version_str2[GVE_VERSION_STR_LEN];
>+};
>+
>+struct gve_adminq_verify_driver_compatibility {
>+	__be64 driver_info_len;
>+	__be64 driver_info_addr;
>+};
>+
>+static_assert(sizeof(struct gve_adminq_verify_driver_compatibility) == 16);
>+
> struct gve_adminq_configure_device_resources {
> 	__be64 counter_array;
> 	__be64 irq_db_addr;
>@@ -345,6 +389,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];
>@@ -372,6 +418,9 @@ int gve_adminq_unregister_page_list(struct gve_priv *priv, u32 page_list_id);
> int gve_adminq_set_mtu(struct gve_priv *priv, u64 mtu);
> int gve_adminq_report_stats(struct gve_priv *priv, u64 stats_report_len,
> 			    dma_addr_t stats_report_addr, u64 interval);
>+int gve_adminq_verify_driver_compatibility(struct gve_priv *priv,
>+					   u64 driver_info_len,
>+					   dma_addr_t driver_info_addr);
> int gve_adminq_report_link_speed(struct gve_priv *priv);
>
> struct gve_ptype_lut;
>diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
>index 5a229a01f49d..5b40f9c53196 100644
>--- a/drivers/net/ethernet/google/gve/gve_main.c
>+++ b/drivers/net/ethernet/google/gve/gve_main.c
>@@ -12,6 +12,8 @@
> #include <linux/sched.h>
> #include <linux/timer.h>
> #include <linux/workqueue.h>
>+#include <linux/utsname.h>
>+#include <linux/version.h>
> #include <net/sch_generic.h>
> #include "gve.h"
> #include "gve_dqo.h"
>@@ -30,6 +32,49 @@
> const char gve_version_str[] = GVE_VERSION;
> static const char gve_version_prefix[] = GVE_VERSION_PREFIX;
>
>+static int gve_verify_driver_compatibility(struct gve_priv *priv)
>+{
>+	int err;
>+	struct gve_driver_info *driver_info;
>+	dma_addr_t driver_info_bus;
>+
>+	driver_info = dma_alloc_coherent(&priv->pdev->dev,
>+					 sizeof(struct gve_driver_info),
>+					 &driver_info_bus, GFP_KERNEL);
>+	if (!driver_info)
>+		return -ENOMEM;
>+
>+	*driver_info = (struct gve_driver_info) {
>+		.os_type = 1, /* Linux */
>+		.os_version_major = cpu_to_be32(LINUX_VERSION_MAJOR),
>+		.os_version_minor = cpu_to_be32(LINUX_VERSION_SUBLEVEL),
>+		.os_version_sub = cpu_to_be32(LINUX_VERSION_PATCHLEVEL),
>+		.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),
>+		},
>+	};
>+	strscpy(driver_info->os_version_str1, utsname()->release,
>+		sizeof(driver_info->os_version_str1));
>+	strscpy(driver_info->os_version_str2, utsname()->version,
>+		sizeof(driver_info->os_version_str2));
>+
>+	err = gve_adminq_verify_driver_compatibility(priv,
>+						     sizeof(struct gve_driver_info),
>+						     driver_info_bus);
>+
>+	/* It's ok if the device doesn't support this */
>+	if (err == -EOPNOTSUPP)
>+		err = 0;
>+
I always find these exception interesting so i had to attempt and chase this
error code down to the source and couldn't find it. 
I think you meant -ENOTSUPP: which comes from gve_adminq_parse_err()

case GVE_ADMINQ_COMMAND_ERROR_UNIMPLEMENTED:
		return -ENOTSUPP;

because there isn't any path in the code that returns -EOPNOTSUPP;

but isn't there any capability that could tell if driver version
compatibility command is supported by device, prior to issuing the command,
so the code can be more explicit.

Re: [PATCH net-next v3 0/2] gve: Handle alternate miss-completions

[Saeed Mahameed]

On 14 Nov 15:35, Jeroen de Borst wrote:
>Some versions of the virtual NIC present miss-completions in
>an alternative way. Let the diver handle these alternate completions
>and announce this capability to the device.
>

nit: you missed to document the 1st "new AdminQ command" patch which has
some special logic to validate driver/os version compatibility with the
device and if it's related to this cover-letter title if at all.

>Changed in v3:
>- Rewording cover letter
>- Added 'Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>'
>Changes in v2:
>- Changed the subject to include 'gve:'
>
>
>Jeroen de Borst (2):
>  gve: Adding a new AdminQ command to verify driver
>  gve: Handle alternate miss completions
>
> drivers/net/ethernet/google/gve/gve.h         |  1 +
> drivers/net/ethernet/google/gve/gve_adminq.c  | 19 +++++++
> drivers/net/ethernet/google/gve/gve_adminq.h  | 51 ++++++++++++++++++
> .../net/ethernet/google/gve/gve_desc_dqo.h    |  5 ++
> drivers/net/ethernet/google/gve/gve_main.c    | 52 +++++++++++++++++++
> drivers/net/ethernet/google/gve/gve_tx_dqo.c  | 18 ++++---
> 6 files changed, 140 insertions(+), 6 deletions(-)
>
>-- 
>2.38.1.431.g37b22c650d-goog
>

Re: [PATCH net-next v3 2/2] gve: Handle alternate miss completions

[Jeroen de Borst]

Saeed,

Thanks for your review. I like the suggestion, but
__test_and_clear_bit is for unsigned longs, comptag is a short.

Jeroen


On Tue, Nov 15, 2022 at 9:17 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On 14 Nov 15:35, Jeroen de Borst wrote:
> >The virtual NIC has 2 ways of indicating a miss-path
> >completion. This handles the alternate.
> >
> >Signed-off-by: Jeroen de Borst <jeroendb@google.com>
> >Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >---
> > drivers/net/ethernet/google/gve/gve_adminq.h   |  4 +++-
> > drivers/net/ethernet/google/gve/gve_desc_dqo.h |  5 +++++
> > drivers/net/ethernet/google/gve/gve_tx_dqo.c   | 18 ++++++++++++------
> > 3 files changed, 20 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/google/gve/gve_adminq.h b/drivers/net/ethernet/google/gve/gve_adminq.h
> >index b9ee8be73f96..cf29662e6ad1 100644
> >--- a/drivers/net/ethernet/google/gve/gve_adminq.h
> >+++ b/drivers/net/ethernet/google/gve/gve_adminq.h
> >@@ -154,6 +154,7 @@ enum gve_driver_capbility {
> >       gve_driver_capability_gqi_rda = 1,
> >       gve_driver_capability_dqo_qpl = 2, /* reserved for future use */
> >       gve_driver_capability_dqo_rda = 3,
> >+      gve_driver_capability_alt_miss_compl = 4,
> > };
> >
> > #define GVE_CAP1(a) BIT((int)a)
> >@@ -164,7 +165,8 @@ enum gve_driver_capbility {
> > #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))
> >+       GVE_CAP1(gve_driver_capability_dqo_rda) | \
> >+       GVE_CAP1(gve_driver_capability_alt_miss_compl))
> >
> > #define GVE_DRIVER_CAPABILITY_FLAGS2 0x0
> > #define GVE_DRIVER_CAPABILITY_FLAGS3 0x0
> >diff --git a/drivers/net/ethernet/google/gve/gve_desc_dqo.h b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> >index e8fe9adef7f2..f79cd0591110 100644
> >--- a/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> >+++ b/drivers/net/ethernet/google/gve/gve_desc_dqo.h
> >@@ -176,6 +176,11 @@ static_assert(sizeof(struct gve_tx_compl_desc) == 8);
> > #define GVE_COMPL_TYPE_DQO_MISS 0x1 /* Miss path completion */
> > #define GVE_COMPL_TYPE_DQO_REINJECTION 0x3 /* Re-injection completion */
> >
> >+/* The most significant bit in the completion tag can change the completion
> >+ * type from packet completion to miss path completion.
> >+ */
> >+#define GVE_ALT_MISS_COMPL_BIT BIT(15)
> >+
> > /* Descriptor to post buffers to HW on buffer queue. */
> > struct gve_rx_desc_dqo {
> >       __le16 buf_id; /* ID returned in Rx completion descriptor */
> >diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> >index 588d64819ed5..762915c6063b 100644
> >--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> >+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> >@@ -953,12 +953,18 @@ int gve_clean_tx_done_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
> >                       atomic_set_release(&tx->dqo_compl.hw_tx_head, tx_head);
> >               } else if (type == GVE_COMPL_TYPE_DQO_PKT) {
> >                       u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
> >-
> >-                      gve_handle_packet_completion(priv, tx, !!napi,
> >-                                                   compl_tag,
> >-                                                   &pkt_compl_bytes,
> >-                                                   &pkt_compl_pkts,
> >-                                                   /*is_reinjection=*/false);
> >+                      if (compl_tag & GVE_ALT_MISS_COMPL_BIT) {
> >+                              compl_tag &= ~GVE_ALT_MISS_COMPL_BIT;
>
> nit: __test_and_clear_bit() and reduce to oneline. also you can drop the
> braces in the if else statements once you squashed the two lines.
>
> >+                              gve_handle_miss_completion(priv, tx, compl_tag,
> >+                                                         &miss_compl_bytes,
> >+                                                         &miss_compl_pkts);
> >+                      } else {
> >+                              gve_handle_packet_completion(priv, tx, !!napi,
> >+                                                           compl_tag,
> >+                                                           &pkt_compl_bytes,
> >+                                                           &pkt_compl_pkts,
> >+                                                           /*is_reinjection=*/false);
> >+                      }
> >               } else if (type == GVE_COMPL_TYPE_DQO_MISS) {
> >                       u16 compl_tag = le16_to_cpu(compl_desc->completion_tag);
> >
> >--
> >2.38.1.431.g37b22c650d-goog
> >

Re: [PATCH net-next v3 2/2] gve: Handle alternate miss completions

[Saeed Mahameed]

On 16 Nov 08:23, Jeroen de Borst wrote:
>Saeed,
>
>Thanks for your review. I like the suggestion, but
>__test_and_clear_bit is for unsigned longs, comptag is a short.
>

gcc will up-cast it for you with no problem.
Assuming it's unsigned short.. if not then consider changing it to unsigned
short, or do the casting yourself.

anyway this was just a minor suggestion.