nvme: Make striping a quirk

Use the standard quirk mechanism to specify which devices
need software assisted striping.

Change-Id: Id8156876a90b4caf9d687637e14c7ad4a66ceda6
Signed-off-by: Ben Walker <benjamin.walker@intel.com>

lib/nvme/nvme_ctrlr.c

@@ -225,17 +225,11 @@ static void
 nvme_ctrlr_construct_intel_support_log_page_list(struct spdk_nvme_ctrlr *ctrlr,
 		struct spdk_nvme_intel_log_page_directory *log_page_directory)
 {
-	struct spdk_pci_id pci_id;
-
 	if (log_page_directory == NULL) {
 		return;
 	}
 
-	if (nvme_transport_ctrlr_get_pci_id(ctrlr, &pci_id)) {
+	if (ctrlr->cdata.vid != SPDK_PCI_VID_INTEL) {
-		return;
-	}
-
-	if (pci_id.vendor_id != SPDK_PCI_VID_INTEL) {
 		return;
 	}
 
@@ -1286,7 +1280,6 @@ nvme_robust_mutex_init_recursive_shared(pthread_mutex_t *mtx)
 int
 nvme_ctrlr_construct(struct spdk_nvme_ctrlr *ctrlr)
 {
-	struct spdk_pci_id pci_id;
 	int rc;
 
 	nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_INIT, NVME_TIMEOUT_INFINITE);
@@ -1305,10 +1298,6 @@ nvme_ctrlr_construct(struct spdk_nvme_ctrlr *ctrlr)
 		return rc;
 	}
 
-	if (nvme_transport_ctrlr_get_pci_id(ctrlr, &pci_id) == 0) {
-		ctrlr->quirks = nvme_get_quirks(&pci_id);
-	}
-
 	TAILQ_INIT(&ctrlr->active_procs);
 
 	return rc;

lib/nvme/nvme_internal.h

@@ -78,6 +78,12 @@
  */
 #define NVME_QUIRK_DELAY_BEFORE_CHK_RDY	0x4
 
+/*
+ * The controller performs best when I/O is split on particular
+ * LBA boundaries.
+ */
+#define NVME_INTEL_QUIRK_STRIPING 0x8
+
 #define NVME_MAX_ASYNC_EVENTS	(8)
 
 #define NVME_MIN_TIMEOUT_PERIOD		(5)
@@ -419,8 +425,6 @@ extern struct nvme_driver *g_spdk_nvme_driver;
 
 #define nvme_min(a,b) (((a)<(b))?(a):(b))
 
-#define INTEL_DC_P3X00_DEVID	0x0953
-
 #define nvme_delay		usleep
 
 static inline uint32_t

lib/nvme/nvme_ns.c

@@ -183,20 +183,15 @@ spdk_nvme_ns_get_data(struct spdk_nvme_ns *ns)
 int nvme_ns_construct(struct spdk_nvme_ns *ns, uint16_t id,
 		      struct spdk_nvme_ctrlr *ctrlr)
 {
-	struct spdk_pci_id pci_id;
-
 	assert(id > 0);
 
 	ns->ctrlr = ctrlr;
 	ns->id = id;
 	ns->stripe_size = 0;
 
-	if (nvme_transport_ctrlr_get_pci_id(ctrlr, &pci_id) == 0) {
+	if (ctrlr->quirks & NVME_INTEL_QUIRK_STRIPING &&
-		if (pci_id.vendor_id == SPDK_PCI_VID_INTEL &&
+	    ctrlr->cdata.vs[3] != 0) {
-		    pci_id.device_id == INTEL_DC_P3X00_DEVID &&
+		ns->stripe_size = (1 << ctrlr->cdata.vs[3]) * ctrlr->min_page_size;
-		    ctrlr->cdata.vs[3] != 0) {
-			ns->stripe_size = (1 << ctrlr->cdata.vs[3]) * ctrlr->min_page_size;
-		}
 	}
 
 	return nvme_ns_identify_update(ns);

lib/nvme/nvme_pcie.c

@@ -632,6 +632,8 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(enum spdk_nvme_transport_type
 		return NULL;
 	}
 
+	pctrlr->ctrlr.quirks = nvme_get_quirks(&probe_info->pci_id);
+
 	rc = nvme_pcie_ctrlr_construct_admin_qpair(&pctrlr->ctrlr);
 	if (rc != 0) {
 		nvme_ctrlr_destruct(&pctrlr->ctrlr);

lib/nvme/nvme_quirks.c

@@ -39,14 +39,15 @@ struct nvme_quirk {
 };
 
 static const struct nvme_quirk nvme_quirks[] = {
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3702}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3702}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3703}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3703}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3704}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3704}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3705}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3705}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3709}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x3709}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x370a}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY	},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_VID_INTEL, 0x370a}, NVME_INTEL_QUIRK_READ_LATENCY | NVME_INTEL_QUIRK_WRITE_LATENCY | NVME_INTEL_QUIRK_STRIPING	},
-	{{SPDK_PCI_VID_MEMBLAZE, 0x0540, SPDK_PCI_ANY_ID, SPDK_PCI_ANY_ID}, NVME_QUIRK_DELAY_BEFORE_CHK_RDY				},
+	{{SPDK_PCI_VID_INTEL, 0x0953, SPDK_PCI_ANY_ID, SPDK_PCI_ANY_ID}, NVME_INTEL_QUIRK_STRIPING								},
-	{{0x0000, 0x0000, 0x0000, 0x0000}, 0												}
+	{{SPDK_PCI_VID_MEMBLAZE, 0x0540, SPDK_PCI_ANY_ID, SPDK_PCI_ANY_ID}, NVME_QUIRK_DELAY_BEFORE_CHK_RDY							},
+	{{0x0000, 0x0000, 0x0000, 0x0000}, 0															}
 };
 
 /* Compare each field. SPDK_PCI_ANY_ID in s1 matches everything */

test/lib/nvme/unit/Makefile

@@ -34,7 +34,8 @@
 SPDK_ROOT_DIR := $(abspath $(CURDIR)/../../../..)
 include $(SPDK_ROOT_DIR)/mk/spdk.common.mk
 
-DIRS-y = nvme_c nvme_ns_cmd_c nvme_qpair_c nvme_ctrlr_c nvme_ctrlr_cmd_c
+DIRS-y = nvme_c nvme_ns_cmd_c nvme_qpair_c nvme_ctrlr_c \
+         nvme_ctrlr_cmd_c nvme_quirks_c
 
 .PHONY: all clean $(DIRS-y)
 

test/lib/nvme/unit/nvme_ctrlr_c/nvme_ctrlr_ut.c

@@ -51,11 +51,6 @@ struct nvme_driver _g_nvme_driver = {
 	.request_mempool = NULL,
 };
 
-static uint16_t g_pci_vendor_id;
-static uint16_t g_pci_device_id;
-static uint16_t g_pci_subvendor_id;
-static uint16_t g_pci_subdevice_id;
-
 uint64_t g_ut_tsc = 0;
 struct spdk_nvme_registers g_ut_nvme_regs = {};
 
@@ -89,11 +84,6 @@ nvme_transport_ctrlr_get_pci_id(struct spdk_nvme_ctrlr *ctrlr, struct spdk_pci_i
 		return -EINVAL;
 	}
 
-	pci_id->vendor_id = g_pci_vendor_id;
-	pci_id->device_id = g_pci_device_id;
-	pci_id->subvendor_id = g_pci_subvendor_id;
-	pci_id->subdevice_id = g_pci_subdevice_id;
-
 	return 0;
 }
 
@@ -1181,21 +1171,19 @@ test_nvme_ctrlr_construct_intel_support_log_page_list(void)
 	bool	res;
 	struct spdk_nvme_ctrlr				ctrlr = {};
 	struct spdk_nvme_intel_log_page_directory	payload = {};
-	struct spdk_pci_id				pci_id;
+	struct spdk_pci_id 				pci_id = {};
 
-	/* set a invalid vendor id */
+	/* Get quirks for a device with all 0 vendor/device id */
-	g_pci_vendor_id = 0xFFFF;
-	nvme_transport_ctrlr_get_pci_id(&ctrlr, &pci_id);
 	ctrlr.quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT(ctrlr.quirks == 0);
 
 	nvme_ctrlr_construct_intel_support_log_page_list(&ctrlr, &payload);
 	res = spdk_nvme_ctrlr_is_log_page_supported(&ctrlr, SPDK_NVME_INTEL_LOG_TEMPERATURE);
 	CU_ASSERT(res == false);
 
-	/* set valid vendor id and log page directory*/
+	/* Set the vendor to Intel, but provide no device id */
-	g_pci_vendor_id = SPDK_PCI_VID_INTEL;
+	ctrlr.cdata.vid = pci_id.vendor_id = SPDK_PCI_VID_INTEL;
 	payload.temperature_statistics_log_len = 1;
-	nvme_transport_ctrlr_get_pci_id(&ctrlr, &pci_id);
 	ctrlr.quirks = nvme_get_quirks(&pci_id);
 	memset(ctrlr.log_page_supported, 0, sizeof(ctrlr.log_page_supported));
 
@@ -1212,11 +1200,10 @@ test_nvme_ctrlr_construct_intel_support_log_page_list(void)
 	/* set valid vendor id, device id and sub device id*/
 	ctrlr.cdata.vid = SPDK_PCI_VID_INTEL;
 	payload.temperature_statistics_log_len = 0;
-	g_pci_vendor_id = SPDK_PCI_VID_INTEL;
+	pci_id.vendor_id = SPDK_PCI_VID_INTEL;
-	g_pci_device_id = 0x0953;
+	pci_id.device_id = 0x0953;
-	g_pci_subvendor_id = SPDK_PCI_VID_INTEL;
+	pci_id.subvendor_id = SPDK_PCI_VID_INTEL;
-	g_pci_subdevice_id = 0x3702;
+	pci_id.subdevice_id = 0x3702;
-	nvme_transport_ctrlr_get_pci_id(&ctrlr, &pci_id);
 	ctrlr.quirks = nvme_get_quirks(&pci_id);
 	memset(ctrlr.log_page_supported, 0, sizeof(ctrlr.log_page_supported));
 

test/lib/nvme/unit/nvme_quirks_c/.gitignore

@@ -0,0 +1 @@
+nvme_quirks_ut

test/lib/nvme/unit/nvme_quirks_c/Makefile

@@ -0,0 +1,38 @@
+#
+#  BSD LICENSE
+#
+#  Copyright (c) Intel Corporation.
+#  All rights reserved.
+#
+#  Redistribution and use in source and binary forms, with or without
+#  modification, are permitted provided that the following conditions
+#  are met:
+#
+#    * Redistributions of source code must retain the above copyright
+#      notice, this list of conditions and the following disclaimer.
+#    * Redistributions in binary form must reproduce the above copyright
+#      notice, this list of conditions and the following disclaimer in
+#      the documentation and/or other materials provided with the
+#      distribution.
+#    * Neither the name of Intel Corporation nor the names of its
+#      contributors may be used to endorse or promote products derived
+#      from this software without specific prior written permission.
+#
+#  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+
+SPDK_ROOT_DIR := $(abspath $(CURDIR)/../../../../..)
+
+TEST_FILE = nvme_quirks_ut.c
+
+include $(SPDK_ROOT_DIR)/mk/nvme.unittest.mk

test/lib/nvme/unit/nvme_quirks_c/nvme_quirks_ut.c

@@ -0,0 +1,100 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright (c) Intel Corporation.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "spdk_cunit.h"
+
+#include "nvme/nvme_quirks.c"
+
+static void
+test_nvme_quirks_striping(void)
+{
+	struct spdk_pci_id pci_id = {};
+	uint64_t quirks = 0;
+
+	/* Non-Intel device should not have striping enabled */
+	quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT((quirks & NVME_INTEL_QUIRK_STRIPING) == 0);
+
+	/* Set the vendor id to Intel, but no device id. No striping. */
+	pci_id.vendor_id = SPDK_PCI_VID_INTEL;
+	quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT((quirks & NVME_INTEL_QUIRK_STRIPING) == 0);
+
+	/* Device ID 0x0953 should have striping enabled */
+	pci_id.device_id = 0x0953;
+	quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT((quirks & NVME_INTEL_QUIRK_STRIPING) != 0);
+
+	/* Even if specific subvendor/subdevice ids are set,
+	 * striping should be enabled.
+	 */
+	pci_id.subvendor_id = SPDK_PCI_VID_INTEL;
+	pci_id.subdevice_id = 0x3704;
+	quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT((quirks & NVME_INTEL_QUIRK_STRIPING) != 0);
+
+	pci_id.subvendor_id = 1234;
+	pci_id.subdevice_id = 42;
+	quirks = nvme_get_quirks(&pci_id);
+	CU_ASSERT((quirks & NVME_INTEL_QUIRK_STRIPING) != 0);
+}
+
+int main(int argc, char **argv)
+{
+	CU_pSuite	suite = NULL;
+	unsigned int	num_failures;
+
+	if (CU_initialize_registry() != CUE_SUCCESS) {
+		return CU_get_error();
+	}
+
+	suite = CU_add_suite("nvme_quirks", NULL, NULL);
+	if (suite == NULL) {
+		CU_cleanup_registry();
+		return CU_get_error();
+	}
+
+	if (
+		CU_add_test(suite, "test nvme_quirks striping",
+			    test_nvme_quirks_striping) == NULL
+	) {
+		CU_cleanup_registry();
+		return CU_get_error();
+	}
+
+	CU_basic_set_mode(CU_BRM_VERBOSE);
+	CU_basic_run_tests();
+	num_failures = CU_get_number_of_failures();
+	CU_cleanup_registry();
+	return num_failures;
+}