mi: Distinguish MI status from NVMe (CDW3) status

We curerently have some overloading in the status values returned from
the nvme_* API, as the NVMe CDW3 values overlap with the
recently-introduced NVMe-MI response header status.

This change introduces a new encoding for the return values of MI
functions, where we use a set of bits in the return value to encode
whether the value is either a MI status value or a NVMe status value. We
leave room for future expansion too, by defining three bits of possible
type values.

This has minimal change to the current API, as we're using 0 for the
current NVMe status codes, so they are all unchanged. Since the MI
values alised those, they will have high bits set now, but we couldn't
previously distinguish them from the NVMe values anyway.

Fixes: #456

Signed-off-by: Jeremy Kerr <[email protected]>

Author: jk-ozlabs

src/nvme/mi.c

@@ -315,13 +315,12 @@ static int nvme_mi_admin_parse_status(struct nvme_mi_resp *resp, __u32 *result)
 	resp_hdr = (struct nvme_mi_msg_resp *)resp->hdr;
 
 	/* If we have a MI error, we can't be sure there's an admin header
-	 * following; return just the MI status
-	 *
-	 * TODO: this may alias the cdw3 result values, see
-	 * https://github.com/linux-nvme/libnvme/issues/456
+	 * following; return just the MI status, with the status type
+	 * indicator of MI.
 	 */
 	if (resp_hdr->status)
-		return resp_hdr->status;
+		return resp_hdr->status |
+			(NVME_STATUS_TYPE_MI << NVME_STATUS_TYPE_SHIFT);
 
 	/* We shouldn't hit this, as we'd have an error reported earlier.
 	 * However, for pointer safety, ensure we have a full admin header

src/nvme/types.h

@@ -6141,6 +6141,75 @@ static inline __u16 nvme_status_code(__u16 status_field)
 	return NVME_GET(status_field, SC);
 }
 
+/**
+ * enum nvme_status_type - type encoding for NVMe return values, when
+ * represented as an int.
+ *
+ * The nvme_* api returns an int, with negative values indicating an internal
+ * or syscall error, zero signifying success, positive values representing
+ * the NVMe status.
+ *
+ * That latter case (the NVMe status) may represent status values from
+ * different parts of the transport/controller/etc, and are at most 16 bits of
+ * data. So, we use the most-significant 3 bits of the signed int to indicate
+ * which type of status this is.
+ *
+ * @NVME_STATUS_TYPE_SHIFT: shift value for status bits
+ * @NVME_STATUS_TYPE_MASK:  mask value for status bits
+ *
+ * @NVME_STATUS_TYPE_NVME:  NVMe command status value, typically from CDW3
+ * @NVME_STATUS_TYPE_MI:    NVMe-MI header status
+ */
+enum nvme_status_type {
+	NVME_STATUS_TYPE_SHIFT		= 27,
+	NVME_STATUS_TYPE_MASK		= 0x7,
+
+	NVME_STATUS_TYPE_NVME		= 0,
+	NVME_STATUS_TYPE_MI		= 1,
+};
+
+/**
+ * nvme_status_get_type() - extract the type from a nvme_* return value
+ * @status: the (non-negative) return value from the NVMe API
+ *
+ * Returns: the type component of the status.
+ */
+static inline __u32 nvme_status_get_type(int status)
+{
+	return NVME_GET(status, STATUS_TYPE);
+}
+
+/**
+ * nvme_status_get_value() - extract the status value from a nvme_* return
+ * value
+ * @status: the (non-negative) return value from the NVMe API
+ *
+ * Returns: the value component of the status; the set of values will depend
+ * on the status type.
+ */
+static inline __u32 nvme_status_get_value(int status)
+{
+	return status & ~(NVME_STATUS_TYPE_MASK << NVME_STATUS_TYPE_SHIFT);
+}
+
+/**
+ * nvme_status_equals() - helper to check a status against a type and value
+ * @status: the (non-negative) return value from the NVMe API
+ * @type: the status type
+ * @value: the status value
+ *
+ * Returns: true if @status is of the specified type and value
+ */
+static inline __u32 nvme_status_equals(int status, enum nvme_status_type type,
+				       unsigned int value)
+{
+	if (status < 0)
+		return false;
+
+	return nvme_status_get_type(status) == type &&
+		nvme_status_get_value(status) == value;
+}
+
 /**
  * enum nvme_admin_opcode - Known NVMe admin opcodes
  * @nvme_admin_delete_sq:		Delete I/O Submission Queue

test/mi-mctp.c

@@ -308,7 +308,8 @@ static void test_admin_resp_err(nvme_mi_ep_t ep, struct test_peer *peer)
 	peer->tx_buf_len = 8;
 
 	rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
-	assert(rc == 0x2);
+	assert(nvme_status_get_type(rc) == NVME_STATUS_TYPE_MI);
+	assert(nvme_status_get_value(rc) == NVME_MI_RESP_INTERNAL_ERR);
 }
 
 /* test: all 4-byte aligned response sizes - should be decoded into the
@@ -332,7 +333,8 @@ static void test_admin_resp_sizes(nvme_mi_ep_t ep, struct test_peer *peer)
 	for (i = 8; i <= 4096 + 8; i+=4) {
 		peer->tx_buf_len = i;
 		rc = nvme_mi_admin_identify_ctrl(ctrl, &id);
-		assert(rc == 2);
+		assert(nvme_status_get_type(rc) == NVME_STATUS_TYPE_MI);
+		assert(nvme_status_get_value(rc) == NVME_MI_RESP_INTERNAL_ERR);
 	}
 
 	nvme_mi_close_ctrl(ctrl);