From 265a8436f47c99967ca534e108331562d1916ce1 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Mon, 10 Feb 2020 14:13:53 -0700 Subject: [PATCH] nvme: Change mapping semantics of controller memory buffer Instead of creating an allocator where the driver manages the space, now, since using the CMB for queues and data has already been disallowed, just create functions to map and unmap the entire CMB. The user can manage the space. Change-Id: I023994deda3b517e14d2ba464c7375bf22b58456 Signed-off-by: Ben Walker Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/785 Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu Reviewed-by: Shuhei Matsumoto Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI --- doc/peer_2_peer.md | 4 +-- examples/nvme/cmb_copy/cmb_copy.c | 8 ++--- examples/nvme/hello_world/hello_world.c | 7 ++-- include/spdk/nvme.h | 36 +++++++++---------- lib/nvme/nvme_ctrlr.c | 14 ++++---- lib/nvme/nvme_internal.h | 4 +-- lib/nvme/nvme_pcie.c | 23 ++++++------ lib/nvme/nvme_transport.c | 12 +++---- .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 4 +-- 9 files changed, 53 insertions(+), 59 deletions(-) diff --git a/doc/peer_2_peer.md b/doc/peer_2_peer.md index 4d169bdb9..fbcc23374 100644 --- a/doc/peer_2_peer.md +++ b/doc/peer_2_peer.md @@ -29,8 +29,8 @@ capabilities are given in the table below. Key Functions | Description ------------------------------------------- | ----------- -spdk_nvme_ctrlr_alloc_cmb_io_buffer() | @copybrief spdk_nvme_ctrlr_alloc_cmb_io_buffer() -spdk_nvme_ctrlr_free_cmb_io_buffer() | @copybrief spdk_nvme_ctrlr_free_cmb_io_buffer() +spdk_nvme_ctrlr_map_cmb() | @copybrief spdk_nvme_ctrlr_map_cmb() +spdk_nvme_ctrlr_unmap_cmb() | @copybrief spdk_nvme_ctrlr_unmap_cmb() spdk_nvme_ctrlr_get_regs_cmbsz() | @copybrief spdk_nvme_ctrlr_get_regs_cmbsz() # Determining device support {#p2p_support} diff --git a/examples/nvme/cmb_copy/cmb_copy.c b/examples/nvme/cmb_copy/cmb_copy.c index c24774e6a..c82d259ab 100644 --- a/examples/nvme/cmb_copy/cmb_copy.c +++ b/examples/nvme/cmb_copy/cmb_copy.c @@ -109,6 +109,7 @@ cmb_copy(void) { int rc = 0, rw; void *buf; + size_t sz; /* Allocate QPs for the read and write controllers */ g_config.read.qpair = spdk_nvme_ctrlr_alloc_io_qpair(g_config.read.ctrlr, NULL, 0); @@ -119,8 +120,8 @@ cmb_copy(void) } /* Allocate a buffer from our CMB */ - buf = spdk_nvme_ctrlr_alloc_cmb_io_buffer(g_config.cmb.ctrlr, g_config.copy_size); - if (buf == NULL) { + buf = spdk_nvme_ctrlr_map_cmb(g_config.cmb.ctrlr, &sz); + if (buf == NULL || sz != g_config.copy_size) { printf("ERROR: buffer allocation failed\n"); printf("Are you sure %s has a valid CMB?\n", g_config.cmb.trid.traddr); @@ -162,8 +163,7 @@ cmb_copy(void) g_config.write.done = 0; /* Free CMB buffer */ - spdk_nvme_ctrlr_free_cmb_io_buffer(g_config.cmb.ctrlr, buf, - g_config.copy_size); + spdk_nvme_ctrlr_unmap_cmb(g_config.cmb.ctrlr); /* Free the queues */ spdk_nvme_ctrlr_free_io_qpair(g_config.read.qpair); diff --git a/examples/nvme/hello_world/hello_world.c b/examples/nvme/hello_world/hello_world.c index c6d2a892e..6e1d9d62a 100644 --- a/examples/nvme/hello_world/hello_world.c +++ b/examples/nvme/hello_world/hello_world.c @@ -138,7 +138,7 @@ write_complete(void *arg, const struct spdk_nvme_cpl *completion) * the data back from the NVMe namespace. */ if (sequence->using_cmb_io) { - spdk_nvme_ctrlr_free_cmb_io_buffer(ns_entry->ctrlr, sequence->buf, 0x1000); + spdk_nvme_ctrlr_unmap_cmb(ns_entry->ctrlr); } else { spdk_free(sequence->buf); } @@ -160,6 +160,7 @@ hello_world(void) struct ns_entry *ns_entry; struct hello_world_sequence sequence; int rc; + size_t sz; ns_entry = g_namespaces; while (ns_entry != NULL) { @@ -187,8 +188,8 @@ hello_world(void) * I/O operations. */ sequence.using_cmb_io = 1; - sequence.buf = spdk_nvme_ctrlr_alloc_cmb_io_buffer(ns_entry->ctrlr, 0x1000); - if (sequence.buf == NULL) { + sequence.buf = spdk_nvme_ctrlr_map_cmb(ns_entry->ctrlr, &sz); + if (sequence.buf == NULL || sz < 0x1000) { sequence.using_cmb_io = 0; sequence.buf = spdk_zmalloc(0x1000, 0x1000, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_DMA); } diff --git a/include/spdk/nvme.h b/include/spdk/nvme.h index 7930d6b35..9153056d0 100644 --- a/include/spdk/nvme.h +++ b/include/spdk/nvme.h @@ -1911,33 +1911,31 @@ int spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload volatile struct spdk_nvme_registers *spdk_nvme_ctrlr_get_registers(struct spdk_nvme_ctrlr *ctrlr); /** - * Allocate an I/O buffer from the controller memory buffer (Experimental). + * Map the controller memory buffer for use in I/O operations. * - * This function allocates registered memory which belongs to the Controller - * Memory Buffer (CMB) of the specified NVMe controller. Note that the CMB has - * to support the WDS and RDS capabilities for the allocation to be successful. - * Also, due to vtophys contraints the CMB must be at least 4MiB in size. Free - * memory allocated with this function using spdk_nvme_ctrlr_free_cmb_io_buffer(). + * This function maps registered memory which belongs to the Controller + * Memory Buffer (CMB) of the specified NVMe controller so that it is visible + * from the CPU. Note that the CMB has to support the WDS and RDS capabilities + * for the mapping to be successful. Also, due to vtophys contraints the CMB must + * be at least 4MiB in size. + * + * This cannot be used in conjunction with placing submission queues in the + * controller memory buffer. * * \param ctrlr Controller from which to allocate memory buffer. - * \param size Size of buffer to allocate in bytes. + * \param size Size of buffer that was mapped. Return value. * - * \return Pointer to controller memory buffer allocation, or NULL if allocation + * \return Pointer to controller memory buffer, or NULL if mapping * was not possible. */ -void *spdk_nvme_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size); +void *spdk_nvme_ctrlr_map_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size); /** - * Free a controller memory I/O buffer (Experimental). + * Free a controller memory I/O buffer. * - * Note this function is currently a NOP which is one reason why this and - * spdk_nvme_ctrlr_alloc_cmb_io_buffer() are currently marked as experimental. - * - * \param ctrlr Controller from which the buffer was allocated. - * \param buf Buffer previously allocated by spdk_nvme_ctrlr_alloc_cmb_io_buffer(). - * \param size Size of buf in bytes. + * \param ctrlr Controller from which to unmap the memory buffer. */ -void spdk_nvme_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size); +void spdk_nvme_ctrlr_unmap_cmb(struct spdk_nvme_ctrlr *ctrlr); /** * Get the transport ID for a given NVMe controller. @@ -3082,9 +3080,9 @@ struct spdk_nvme_transport_ops { uint16_t (*ctrlr_get_max_sges)(struct spdk_nvme_ctrlr *ctrlr); - void *(*ctrlr_alloc_cmb_io_buffer)(struct spdk_nvme_ctrlr *ctrlr, size_t size); + void *(*ctrlr_map_cmb)(struct spdk_nvme_ctrlr *ctrlr, size_t *size); - int (*ctrlr_free_cmb_io_buffer)(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size); + int (*ctrlr_unmap_cmb)(struct spdk_nvme_ctrlr *ctrlr); struct spdk_nvme_qpair *(*ctrlr_create_io_qpair)(struct spdk_nvme_ctrlr *ctrlr, uint16_t qid, const struct spdk_nvme_io_qpair_opts *opts); diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 4a106a355..5d5abdd2c 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -3364,7 +3364,7 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui } void * -spdk_nvme_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) +spdk_nvme_ctrlr_map_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size) { void *buf; @@ -3373,20 +3373,18 @@ spdk_nvme_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) } nvme_robust_mutex_lock(&ctrlr->ctrlr_lock); - buf = nvme_transport_ctrlr_alloc_cmb_io_buffer(ctrlr, size); + buf = nvme_transport_ctrlr_map_cmb(ctrlr, size); nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); return buf; } void -spdk_nvme_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size) +spdk_nvme_ctrlr_unmap_cmb(struct spdk_nvme_ctrlr *ctrlr) { - if (buf && size) { - nvme_robust_mutex_lock(&ctrlr->ctrlr_lock); - nvme_transport_ctrlr_free_cmb_io_buffer(ctrlr, buf, size); - nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); - } + nvme_robust_mutex_lock(&ctrlr->ctrlr_lock); + nvme_transport_ctrlr_unmap_cmb(ctrlr); + nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); } bool diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 74c4beffd..19199b461 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -1162,8 +1162,8 @@ uint32_t nvme_transport_ctrlr_get_max_xfer_size(struct spdk_nvme_ctrlr *ctrlr); uint16_t nvme_transport_ctrlr_get_max_sges(struct spdk_nvme_ctrlr *ctrlr); struct spdk_nvme_qpair *nvme_transport_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, uint16_t qid, const struct spdk_nvme_io_qpair_opts *opts); -void *nvme_transport_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size); -int nvme_transport_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size); +void *nvme_transport_ctrlr_map_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size); +int nvme_transport_ctrlr_unmap_cmb(struct spdk_nvme_ctrlr *ctrlr); int nvme_transport_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpair *qpair); int nvme_transport_ctrlr_connect_qpair(struct spdk_nvme_ctrlr *ctrlr, diff --git a/lib/nvme/nvme_pcie.c b/lib/nvme/nvme_pcie.c index 3a1a7e272..375a8738f 100644 --- a/lib/nvme/nvme_pcie.c +++ b/lib/nvme/nvme_pcie.c @@ -582,11 +582,13 @@ nvme_pcie_ctrlr_unmap_cmb(struct nvme_pcie_ctrlr *pctrlr) } static void * -nvme_pcie_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) +nvme_pcie_ctrlr_map_io_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size) { struct nvme_pcie_ctrlr *pctrlr = nvme_pcie_ctrlr(ctrlr); uint64_t offset; + *size = 0; + if (pctrlr->cmb.bar_va == NULL) { SPDK_DEBUGLOG(SPDK_LOG_NVME, "CMB not available\n"); return NULL; @@ -604,25 +606,20 @@ nvme_pcie_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) offset = (pctrlr->cmb.current_offset + (3)) & ~(3); - /* CMB may only consume part of the BAR, calculate accordingly */ - if (offset + size > pctrlr->cmb.end) { - SPDK_ERRLOG("Tried to allocate past valid CMB range!\n"); + if (offset >= pctrlr->cmb.end) { return NULL; } - pctrlr->cmb.current_offset = offset + size; + *size = pctrlr->cmb.end - offset; + + pctrlr->cmb.current_offset = pctrlr->cmb.end; return pctrlr->cmb.bar_va + offset; } static int -nvme_pcie_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size) +nvme_pcie_ctrlr_unmap_io_cmb(struct spdk_nvme_ctrlr *ctrlr) { - /* - * Do nothing for now. - * TODO: Track free space so buffers may be reused. - */ - SPDK_ERRLOG("no deallocation for CMB buffers yet!\n"); return 0; } @@ -2419,8 +2416,8 @@ const struct spdk_nvme_transport_ops pcie_ops = { .ctrlr_get_max_xfer_size = nvme_pcie_ctrlr_get_max_xfer_size, .ctrlr_get_max_sges = nvme_pcie_ctrlr_get_max_sges, - .ctrlr_alloc_cmb_io_buffer = nvme_pcie_ctrlr_alloc_cmb_io_buffer, - .ctrlr_free_cmb_io_buffer = nvme_pcie_ctrlr_free_cmb_io_buffer, + .ctrlr_map_cmb = nvme_pcie_ctrlr_map_io_cmb, + .ctrlr_unmap_cmb = nvme_pcie_ctrlr_unmap_io_cmb, .ctrlr_create_io_qpair = nvme_pcie_ctrlr_create_io_qpair, .ctrlr_delete_io_qpair = nvme_pcie_ctrlr_delete_io_qpair, diff --git a/lib/nvme/nvme_transport.c b/lib/nvme/nvme_transport.c index 61cf6f0aa..0e090869c 100644 --- a/lib/nvme/nvme_transport.c +++ b/lib/nvme/nvme_transport.c @@ -216,26 +216,26 @@ nvme_transport_ctrlr_get_max_sges(struct spdk_nvme_ctrlr *ctrlr) } void * -nvme_transport_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) +nvme_transport_ctrlr_map_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size) { const struct spdk_nvme_transport *transport = nvme_get_transport(ctrlr->trid.trstring); assert(transport != NULL); - if (transport->ops.ctrlr_alloc_cmb_io_buffer != NULL) { - return transport->ops.ctrlr_alloc_cmb_io_buffer(ctrlr, size); + if (transport->ops.ctrlr_map_cmb != NULL) { + return transport->ops.ctrlr_map_cmb(ctrlr, size); } return NULL; } int -nvme_transport_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size) +nvme_transport_ctrlr_unmap_cmb(struct spdk_nvme_ctrlr *ctrlr) { const struct spdk_nvme_transport *transport = nvme_get_transport(ctrlr->trid.trstring); assert(transport != NULL); - if (transport->ops.ctrlr_free_cmb_io_buffer != NULL) { - return transport->ops.ctrlr_free_cmb_io_buffer(ctrlr, buf, size); + if (transport->ops.ctrlr_unmap_cmb != NULL) { + return transport->ops.ctrlr_unmap_cmb(ctrlr); } return 0; diff --git a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c index cbba3a42f..884e6ab77 100644 --- a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c +++ b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c @@ -137,13 +137,13 @@ nvme_transport_ctrlr_get_max_sges(struct spdk_nvme_ctrlr *ctrlr) } void * -nvme_transport_ctrlr_alloc_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, size_t size) +nvme_transport_ctrlr_map_cmb(struct spdk_nvme_ctrlr *ctrlr, size_t *size) { return NULL; } int -nvme_transport_ctrlr_free_cmb_io_buffer(struct spdk_nvme_ctrlr *ctrlr, void *buf, size_t size) +nvme_transport_ctrlr_unmap_cmb(struct spdk_nvme_ctrlr *ctrlr) { return 0; }