Commit 074c62d0 authored by Marcin Spiewak's avatar Marcin Spiewak Committed by Jim Harris
Browse files

nvme: fixed wrong PSDT set, if no support for SGLs in MPTR 



This patch fixes a bug, where the cmd.psdt was incorrectly
set if the NVMe controller reported in Identify Controller
Data Structure that it supports SGLs (bits 01:00 set
to 10b), but it doesn't support Metadata Pointer
containing an SGL Descriptor (bit 19 set to 0).
In such case the cmd.psdt shall be set to
SPDK_NVME_PSDT_SGL_MPTR_CONTIG, while it was set to
SPDK_NVME_PSDT_SGL_MPTR_SGL causing the controller
reported INVALID FIELD error.
Now it is fixed, and the cmd.psdt is correctly set for
both values passed in bit 19.

Fixes #2991

Change-Id: Ie4af5b7a6c1a8c5f69ee3ed80646e5c02f36286e
Signed-off-by: default avatarMarcin Spiewak <marcin.spiewak@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/18094


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
parent c2550778

include/spdk/nvme.h

+1 −0
Original line number Diff line number Diff line
@@ -570,6 +570,7 @@ enum spdk_nvme_ctrlr_flags { 
	SPDK_NVME_CTRLR_SGL_REQUIRES_DWORD_ALIGNMENT	= 1 << 4, /**< Dword alignment is required for SGL */

	SPDK_NVME_CTRLR_ZONE_APPEND_SUPPORTED		= 1 << 5, /**< Zone Append is supported (within Zoned Namespaces) */

	SPDK_NVME_CTRLR_DIRECTIVES_SUPPORTED		= 1 << 6, /**< The Directives is supported */

	SPDK_NVME_CTRLR_MPTR_SGL_SUPPORTED		= 1 << 7, /**< MPTR containing SGL descriptor is supported */

};



/**

lib/nvme/nvme_ctrlr.c

+4 −0
Original line number Diff line number Diff line
@@ -1957,6 +1957,10 @@ nvme_ctrlr_identify_done(void *arg, const struct spdk_nvme_cpl *cpl) 
		NVME_CTRLR_DEBUGLOG(ctrlr, "transport max_sges %u\n", ctrlr->max_sges);

	}



	if (ctrlr->cdata.sgls.metadata_address && !(ctrlr->quirks & NVME_QUIRK_NOT_USE_SGL)) {

		ctrlr->flags |= SPDK_NVME_CTRLR_MPTR_SGL_SUPPORTED;

	}



	if (ctrlr->cdata.oacs.security && !(ctrlr->quirks & NVME_QUIRK_OACS_SECURITY)) {

		ctrlr->flags |= SPDK_NVME_CTRLR_SECURITY_SEND_RECV_SUPPORTED;

	}

lib/nvme/nvme_pcie_common.c

+6 −3
Original line number Diff line number Diff line
@@ -1582,7 +1582,7 @@ static build_req_fn const g_nvme_pcie_build_req_table[][2] = { 


static int

nvme_pcie_qpair_build_metadata(struct spdk_nvme_qpair *qpair, struct nvme_tracker *tr,

			       bool sgl_supported, bool dword_aligned)

			       bool sgl_supported, bool mptr_sgl_supported, bool dword_aligned)

{

	void *md_payload;

	struct nvme_request *req = tr->req;
@@ -1596,7 +1596,7 @@ nvme_pcie_qpair_build_metadata(struct spdk_nvme_qpair *qpair, struct nvme_tracke 
		}



		mapping_length = req->md_size;

		if (sgl_supported && dword_aligned) {

		if (sgl_supported && mptr_sgl_supported && dword_aligned) {

			assert(req->cmd.psdt == SPDK_NVME_PSDT_SGL_MPTR_CONTIG);

			req->cmd.psdt = SPDK_NVME_PSDT_SGL_MPTR_SGL;
@@ -1632,6 +1632,7 @@ nvme_pcie_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_reques 
	struct nvme_pcie_qpair	*pqpair = nvme_pcie_qpair(qpair);

	enum nvme_payload_type	payload_type;

	bool			sgl_supported;

	bool			mptr_sgl_supported;

	bool			dword_aligned = true;



	if (spdk_unlikely(nvme_qpair_is_admin_queue(qpair))) {
@@ -1662,6 +1663,8 @@ nvme_pcie_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_reques 
		 */

		sgl_supported = (ctrlr->flags & SPDK_NVME_CTRLR_SGL_SUPPORTED) != 0 &&

				!nvme_qpair_is_admin_queue(qpair);

		mptr_sgl_supported = (ctrlr->flags & SPDK_NVME_CTRLR_MPTR_SGL_SUPPORTED) != 0 &&

				     !nvme_qpair_is_admin_queue(qpair);



		if (sgl_supported) {

			/* Don't use SGL for DSM command */
@@ -1686,7 +1689,7 @@ nvme_pcie_qpair_submit_request(struct spdk_nvme_qpair *qpair, struct nvme_reques 
			goto exit;

		}



		rc = nvme_pcie_qpair_build_metadata(qpair, tr, sgl_supported, dword_aligned);

		rc = nvme_pcie_qpair_build_metadata(qpair, tr, sgl_supported, mptr_sgl_supported, dword_aligned);

		if (rc < 0) {

			assert(rc == -EFAULT);

			rc = 0;

test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c

+4 −4
Original line number Diff line number Diff line
@@ -509,7 +509,7 @@ test_nvme_pcie_qpair_build_metadata(void) 
	tr.prp_sgl_bus_addr = 0xDBADBEEF;

	MOCK_SET(spdk_vtophys, 0xDCADBEE0);



	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, true, true);

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, true, true, true);

	CU_ASSERT(rc == 0);

	CU_ASSERT(req.cmd.psdt = SPDK_NVME_PSDT_SGL_MPTR_SGL);

	CU_ASSERT(tr.meta_sgl.address == 0xDCADBEE0);
@@ -521,7 +521,7 @@ test_nvme_pcie_qpair_build_metadata(void) 
	/* Non-IOVA contiguous metadata buffers should fail. */

	g_vtophys_size = 1024;

	req.cmd.psdt = SPDK_NVME_PSDT_SGL_MPTR_CONTIG;

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, true, true);

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, true, true, true);

	CU_ASSERT(rc == -EINVAL);

	g_vtophys_size = 0;
@@ -530,13 +530,13 @@ test_nvme_pcie_qpair_build_metadata(void) 
	/* Build non sgl metadata */

	MOCK_SET(spdk_vtophys, 0xDDADBEE0);



	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, false, true);

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, false, false, true);

	CU_ASSERT(rc == 0);

	CU_ASSERT(req.cmd.mptr == 0xDDADBEE0);



	/* Non-IOVA contiguous metadata buffers should fail. */

	g_vtophys_size = 1024;

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, false, true);

	rc = nvme_pcie_qpair_build_metadata(qpair, &tr, false, false, true);

	CU_ASSERT(rc == -EINVAL);

	g_vtophys_size = 0;