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: https://github.com/linux-nvme/libnvme/issues/456
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

diff --git a/src/nvme/mi.c b/src/nvme/mi.c
index e5ef0a0cd152c97b11c68ffbc38b875406335906..fa0e1df19e943a8cb05abe8a00d2681f5acb640d 100644 (file)
--- a/src/nvme/mi.c
+++ b/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

diff --git a/src/nvme/types.h b/src/nvme/types.h
index 14f50d655c8157a31d51911d844461e6cf647664..cba57676da489f4e516f7a79324b204154d9111a 100644 (file)
--- a/src/nvme/types.h
+++ b/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

diff --git a/test/mi-mctp.c b/test/mi-mctp.c
index 0773d000bb60245184c461d38ee3dfbccca80c75..6d83d427e32af471b7ae1bbafba9c7bd4b7a7f43 100644 (file)
--- a/test/mi-mctp.c
+++ b/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);