Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v11 0/9]  nvmet: add target passthru commands support
@ 2020-02-20 20:36 Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands Logan Gunthorpe
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

Hi,

This is v11 of the passthru patchset. There was little review for v10
so I'm hoping to get some more review this cycle.

v10 addressed the vast majority of Christoph's feedback from v9.There
were a couple issues that were not addressed discussed below.

I don't think cloning the ctrl_id or the subsysnqn is a good idea.
I sent an email trying to explain why here[1] but there was no response.
In short, I think cloning the ctrl_id will break multipathing over
fabrics and copying the subsysnqn only has the effect of breaking
loopback; the user can always copy the underlying subsysnqn if it
makes sense for their overall system.

I maintain overriding the CMIC bit in the ctrl id is necessary to
allow multipath over fabrics even if the underlying device did
not support multipath.

I also think the black list for admin commands is appropriate, and I
added it based on Sagi's feedback[2]. There are plenty of commands that
may be dangerous like firmware update and format NVM commands, and NS
attach commands won't work out of the box because we don't copy the
ctrl_id. It seems like there's more commands to be careful of than ones
that are that are obviously acceptable. So, I think the prudent course
is blacklisting by default until someone has a usecase and can show
the command is safe seems and makes sense. For our present use cases,
the identify, log page and vendor specific commands are all that we
care about.

A git branch is available here:

https://github.com/sbates130272/linux-p2pmem nvmet_passthru_v11

Thanks,

Logan

[1] https://lore.kernel.org/linux-block/247eca47-c3bc-6452-fb19-f7aa27b05a60@deltatee.com/
[2] https://lore.kernel.org/linux-block/e4430207-7def-8776-0289-0d58689dc0cd@grimberg.me/

--

v11 Changes:
  1. Rebased onto v5.6-rc2
  2. Collected Max's Reviewed-By tag

v10 Changes:
  1. Rebased onto v5.5-rc1
  2. Disable all exports in core nvme if CONFIG_NVME_TARGET_PASSTHRU is
     not set and put them near the end of the file with a big fat
     comment (per Christoph)
  3. Don't fake up the vs field: pass it through as is and bump
     it to 1.2.1 if it is below that (per Christoph)
  4. Rework how passthru requests are submitted into the core
     with proper nvme_passthru_start/end handling (per Christoph)
  5. Rework how commands are parsed with passthru hooks in
     parse_admin_cmd() and nvmet_parse_io_cmd() (per Christoph)
  6. Rework commands are handled so they are only done in a work
     item if absolutely necessary (per Christoph)
  7. The data_len hack was dropped as a patchset was introduced to
     remove data_len altogether (per Christoph)
  8. The passthru accounting changes are now in v5.5-rc1
  9. A large number of other minor cleanups that were pointed out by
     Christoph

v9 Changes:
  1. Rebased onto v5.4-rc2 (required adjusting nvme_identify_ns() usage)
  2. Collected Sagi's Reviewed-By Tags
  3. Squashed seperate Kconfig patch into passthru patch (Per Sagi)
  4. Set REQ_FUA for flush requests and remove special casing
     on RQF_IO_STAT (Per Sagi)

v8 Changes:
  1. Rebased onto v5.3-rc6
  2. Collected Max's Reviewed-By tags
  3. Converted admin command black-list to a white-list, but
     allow all vendor specific commands. With this, we feel
     it's safe to allow multiple connections from hosts.
     (As per Sagi's feedback)

v7 Changes:
  1. Rebased onto v5.3-rc2
  2. Rework nvme_ctrl_get_by_path() to use filp_open() instead of
     the cdev changes that were in v6. (Per Al)
  3. Override the cmic bit to allow multipath and allow
     multiple connections from the same hostnqn. (At the same
     time I cleaned up the method of rejecting multiple connections.)
     See Patch 8)
  4. Found a bug when used with the tcp transport (See Patch 10)

v6 Changes:
  1. Rebased onto v5.3-rc1
  2. Rework configfs interface to simply be a passthru directory
     within the existing subsystem. The directory is similar to
     and consistent with a namespace directory.
  3. Have the configfs take a path instead of a bare controller name
  4. Renamed the main passthru file to io-cmd-passthru.c for consistency
     with the file and block-dev methods.
  5. Cleaned up all the CONFIG_NVME_TARGET_PASSTHRU usage to remove
     all the inline #ifdefs
  6. Restructured nvmet_passthru_make_request() a bit for clearer code
  7. Moved nvme_find_get_ns() call into nvmet_passthru_execute_cmd()
     seeing calling it in nvmet_req_init() causes a lockdep warning
     due to nvme_find_get_ns() being able to sleep.
  8. Added a check in nvmet_passthru_execute_cmd() to ensure we don't
     violate queue_max_segments or queue_max_hw_sectors and overrode
     mdts to ensure hosts don't intentionally submit commands
     that will exceed these limits.
  9. Reworked the code which ensures there's only one subsystem per
     passthru controller to use an xarray instead of a list as this is
     simpler and more easily fixed some bugs triggered by disabling
     subsystems that weren't enabled.
 10. Removed the overide of the target cntlid with the passthru cntlid;
     this seemed like a really bad idea especially in the presence of
     mixed systems as you could end up with two ctrlrs with the same
     cntlid. For now, commands that depend on cntlid are black listed.
 11. Implement block accounting for passthru so the target can track
     usage using /proc/diskstats
 12. A number of other minor bug fixes and cleanups

v5 Changes (not sent to list, from Chaitanya):
  1. Added workqueue for admin commands.
  2. Added kconfig option for the pass-thru feature.
  3. Restructure the parsing code according to your suggestion,
     call nvmet_xxx_parse_cmd() from nvmet_passthru_parse_cmd().
  4. Use pass-thru instead of pt.
  5. Several cleanups and add comments at the appropriate locations.
  6. Minimize the code for checking pass-thru ns across all the subsystems.
  7. Removed the delays in the ns related admin commands since I was
     not able to reproduce the previous bug.

v4 Changes:
  1. Add request polling interface to the block layer.
  2. Use request polling interface in the NVMEoF target passthru code
     path.
  3. Add checks suggested by Sagi for creating one target ctrl per
     passthru ctrl.
  4. Don't enable the namespace if it belongs to the configured passthru
     ctrl.
  5. Adjust the code latest kernel.

v3 Changes:
  1. Split the addition of passthru command handlers and integration
     into two different patches since we add guards to create one target
     controller per passthru controller. This way it will be easier to
     review the code.
  2. Adjust the code for 4.18.

v2 Changes:
  1. Update the new nvme core controller find API naming and
     changed the string comparison of the ctrl.
  2. Get rid of the newly added #defines for target ctrl values.
  3. Use the newly added structure members in the same patch where
     they are used. Aggregate the passthru command handling support
     and integration with nvmet-core into one patch.
  4. Introduce global NVMe Target subsystem list for connected and
     not connected subsystems on the target side.
  5. Add check when configuring the target ns and target
     passthru ctrl to allow only one target controller to be created
     for one passthru subsystem.
  6. Use the passthru ctrl cntlid when creating the
     target controller.

Chaitanya Kulkarni (1):
  nvmet-passthru: Introduce NVMet passthru Kconfig option

Logan Gunthorpe (8):
  nvme-core: Clear any SGL flags in passthru commands
  nvme: Create helper function to obtain command effects
  nvme: Move nvme_passthru_[start|end]() calls to common helper
  nvme-core: Introduce nvme_ctrl_get_by_path()
  nvme: Export existing nvme core functions
  nvmet-passthru: Add passthru code to process commands
  nvmet-passthru: Add enable/disable helpers
  nvmet-configfs: Introduce passthru configfs interface

 drivers/nvme/host/core.c        | 229 ++++++++++-------
 drivers/nvme/host/nvme.h        |  14 +
 drivers/nvme/target/Kconfig     |  10 +
 drivers/nvme/target/Makefile    |   1 +
 drivers/nvme/target/admin-cmd.c |   7 +-
 drivers/nvme/target/configfs.c  | 102 ++++++++
 drivers/nvme/target/core.c      |  13 +-
 drivers/nvme/target/nvmet.h     |  52 ++++
 drivers/nvme/target/passthru.c  | 435 ++++++++++++++++++++++++++++++++
 include/linux/nvme.h            |   1 +
 10 files changed, 771 insertions(+), 93 deletions(-)
 create mode 100644 drivers/nvme/target/passthru.c


base-commit: 11a48a5a18c63fd7621bb050228cebf13566e4d8
--
2.20.1

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:08   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 2/9] nvme: Create helper function to obtain command effects Logan Gunthorpe
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

The host driver should decide whether to use SGLs or PRPs and they
currently assume the flags are cleared after the call to
nvme_setup_cmd(). However, passed-through commands may erroneously
set these bits; so clear them for all cases.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/host/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ada59df642d2..330b05ef1b16 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -764,6 +764,8 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
 	case REQ_OP_DRV_IN:
 	case REQ_OP_DRV_OUT:
 		memcpy(cmd, nvme_req(req)->cmd, sizeof(*cmd));
+		/* passthru commands should let the driver set the SGL flags */
+		cmd->common.flags &= ~NVME_CMD_SGL_ALL;
 		break;
 	case REQ_OP_FLUSH:
 		nvme_setup_flush(ns, cmd);
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 2/9] nvme: Create helper function to obtain command effects
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:09   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper Logan Gunthorpe
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

Separate the code to obtain command effects from the code
to start a passthru request and open code nvme_known_admin_effects()
in the new helper.

The new helper function will be necessary for nvmet passthru
code to determine if we need to change out of interrupt context
to handle the effects.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/host/core.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 330b05ef1b16..101137bdfece 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1300,22 +1300,8 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			metadata, meta_len, lower_32_bits(io.slba), NULL, 0);
 }
 
-static u32 nvme_known_admin_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_admin_format_nvm:
-		return NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
-					NVME_CMD_EFFECTS_CSE_MASK;
-	case nvme_admin_sanitize_nvm:
-		return NVME_CMD_EFFECTS_CSE_MASK;
-	default:
-		break;
-	}
-	return 0;
-}
-
-static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-								u8 opcode)
+static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+				u8 opcode)
 {
 	u32 effects = 0;
 
@@ -1331,7 +1317,26 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 
 	if (ctrl->effects)
 		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
+
+	switch (opcode) {
+	case nvme_admin_format_nvm:
+		effects |= NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
+			NVME_CMD_EFFECTS_CSE_MASK;
+		break;
+	case nvme_admin_sanitize_nvm:
+		effects |= NVME_CMD_EFFECTS_CSE_MASK;
+		break;
+	default:
+		break;
+	}
+
+	return effects;
+}
+
+static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			       u8 opcode)
+{
+	u32 effects = nvme_command_effects(ctrl, ns, opcode);
 
 	/*
 	 * For simplicity, IO to all namespaces is quiesced even if the command
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 2/9] nvme: Create helper function to obtain command effects Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:10   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option Logan Gunthorpe
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

Introduce a new nvme_execute_passthru_rq() helper which calls
nvme_passthru_[start|end]() around blk_execute_rq(). This ensures
all passthru calls (including nvme_submit_io()) will be wrapped
appropriately.

nvme_execute_passthru_rq() will also be useful for the nvmet passthru
code.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/host/core.c | 193 ++++++++++++++++++++-------------------
 1 file changed, 100 insertions(+), 93 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 101137bdfece..219e24304b4e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -898,6 +898,105 @@ static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 	return ERR_PTR(ret);
 }
 
+static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+				u8 opcode)
+{
+	u32 effects = 0;
+
+	if (ns) {
+		if (ctrl->effects)
+			effects = le32_to_cpu(ctrl->effects->iocs[opcode]);
+		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
+			dev_warn(ctrl->device,
+				 "IO command:%02x has unhandled effects:%08x\n",
+				 opcode, effects);
+		return 0;
+	}
+
+	if (ctrl->effects)
+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+
+	switch (opcode) {
+	case nvme_admin_format_nvm:
+		effects |= NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
+			NVME_CMD_EFFECTS_CSE_MASK;
+		break;
+	case nvme_admin_sanitize_nvm:
+		effects |= NVME_CMD_EFFECTS_CSE_MASK;
+		break;
+	default:
+		break;
+	}
+
+	return effects & ~NVME_CMD_EFFECTS_CSUPP;
+}
+
+static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			       u8 opcode)
+{
+	u32 effects = nvme_command_effects(ctrl, ns, opcode);
+
+	/*
+	 * For simplicity, IO to all namespaces is quiesced even if the command
+	 * effects say only one namespace is affected.
+	 */
+	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
+		mutex_lock(&ctrl->scan_lock);
+		mutex_lock(&ctrl->subsys->lock);
+		nvme_mpath_start_freeze(ctrl->subsys);
+		nvme_mpath_wait_freeze(ctrl->subsys);
+		nvme_start_freeze(ctrl);
+		nvme_wait_freeze(ctrl);
+	}
+	return effects;
+}
+
+static void nvme_update_formats(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	down_read(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		if (ns->disk && nvme_revalidate_disk(ns->disk))
+			nvme_set_queue_dying(ns);
+	up_read(&ctrl->namespaces_rwsem);
+}
+
+static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
+{
+	/*
+	 * Revalidate LBA changes prior to unfreezing. This is necessary to
+	 * prevent memory corruption if a logical block size was changed by
+	 * this command.
+	 */
+	if (effects & NVME_CMD_EFFECTS_LBCC)
+		nvme_update_formats(ctrl);
+	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
+		nvme_unfreeze(ctrl);
+		nvme_mpath_unfreeze(ctrl->subsys);
+		mutex_unlock(&ctrl->subsys->lock);
+		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
+		mutex_unlock(&ctrl->scan_lock);
+	}
+	if (effects & NVME_CMD_EFFECTS_CCC)
+		nvme_init_identify(ctrl);
+	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
+		nvme_queue_scan(ctrl);
+}
+
+static void nvme_execute_passthru_rq(struct request *rq)
+{
+	struct nvme_command *cmd = nvme_req(rq)->cmd;
+	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
+	struct nvme_ns *ns = rq->q->queuedata;
+	struct gendisk *disk = ns ? ns->disk : NULL;
+	u32 effects;
+
+	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
+	blk_execute_rq(rq->q, disk, rq, 0);
+	nvme_passthru_end(ctrl, effects);
+}
+
 static int nvme_submit_user_cmd(struct request_queue *q,
 		struct nvme_command *cmd, void __user *ubuffer,
 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
@@ -936,7 +1035,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 		}
 	}
 
-	blk_execute_rq(req->q, disk, req, 0);
+	nvme_execute_passthru_rq(req);
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		ret = -EINTR;
 	else
@@ -1300,99 +1399,12 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			metadata, meta_len, lower_32_bits(io.slba), NULL, 0);
 }
 
-static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-				u8 opcode)
-{
-	u32 effects = 0;
-
-	if (ns) {
-		if (ctrl->effects)
-			effects = le32_to_cpu(ctrl->effects->iocs[opcode]);
-		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
-			dev_warn(ctrl->device,
-				 "IO command:%02x has unhandled effects:%08x\n",
-				 opcode, effects);
-		return 0;
-	}
-
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-
-	switch (opcode) {
-	case nvme_admin_format_nvm:
-		effects |= NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
-			NVME_CMD_EFFECTS_CSE_MASK;
-		break;
-	case nvme_admin_sanitize_nvm:
-		effects |= NVME_CMD_EFFECTS_CSE_MASK;
-		break;
-	default:
-		break;
-	}
-
-	return effects;
-}
-
-static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-			       u8 opcode)
-{
-	u32 effects = nvme_command_effects(ctrl, ns, opcode);
-
-	/*
-	 * For simplicity, IO to all namespaces is quiesced even if the command
-	 * effects say only one namespace is affected.
-	 */
-	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
-		mutex_lock(&ctrl->scan_lock);
-		mutex_lock(&ctrl->subsys->lock);
-		nvme_mpath_start_freeze(ctrl->subsys);
-		nvme_mpath_wait_freeze(ctrl->subsys);
-		nvme_start_freeze(ctrl);
-		nvme_wait_freeze(ctrl);
-	}
-	return effects;
-}
-
-static void nvme_update_formats(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
-		if (ns->disk && nvme_revalidate_disk(ns->disk))
-			nvme_set_queue_dying(ns);
-	up_read(&ctrl->namespaces_rwsem);
-}
-
-static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
-{
-	/*
-	 * Revalidate LBA changes prior to unfreezing. This is necessary to
-	 * prevent memory corruption if a logical block size was changed by
-	 * this command.
-	 */
-	if (effects & NVME_CMD_EFFECTS_LBCC)
-		nvme_update_formats(ctrl);
-	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
-		nvme_unfreeze(ctrl);
-		nvme_mpath_unfreeze(ctrl->subsys);
-		mutex_unlock(&ctrl->subsys->lock);
-		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
-		mutex_unlock(&ctrl->scan_lock);
-	}
-	if (effects & NVME_CMD_EFFECTS_CCC)
-		nvme_init_identify(ctrl);
-	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC))
-		nvme_queue_scan(ctrl);
-}
-
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd)
 {
 	struct nvme_passthru_cmd cmd;
 	struct nvme_command c;
 	unsigned timeout = 0;
-	u32 effects;
 	u64 result;
 	int status;
 
@@ -1419,12 +1431,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
-	effects = nvme_passthru_start(ctrl, ns, cmd.opcode);
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
 			(void __user *)(uintptr_t)cmd.metadata,
 			cmd.metadata_len, 0, &result, timeout);
-	nvme_passthru_end(ctrl, effects);
 
 	if (status >= 0) {
 		if (put_user(result, &ucmd->result))
@@ -1440,7 +1450,6 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct nvme_passthru_cmd64 cmd;
 	struct nvme_command c;
 	unsigned timeout = 0;
-	u32 effects;
 	int status;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1466,12 +1475,10 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
-	effects = nvme_passthru_start(ctrl, ns, cmd.opcode);
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
 			(void __user *)(uintptr_t)cmd.metadata, cmd.metadata_len,
 			0, &cmd.result, timeout);
-	nvme_passthru_end(ctrl, effects);
 
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (2 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:12   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 5/9] nvme-core: Introduce nvme_ctrl_get_by_path() Logan Gunthorpe
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

This patch updates KConfig file for the NVMeOF target where we add new
option so that user can selectively enable/disable passthru code.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
[logang@deltatee.com: fixed some of the wording in the help message]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index d7f48c0fb311..2478cb5a932d 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -15,6 +15,16 @@ config NVME_TARGET
 	  To configure the NVMe target you probably want to use the nvmetcli
 	  tool from http://git.infradead.org/users/hch/nvmetcli.git.
 
+config NVME_TARGET_PASSTHRU
+	bool "NVMe Target Passthrough support"
+	depends on NVME_CORE
+	depends on NVME_TARGET
+	help
+	  This enables target side NVMe passthru controller support for the
+	  NVMe Over Fabrics protocol. It allows for hosts to manage and
+	  directly access an actual NVMe controller residing on the target
+	  side, incuding executing Vendor Unique Commands.
+
 config NVME_TARGET_LOOP
 	tristate "NVMe loopback device support"
 	depends on NVME_TARGET
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 5/9] nvme-core: Introduce nvme_ctrl_get_by_path()
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (3 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 6/9] nvme: Export existing nvme core functions Logan Gunthorpe
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

nvme_ctrl_get_by_path() is analagous to blkdev_get_by_path() except it
gets a struct nvme_ctrl from the path to its char dev (/dev/nvme0).
It makes use of filp_open() to open the file and uses the private
data to obtain a pointer to the struct nvme_ctrl. If the fops of the
file do not match, -EINVAL is returned.

The purpose of this function is to support NVMe-OF target passthru.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 31 +++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  9 +++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 219e24304b4e..756f8ee201d9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4227,6 +4227,37 @@ void nvme_sync_queues(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_sync_queues);
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+/*
+ * The exports that follow within this ifdef are only for
+ * use by the nvmet-passthru and should not be used for
+ * other things.
+ */
+
+struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path)
+{
+	struct nvme_ctrl *ctrl;
+	struct file *f;
+
+	f = filp_open(path, O_RDWR, 0);
+	if (IS_ERR(f))
+		return ERR_CAST(f);
+
+	if (f->f_op != &nvme_dev_fops) {
+		ctrl = ERR_PTR(-EINVAL);
+		goto out_close;
+	}
+
+	ctrl = f->private_data;
+	nvme_get_ctrl(ctrl);
+
+out_close:
+	filp_close(f, NULL);
+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(nvme_ctrl_get_by_path);
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
 /*
  * Check we didn't inadvertently grow the command structure sizes:
  */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1024fec7914c..196a0d38e19c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -687,4 +687,13 @@ void nvme_hwmon_init(struct nvme_ctrl *ctrl);
 static inline void nvme_hwmon_init(struct nvme_ctrl *ctrl) { }
 #endif
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+/*
+ * The exports that follow within this ifdef are only for
+ * use by the nvmet-passthru and should not be used for
+ * other things.
+ */
+struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path);
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
 #endif /* _NVME_H */
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 6/9] nvme: Export existing nvme core functions
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (4 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 5/9] nvme-core: Introduce nvme_ctrl_get_by_path() Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:13   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands Logan Gunthorpe
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

Export nvme_put_ns(), nvme_command_effects(), nvme_execute_passthru_rq()
and nvme_find_get_ns() for use in the nvmet passthru code.

The exports are conditional on CONFIG_NVME_TARGET_PASSTHRU.

Based-on-a-patch-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/nvme/host/core.c | 14 +++++++++-----
 drivers/nvme/host/nvme.h |  5 +++++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 756f8ee201d9..ea6285da7dbd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -465,7 +465,7 @@ static void nvme_free_ns(struct kref *kref)
 	kfree(ns);
 }
 
-static void nvme_put_ns(struct nvme_ns *ns)
+void nvme_put_ns(struct nvme_ns *ns)
 {
 	kref_put(&ns->kref, nvme_free_ns);
 }
@@ -898,8 +898,8 @@ static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 	return ERR_PTR(ret);
 }
 
-static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-				u8 opcode)
+u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			 u8 opcode)
 {
 	u32 effects = 0;
 
@@ -984,7 +984,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 		nvme_queue_scan(ctrl);
 }
 
-static void nvme_execute_passthru_rq(struct request *rq)
+void nvme_execute_passthru_rq(struct request *rq)
 {
 	struct nvme_command *cmd = nvme_req(rq)->cmd;
 	struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
@@ -3449,7 +3449,7 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 	return nsa->head->ns_id - nsb->head->ns_id;
 }
 
-static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns *ns, *ret = NULL;
 
@@ -4233,6 +4233,10 @@ EXPORT_SYMBOL_GPL(nvme_sync_queues);
  * use by the nvmet-passthru and should not be used for
  * other things.
  */
+EXPORT_SYMBOL_GPL(nvme_put_ns);
+EXPORT_SYMBOL_GPL(nvme_command_effects);
+EXPORT_SYMBOL_GPL(nvme_execute_passthru_rq);
+EXPORT_SYMBOL_GPL(nvme_find_get_ns);
 
 struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path)
 {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 196a0d38e19c..5e96c30b8291 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -693,6 +693,11 @@ static inline void nvme_hwmon_init(struct nvme_ctrl *ctrl) { }
  * use by the nvmet-passthru and should not be used for
  * other things.
  */
+void nvme_put_ns(struct nvme_ns *ns);
+u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			 u8 opcode);
+void nvme_execute_passthru_rq(struct request *rq);
+struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned int nsid);
 struct nvme_ctrl *nvme_ctrl_get_by_path(const char *path);
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (5 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 6/9] nvme: Export existing nvme core functions Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:28   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers Logan Gunthorpe
  2020-02-20 20:36 ` [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface Logan Gunthorpe
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

Add passthru command handling capability for the NVMeOF target and
export passthru APIs which are used to integrate passthru
code with nvmet-core.

The new file passthru.c handles passthru cmd parsing and execution.
In the passthru mode, we create a block layer request from the nvmet
request and map the data on to the block layer request. Only admin
commands on a white list are let through which includes vendor specific
commands.

Based-on-a-patch-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/Makefile    |   1 +
 drivers/nvme/target/admin-cmd.c |   7 +-
 drivers/nvme/target/core.c      |   3 +
 drivers/nvme/target/nvmet.h     |  39 ++++
 drivers/nvme/target/passthru.c  | 348 ++++++++++++++++++++++++++++++++
 include/linux/nvme.h            |   1 +
 6 files changed, 397 insertions(+), 2 deletions(-)
 create mode 100644 drivers/nvme/target/passthru.c

diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index 2b33836f3d3e..ebf91fc4c72e 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NVME_TARGET_TCP)		+= nvmet-tcp.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o fabrics-cmd.o \
 			discovery.o io-cmd-file.o io-cmd-bdev.o
+nvmet-$(CONFIG_NVME_TARGET_PASSTHRU)	+= passthru.o
 nvme-loop-y	+= loop.o
 nvmet-rdma-y	+= rdma.o
 nvmet-fc-y	+= fc.o
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 72a7e41f3018..5f281cd3812a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -716,7 +716,7 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 	return 0;
 }
 
-static void nvmet_execute_set_features(struct nvmet_req *req)
+void nvmet_execute_set_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
@@ -782,7 +782,7 @@ void nvmet_get_feat_async_event(struct nvmet_req *req)
 	nvmet_set_result(req, READ_ONCE(req->sq->ctrl->aen_enabled));
 }
 
-static void nvmet_execute_get_features(struct nvmet_req *req)
+void nvmet_execute_get_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
@@ -898,6 +898,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 	if (unlikely(ret))
 		return ret;
 
+	if (nvmet_req_passthru_ctrl(req))
+		return nvmet_parse_passthru_admin_cmd(req);
+
 	switch (cmd->common.opcode) {
 	case nvme_admin_get_log_page:
 		req->execute = nvmet_execute_get_log_page;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 576de773b4db..40998b514439 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -841,6 +841,9 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 	if (unlikely(ret))
 		return ret;
 
+	if (nvmet_req_passthru_ctrl(req))
+		return nvmet_setup_passthru_command(req);
+
 	req->ns = nvmet_find_namespace(req->sq->ctrl, cmd->rw.nsid);
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index eda28b22a2c8..6e632d0a88ca 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -227,6 +227,10 @@ struct nvmet_subsys {
 
 	struct config_group	namespaces_group;
 	struct config_group	allowed_hosts_group;
+
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+	struct nvme_ctrl	*passthru_ctrl;
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
@@ -302,6 +306,11 @@ struct nvmet_req {
 			struct bio_vec          *bvec;
 			struct work_struct      work;
 		} f;
+		struct {
+			struct request		*rq;
+			struct work_struct      work;
+			u16 (*end_req)(struct nvmet_req *req);
+		} p;
 	};
 	int			sg_cnt;
 	/* data length as parsed from the SGL descriptor: */
@@ -379,6 +388,8 @@ void nvmet_req_complete(struct nvmet_req *req, u16 status);
 int nvmet_req_alloc_sgl(struct nvmet_req *req);
 void nvmet_req_free_sgl(struct nvmet_req *req);
 
+void nvmet_execute_set_features(struct nvmet_req *req);
+void nvmet_execute_get_features(struct nvmet_req *req);
 void nvmet_execute_keep_alive(struct nvmet_req *req);
 
 void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq, u16 qid,
@@ -500,6 +511,34 @@ static inline u32 nvmet_dsm_len(struct nvmet_req *req)
 		sizeof(struct nvme_dsm_range);
 }
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req);
+u16 nvmet_setup_passthru_command(struct nvmet_req *req);
+static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return subsys->passthru_ctrl;
+}
+#else /* CONFIG_NVME_TARGET_PASSTHRU */
+static inline u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
+{
+	return 0;
+}
+static inline u16 nvmet_setup_passthru_command(struct nvmet_req *req)
+{
+	return 0;
+}
+static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
+{
+	return NULL;
+}
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static inline struct nvme_ctrl *
+nvmet_req_passthru_ctrl(struct nvmet_req *req)
+{
+	return nvmet_passthru_ctrl(req->sq->ctrl->subsys);
+}
+
 u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
 
 /* Convert a 32-bit number to a 16-bit 0's based number */
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
new file mode 100644
index 000000000000..01127bfa6c8a
--- /dev/null
+++ b/drivers/nvme/target/passthru.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NVMe Over Fabrics Target Passthrough command implementation.
+ *
+ * Copyright (c) 2017-2018 Western Digital Corporation or its
+ * affiliates.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/module.h>
+
+#include "../host/nvme.h"
+#include "nvmet.h"
+
+static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
+	struct request *rq = req->p.rq;
+	u16 status;
+
+	nvme_execute_passthru_rq(rq);
+
+	status = nvme_req(rq)->status;
+	if (status == NVME_SC_SUCCESS && req->p.end_req)
+		status = req->p.end_req(req);
+
+	req->cqe->result = nvme_req(rq)->result;
+	nvmet_req_complete(req, status);
+	blk_put_request(rq);
+}
+
+static void nvmet_passthru_req_done(struct request *rq,
+				    blk_status_t blk_status)
+{
+	struct nvmet_req *req = rq->end_io_data;
+
+	req->cqe->result = nvme_req(rq)->result;
+	nvmet_req_complete(req, nvme_req(rq)->status);
+	blk_put_request(rq);
+}
+
+static int nvmet_passthru_map_sg(struct nvmet_req *req, struct request *rq)
+{
+	int sg_cnt = req->sg_cnt;
+	struct scatterlist *sg;
+	int op_flags = 0;
+	struct bio *bio;
+	int i, ret;
+
+	if (req->cmd->common.opcode == nvme_cmd_flush)
+		op_flags = REQ_FUA;
+	else if (nvme_is_write(req->cmd))
+		op_flags = REQ_SYNC | REQ_IDLE;
+
+
+	bio = bio_alloc(GFP_KERNEL, min(sg_cnt, BIO_MAX_PAGES));
+	bio->bi_end_io = bio_put;
+
+	for_each_sg(req->sg, sg, req->sg_cnt, i) {
+		if (bio_add_page(bio, sg_page(sg), sg->length,
+				 sg->offset) != sg->length) {
+			ret = blk_rq_append_bio(rq, &bio);
+			if (unlikely(ret))
+				return ret;
+
+			bio->bi_opf = req_op(rq) | op_flags;
+			bio = bio_alloc(GFP_KERNEL,
+					min(sg_cnt, BIO_MAX_PAGES));
+		}
+		sg_cnt--;
+	}
+
+	ret = blk_rq_append_bio(rq, &bio);
+	if (unlikely(ret))
+		return ret;
+
+	return 0;
+}
+
+static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
+{
+	struct nvme_ctrl *ctrl = nvmet_req_passthru_ctrl(req);
+	struct nvme_ns *ns = NULL;
+	struct request *rq = NULL;
+	struct request_queue *q;
+	u32 effects;
+	u16 status;
+	int ret;
+
+	if (likely(req->sq->qid != 0)) {
+		u32 nsid = le32_to_cpu(req->cmd->common.nsid);
+
+		ns = nvme_find_get_ns(ctrl, nsid);
+		if (unlikely(!ns)) {
+			pr_err("failed to get passthru ns nsid:%u\n", nsid);
+			status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+			goto fail_out;
+		}
+	}
+
+	if (ns)
+		q = ns->queue;
+	else
+		q = ctrl->admin_q;
+
+	rq = nvme_alloc_request(q, req->cmd, BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
+	if (IS_ERR(rq)) {
+		rq = NULL;
+		status = NVME_SC_INTERNAL;
+		goto fail_out;
+	}
+
+	if (req->sg_cnt) {
+		ret = nvmet_passthru_map_sg(req, rq);
+		if (unlikely(ret)) {
+			status = NVME_SC_INTERNAL;
+			goto fail_out;
+		}
+	}
+
+	if (blk_rq_nr_phys_segments(rq) > queue_max_segments(rq->q)) {
+		status = NVME_SC_INVALID_FIELD;
+		goto fail_out;
+	}
+
+	if ((blk_rq_payload_bytes(rq) >> 9) > queue_max_hw_sectors(rq->q)) {
+		status = NVME_SC_INVALID_FIELD;
+		goto fail_out;
+	}
+
+	/*
+	 * If there are effects for the command we are about to execute, or
+	 * an end_req function we need to use nvme_execute_passthru_rq()
+	 * synchronously in a work item seeing the end_req function and
+	 * nvme_passthru_end() can't be called in the request done callback
+	 * which is typically in interrupt context.
+	 */
+	effects = nvme_command_effects(ctrl, ns, req->cmd->common.opcode);
+	if (req->p.end_req || effects) {
+		INIT_WORK(&req->p.work, nvmet_passthru_execute_cmd_work);
+		req->p.rq = rq;
+		schedule_work(&req->p.work);
+	} else {
+		rq->end_io_data = req;
+		blk_execute_rq_nowait(rq->q, ns ? ns->disk : NULL, rq, 0,
+				      nvmet_passthru_req_done);
+	}
+
+	if (ns)
+		nvme_put_ns(ns);
+
+	return;
+
+fail_out:
+	if (ns)
+		nvme_put_ns(ns);
+	nvmet_req_complete(req, status);
+	blk_put_request(rq);
+}
+
+static u16 nvmet_passthru_override_id_ctrl(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvme_ctrl *pctrl = ctrl->subsys->passthru_ctrl;
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ctrl *id;
+	u32 max_hw_sectors;
+	int page_shift;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id)
+		return NVME_SC_INTERNAL;
+
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(*id));
+	if (status)
+		goto out_free;
+
+	id->cntlid = cpu_to_le16(ctrl->cntlid);
+	id->ver = cpu_to_le32(ctrl->subsys->ver);
+
+	/*
+	 * The passthru NVMe driver may have a limit on the number of segments
+	 * which depends on the host's memory fragementation. To solve this,
+	 * ensure mdts is limitted to the pages equal to the number of
+	 * segments.
+	 */
+	max_hw_sectors = min_not_zero(pctrl->max_segments << (PAGE_SHIFT - 9),
+				      pctrl->max_hw_sectors);
+
+	page_shift = NVME_CAP_MPSMIN(ctrl->cap) + 12;
+
+	id->mdts = ilog2(max_hw_sectors) + 9 - page_shift;
+
+	id->acl = 3;
+	/*
+	 * We export aerl limit for the fabrics controller, update this when
+	 * passthru based aerl support is added.
+	 */
+	id->aerl = NVMET_ASYNC_EVENTS - 1;
+
+	/* emulate kas as most of the PCIe ctrl don't have a support for kas */
+	id->kas = cpu_to_le16(NVMET_KAS);
+
+	/* don't support host memory buffer */
+	id->hmpre = 0;
+	id->hmmin = 0;
+
+	id->sqes = min_t(__u8, ((0x6 << 4) | 0x6), id->sqes);
+	id->cqes = min_t(__u8, ((0x4 << 4) | 0x4), id->cqes);
+	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
+
+	/* don't support fuse commands */
+	id->fuses = 0;
+
+	id->sgls = cpu_to_le32(1 << 0); /* we always support SGLs */
+	if (ctrl->ops->has_keyed_sgls)
+		id->sgls |= cpu_to_le32(1 << 2);
+	if (req->port->inline_data_size)
+		id->sgls |= cpu_to_le32(1 << 20);
+
+	/*
+	 * When passsthru controller is setup using nvme-loop transport it will
+	 * export the passthru ctrl subsysnqn (PCIe NVMe ctrl) and will fail in
+	 * the nvme/host/core.c in the nvme_init_subsystem()->nvme_active_ctrl()
+	 * code path with duplicate ctr subsynqn. In order to prevent that we
+	 * mask the passthru-ctrl subsysnqn with the target ctrl subsysnqn.
+	 */
+	memcpy(id->subnqn, ctrl->subsysnqn, sizeof(id->subnqn));
+
+	/* use fabric id-ctrl values */
+	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
+				req->port->inline_data_size) / 16);
+	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
+
+	id->msdbd = ctrl->ops->msdbd;
+
+	/* Support multipath connections with fabrics */
+	id->cmic |= 1 << 1;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(struct nvme_id_ctrl));
+
+out_free:
+	kfree(id);
+	return status;
+}
+
+static u16 nvmet_passthru_override_id_ns(struct nvmet_req *req)
+{
+	u16 status = NVME_SC_SUCCESS;
+	struct nvme_id_ns *id;
+	int i;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id)
+		return NVME_SC_INTERNAL;
+
+	status = nvmet_copy_from_sgl(req, 0, id, sizeof(struct nvme_id_ns));
+	if (status)
+		goto out_free;
+
+	for (i = 0; i < (id->nlbaf + 1); i++)
+		if (id->lbaf[i].ms)
+			memset(&id->lbaf[i], 0, sizeof(id->lbaf[i]));
+
+	id->flbas = id->flbas & ~(1 << 4);
+
+	/*
+	 * Presently the NVMEof target code does not support sending
+	 * metadata, so we must disable it here. This should be updated
+	 * once target starts supporting metadata.
+	 */
+	id->mc = 0;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+
+out_free:
+	kfree(id);
+	return status;
+}
+
+u16 nvmet_setup_passthru_command(struct nvmet_req *req)
+{
+	req->p.end_req = NULL;
+	req->execute = nvmet_passthru_execute_cmd;
+	return NVME_SC_SUCCESS;
+}
+
+u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
+{
+	/*
+	 * Passthru all vendor specific commands
+	 */
+	if (req->cmd->common.opcode >= nvme_admin_vendor_start)
+		return nvmet_setup_passthru_command(req);
+
+	switch (req->cmd->common.opcode) {
+	case nvme_admin_async_event:
+		req->execute = nvmet_execute_async_event;
+		return NVME_SC_SUCCESS;
+	case nvme_admin_keep_alive:
+		/*
+		 * Most PCIe ctrls don't support keep alive cmd, we route keep
+		 * alive to the non-passthru mode. In future please change this
+		 * code when PCIe ctrls with keep alive support available.
+		 */
+		req->execute = nvmet_execute_keep_alive;
+		return NVME_SC_SUCCESS;
+	case nvme_admin_set_features:
+		switch (le32_to_cpu(req->cmd->features.fid)) {
+		case NVME_FEAT_ASYNC_EVENT:
+		case NVME_FEAT_KATO:
+		case NVME_FEAT_NUM_QUEUES:
+			req->execute = nvmet_execute_set_features;
+			return NVME_SC_SUCCESS;
+		default:
+			return nvmet_setup_passthru_command(req);
+		}
+		break;
+	case nvme_admin_get_features:
+		switch (le32_to_cpu(req->cmd->features.fid)) {
+		case NVME_FEAT_ASYNC_EVENT:
+		case NVME_FEAT_KATO:
+		case NVME_FEAT_NUM_QUEUES:
+			req->execute = nvmet_execute_get_features;
+			return NVME_SC_SUCCESS;
+		default:
+			return nvmet_setup_passthru_command(req);
+		}
+		break;
+	case nvme_admin_identify:
+		switch (req->cmd->identify.cns) {
+		case NVME_ID_CNS_CTRL:
+			req->execute = nvmet_passthru_execute_cmd;
+			req->p.end_req = nvmet_passthru_override_id_ctrl;
+			return NVME_SC_SUCCESS;
+		case NVME_ID_CNS_NS:
+			req->execute = nvmet_passthru_execute_cmd;
+			req->p.end_req = nvmet_passthru_override_id_ns;
+			return NVME_SC_SUCCESS;
+		default:
+			return nvmet_setup_passthru_command(req);
+		}
+	case nvme_admin_get_log_page:
+		return nvmet_setup_passthru_command(req);
+	default:
+		/* By default, blacklist all admin commands */
+		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+	}
+}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3d5189f46cb1..e29f4b8145fa 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -858,6 +858,7 @@ enum nvme_admin_opcode {
 	nvme_admin_security_recv	= 0x82,
 	nvme_admin_sanitize_nvm		= 0x84,
 	nvme_admin_get_lba_status	= 0x86,
+	nvme_admin_vendor_start		= 0xC0,
 };
 
 #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (6 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:33   ` Sagi Grimberg
  2020-02-20 20:36 ` [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface Logan Gunthorpe
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

This patch adds helper functions which are used in the NVMeOF configfs
when the user is configuring the passthru subsystem. Here we ensure
that only one subsys is assigned to each nvme_ctrl by using an xarray
on the cntlid.

The subsystem's version number is overridden by the passed through
controller's version. However, if that version is less than 1.2.1,
then we bump the advertised version to that and print a warning
in dmesg.

Based-on-a-patch-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/nvme/target/configfs.c |  3 ++
 drivers/nvme/target/core.c     | 10 +++-
 drivers/nvme/target/nvmet.h    | 12 +++++
 drivers/nvme/target/passthru.c | 87 ++++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..354d43fab8db 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -828,6 +828,9 @@ static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
 	int major, minor, tertiary = 0;
 	int ret;
 
+	/* passthru subsystems use the underlying controller's version */
+	if (nvmet_passthru_ctrl(subsys))
+		return -EINVAL;
 
 	ret = sscanf(page, "%d.%d.%d\n", &major, &minor, &tertiary);
 	if (ret != 2 && ret != 3)
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 40998b514439..b282fb4e45ed 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -522,6 +522,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 
 	mutex_lock(&subsys->lock);
 	ret = 0;
+
+	if (nvmet_passthru_ctrl(subsys)) {
+		pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+		goto out_unlock;
+	}
+
 	if (ns->enabled)
 		goto out_unlock;
 
@@ -1418,7 +1424,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	if (!subsys)
 		return ERR_PTR(-ENOMEM);
 
-	subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */
+	subsys->ver = NVMET_DEFAULT_VS;
 	/* generate a random serial number as our controllers are ephemeral: */
 	get_random_bytes(&subsys->serial, sizeof(subsys->serial));
 
@@ -1459,6 +1465,8 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
+	nvmet_passthru_subsys_free(subsys);
+
 	kfree(subsys->subsysnqn);
 	kfree(subsys);
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6e632d0a88ca..7c247d803015 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -20,6 +20,8 @@
 #include <linux/blkdev.h>
 #include <linux/radix-tree.h>
 
+#define NVMET_DEFAULT_VS		NVME_VS(1, 3, 0)
+
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
 #define NVMET_NO_ERROR_LOC		((u16)-1)
@@ -230,6 +232,7 @@ struct nvmet_subsys {
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
+	char			*passthru_ctrl_path;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
@@ -512,6 +515,9 @@ static inline u32 nvmet_dsm_len(struct nvmet_req *req)
 }
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys);
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys);
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys);
 u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req);
 u16 nvmet_setup_passthru_command(struct nvmet_req *req);
 static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
@@ -519,6 +525,12 @@ static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
 	return subsys->passthru_ctrl;
 }
 #else /* CONFIG_NVME_TARGET_PASSTHRU */
+static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+}
+static inline void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+}
 static inline u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 {
 	return 0;
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 01127bfa6c8a..0705f4ff8b16 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -11,6 +11,11 @@
 #include "../host/nvme.h"
 #include "nvmet.h"
 
+/*
+ * xarray to maintain one passthru subsystem per nvme controller.
+ */
+static DEFINE_XARRAY(passthru_subsystems);
+
 static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
 {
 	struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
@@ -346,3 +351,85 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 }
+
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
+{
+	struct nvme_ctrl *ctrl;
+	int ret = -EINVAL;
+	void *old;
+
+	mutex_lock(&subsys->lock);
+	if (!subsys->passthru_ctrl_path)
+		goto out_unlock;
+	if (subsys->passthru_ctrl)
+		goto out_unlock;
+
+	if (subsys->nr_namespaces) {
+		pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+		goto out_unlock;
+	}
+
+	ctrl = nvme_ctrl_get_by_path(subsys->passthru_ctrl_path);
+	if (IS_ERR(ctrl)) {
+		ret = PTR_ERR(ctrl);
+		pr_err("failed to open nvme controller %s\n",
+		       subsys->passthru_ctrl_path);
+
+		goto out_unlock;
+	}
+
+	old = xa_cmpxchg(&passthru_subsystems, ctrl->cntlid, NULL,
+			 subsys, GFP_KERNEL);
+	if (xa_is_err(old)) {
+		ret = xa_err(old);
+		goto out_put_ctrl;
+	}
+
+	if (old)
+		goto out_put_ctrl;
+
+	subsys->passthru_ctrl = ctrl;
+	subsys->ver = ctrl->vs;
+
+	if (subsys->ver < NVME_VS(1, 2, 1)) {
+		pr_warn("nvme controller version is too old: %d.%d.%d, advertising 1.2.1\n",
+			(int)NVME_MAJOR(subsys->ver),
+			(int)NVME_MINOR(subsys->ver),
+			(int)NVME_TERTIARY(subsys->ver));
+		subsys->ver = NVME_VS(1, 2, 1);
+	}
+
+	mutex_unlock(&subsys->lock);
+	return 0;
+
+out_put_ctrl:
+	nvme_put_ctrl(ctrl);
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
+}
+
+static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+	if (subsys->passthru_ctrl) {
+		xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
+		nvme_put_ctrl(subsys->passthru_ctrl);
+	}
+	subsys->passthru_ctrl = NULL;
+	subsys->ver = NVMET_DEFAULT_VS;
+}
+
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+	mutex_lock(&subsys->lock);
+	__nvmet_passthru_ctrl_disable(subsys);
+	mutex_unlock(&subsys->lock);
+}
+
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+	mutex_lock(&subsys->lock);
+	__nvmet_passthru_ctrl_disable(subsys);
+	kfree(subsys->passthru_ctrl_path);
+	mutex_unlock(&subsys->lock);
+}
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface
  2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
                   ` (7 preceding siblings ...)
  2020-02-20 20:36 ` [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers Logan Gunthorpe
@ 2020-02-20 20:36 ` Logan Gunthorpe
  2020-02-26 23:35   ` Sagi Grimberg
  8 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-20 20:36 UTC (permalink / raw)
  To: linux-kernel, linux-nvme
  Cc: Sagi Grimberg, Chaitanya Kulkarni, Stephen Bates, Jens Axboe,
	Keith Busch, Max Gurtovoy, Logan Gunthorpe, Christoph Hellwig

When CONFIG_NVME_TARGET_PASSTHRU as 'passthru' directory will
be added to each subsystem. The directory is similar to a namespace
and has two attributes: device_path and enable. The user must set the
path to the nvme controller's char device and write '1' to enable the
subsystem to use passthru.

Any given subsystem is prevented from enabling both a regular namespace
and the passthru device. If one is enabled, enabling the other will
produce an error.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/configfs.c | 99 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h    |  1 +
 2 files changed, 100 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 354d43fab8db..1ae3f87f611d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -615,6 +615,103 @@ static const struct config_item_type nvmet_namespaces_type = {
 	.ct_owner		= THIS_MODULE,
 };
 
+#ifdef CONFIG_NVME_TARGET_PASSTHRU
+
+static ssize_t nvmet_passthru_device_path_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", subsys->passthru_ctrl_path);
+}
+
+static ssize_t nvmet_passthru_device_path_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	size_t len;
+	int ret;
+
+	mutex_lock(&subsys->lock);
+
+	ret = -EBUSY;
+	if (subsys->passthru_ctrl)
+		goto out_unlock;
+
+	ret = -EINVAL;
+	len = strcspn(page, "\n");
+	if (!len)
+		goto out_unlock;
+
+	kfree(subsys->passthru_ctrl_path);
+	ret = -ENOMEM;
+	subsys->passthru_ctrl_path = kstrndup(page, len, GFP_KERNEL);
+	if (!subsys->passthru_ctrl_path)
+		goto out_unlock;
+
+	mutex_unlock(&subsys->lock);
+
+	return count;
+out_unlock:
+	mutex_unlock(&subsys->lock);
+	return ret;
+}
+CONFIGFS_ATTR(nvmet_passthru_, device_path);
+
+static ssize_t nvmet_passthru_enable_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+
+	return sprintf(page, "%d\n", subsys->passthru_ctrl ? 1 : 0);
+}
+
+static ssize_t nvmet_passthru_enable_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item->ci_parent);
+	bool enable;
+	int ret = 0;
+
+	if (strtobool(page, &enable))
+		return -EINVAL;
+
+	if (enable)
+		ret = nvmet_passthru_ctrl_enable(subsys);
+	else
+		nvmet_passthru_ctrl_disable(subsys);
+
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_passthru_, enable);
+
+static struct configfs_attribute *nvmet_passthru_attrs[] = {
+	&nvmet_passthru_attr_device_path,
+	&nvmet_passthru_attr_enable,
+	NULL,
+};
+
+static const struct config_item_type nvmet_passthru_type = {
+	.ct_attrs		= nvmet_passthru_attrs,
+	.ct_owner		= THIS_MODULE,
+};
+
+static void nvmet_add_passthru_group(struct nvmet_subsys *subsys)
+{
+	config_group_init_type_name(&subsys->passthru_group,
+				    "passthru", &nvmet_passthru_type);
+	configfs_add_default_group(&subsys->passthru_group,
+				   &subsys->group);
+}
+
+#else /* CONFIG_NVME_TARGET_PASSTHRU */
+
+static void nvmet_add_passthru_group(struct nvmet_subsys *subsys)
+{
+}
+
+#endif /* CONFIG_NVME_TARGET_PASSTHRU */
+
 static int nvmet_port_subsys_allow_link(struct config_item *parent,
 		struct config_item *target)
 {
@@ -918,6 +1015,8 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 	configfs_add_default_group(&subsys->allowed_hosts_group,
 			&subsys->group);
 
+	nvmet_add_passthru_group(subsys);
+
 	return &subsys->group;
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 7c247d803015..2ff9a41223d4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -233,6 +233,7 @@ struct nvmet_subsys {
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
 	char			*passthru_ctrl_path;
+	struct config_group	passthru_group;
 #endif /* CONFIG_NVME_TARGET_PASSTHRU */
 };
 
-- 
2.20.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands
  2020-02-20 20:36 ` [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands Logan Gunthorpe
@ 2020-02-26 23:08   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:08 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 2/9] nvme: Create helper function to obtain command effects
  2020-02-20 20:36 ` [PATCH v11 2/9] nvme: Create helper function to obtain command effects Logan Gunthorpe
@ 2020-02-26 23:09   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:09 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper
  2020-02-20 20:36 ` [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper Logan Gunthorpe
@ 2020-02-26 23:10   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:10 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option
  2020-02-20 20:36 ` [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option Logan Gunthorpe
@ 2020-02-26 23:12   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:12 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig


> +config NVME_TARGET_PASSTHRU
> +	bool "NVMe Target Passthrough support"
> +	depends on NVME_CORE
> +	depends on NVME_TARGET
> +	help
> +	  This enables target side NVMe passthru controller support for the
> +	  NVMe Over Fabrics protocol. It allows for hosts to manage and
> +	  directly access an actual NVMe controller residing on the target
> +	  side, incuding executing Vendor Unique Commands.

If unsure, say N ?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 6/9] nvme: Export existing nvme core functions
  2020-02-20 20:36 ` [PATCH v11 6/9] nvme: Export existing nvme core functions Logan Gunthorpe
@ 2020-02-26 23:13   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:13 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands
  2020-02-20 20:36 ` [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands Logan Gunthorpe
@ 2020-02-26 23:28   ` Sagi Grimberg
  2020-02-27 17:33     ` Logan Gunthorpe
  0 siblings, 1 reply; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:28 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig


> +u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
> +{
> +	/*
> +	 * Passthru all vendor specific commands
> +	 */
> +	if (req->cmd->common.opcode >= nvme_admin_vendor_start)
> +		return nvmet_setup_passthru_command(req);
> +
> +	switch (req->cmd->common.opcode) {
> +	case nvme_admin_async_event:
> +		req->execute = nvmet_execute_async_event;
> +		return NVME_SC_SUCCESS;
> +	case nvme_admin_keep_alive:
> +		/*
> +		 * Most PCIe ctrls don't support keep alive cmd, we route keep
> +		 * alive to the non-passthru mode. In future please change this
> +		 * code when PCIe ctrls with keep alive support available.
> +		 */
> +		req->execute = nvmet_execute_keep_alive;
> +		return NVME_SC_SUCCESS;
> +	case nvme_admin_set_features:
> +		switch (le32_to_cpu(req->cmd->features.fid)) {
> +		case NVME_FEAT_ASYNC_EVENT:
> +		case NVME_FEAT_KATO:
> +		case NVME_FEAT_NUM_QUEUES:
> +			req->execute = nvmet_execute_set_features;
> +			return NVME_SC_SUCCESS;
> +		default:
> +			return nvmet_setup_passthru_command(req);
> +		}

This looks questionable... There are tons of features that doesn't
make sense here like hmb, temperature stuff, irq stuff, timestamps,
reservations etc... passing-through these will have confusing
semantics.. Maybe white-list what actually makes sense to passthru?

> +		break;
> +	case nvme_admin_get_features:
> +		switch (le32_to_cpu(req->cmd->features.fid)) {
> +		case NVME_FEAT_ASYNC_EVENT:
> +		case NVME_FEAT_KATO:
> +		case NVME_FEAT_NUM_QUEUES:
> +			req->execute = nvmet_execute_get_features;
> +			return NVME_SC_SUCCESS;
> +		default:
> +			return nvmet_setup_passthru_command(req);
> +		}

Same here.

> +		break;
> +	case nvme_admin_identify:
> +		switch (req->cmd->identify.cns) {
> +		case NVME_ID_CNS_CTRL:
> +			req->execute = nvmet_passthru_execute_cmd;
> +			req->p.end_req = nvmet_passthru_override_id_ctrl;
> +			return NVME_SC_SUCCESS;
> +		case NVME_ID_CNS_NS:
> +			req->execute = nvmet_passthru_execute_cmd;
> +			req->p.end_req = nvmet_passthru_override_id_ns;
> +			return NVME_SC_SUCCESS;

Aren't you missing NVME_ID_CNS_NS_DESC_LIST? and
NVME_ID_CNS_NS_ACTIVE_LIST?

> +		default:
> +			return nvmet_setup_passthru_command(req);
> +		}

Also here, all the namespace management stuff has questionable
semantics in my mind...

> +	case nvme_admin_get_log_page:
> +		return nvmet_setup_passthru_command(req);
> +	default:
> +		/* By default, blacklist all admin commands */
> +		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +	}
> +}
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 3d5189f46cb1..e29f4b8145fa 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -858,6 +858,7 @@ enum nvme_admin_opcode {
>   	nvme_admin_security_recv	= 0x82,
>   	nvme_admin_sanitize_nvm		= 0x84,
>   	nvme_admin_get_lba_status	= 0x86,
> +	nvme_admin_vendor_start		= 0xC0,
>   };
>   
>   #define nvme_admin_opcode_name(opcode)	{ opcode, #opcode }
> 

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers
  2020-02-20 20:36 ` [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers Logan Gunthorpe
@ 2020-02-26 23:33   ` Sagi Grimberg
  2020-02-27 17:37     ` Logan Gunthorpe
  0 siblings, 1 reply; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:33 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig


> +	if (subsys->ver < NVME_VS(1, 2, 1)) {
> +		pr_warn("nvme controller version is too old: %d.%d.%d, advertising 1.2.1\n",
> +			(int)NVME_MAJOR(subsys->ver),
> +			(int)NVME_MINOR(subsys->ver),
> +			(int)NVME_TERTIARY(subsys->ver));
> +		subsys->ver = NVME_VS(1, 2, 1);

Umm.. is this OK? do we implement the mandatory 1.2.1 features on behalf
of the passthru device?

> +	}
> +
> +	mutex_unlock(&subsys->lock);
> +	return 0;
> +
> +out_put_ctrl:
> +	nvme_put_ctrl(ctrl);
> +out_unlock:
> +	mutex_unlock(&subsys->lock);
> +	return ret;
> +}
> +
> +static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
> +{
> +	if (subsys->passthru_ctrl) {
> +		xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
> +		nvme_put_ctrl(subsys->passthru_ctrl);
> +	}
> +	subsys->passthru_ctrl = NULL;
> +	subsys->ver = NVMET_DEFAULT_VS;
> +}

Isn't it strange that a subsystem changes its version in its lifetime?

> +
> +void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
> +{
> +	mutex_lock(&subsys->lock);
> +	__nvmet_passthru_ctrl_disable(subsys);
> +	mutex_unlock(&subsys->lock);
> +}
> +
> +void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
> +{
> +	mutex_lock(&subsys->lock);
> +	__nvmet_passthru_ctrl_disable(subsys);
> +	kfree(subsys->passthru_ctrl_path);
> +	mutex_unlock(&subsys->lock);

Nit, any reason why the free is in the mutex?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface
  2020-02-20 20:36 ` [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface Logan Gunthorpe
@ 2020-02-26 23:35   ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-26 23:35 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands
  2020-02-26 23:28   ` Sagi Grimberg
@ 2020-02-27 17:33     ` Logan Gunthorpe
  0 siblings, 0 replies; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-27 17:33 UTC (permalink / raw)
  To: Sagi Grimberg, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig

Thanks for the Review!

On 2020-02-26 4:28 p.m., Sagi Grimberg wrote:
> This looks questionable... There are tons of features that doesn't
> make sense here like hmb, temperature stuff, irq stuff, timestamps,
> reservations etc... passing-through these will have confusing
> semantics.. Maybe white-list what actually makes sense to passthru?

Yes, I agree a white-list here probably makes sense. I'll try to come up
with a list of features to start that whitelist, though my list might be
a bit different from yours: I don't see why temperature or timestamps
can't be passed through.

Also note: Christoph was advocating against the whitelist for the
commands, though, I agree with you that it is the most sensible approach.

>> +        break;
>> +    case nvme_admin_identify:
>> +        switch (req->cmd->identify.cns) {
>> +        case NVME_ID_CNS_CTRL:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ctrl;
>> +            return NVME_SC_SUCCESS;
>> +        case NVME_ID_CNS_NS:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ns;
>> +            return NVME_SC_SUCCESS;
> 
> Aren't you missing NVME_ID_CNS_NS_DESC_LIST? and
> NVME_ID_CNS_NS_ACTIVE_LIST?

Well no, seeing they can be passed through the default path.... But in
the light of the comment below, yes.

>> +        default:
>> +            return nvmet_setup_passthru_command(req);
>> +        }
> 
> Also here, all the namespace management stuff has questionable
> semantics in my mind...

Yes, I agree with that. I'll make the change in the next revision.

Logan

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers
  2020-02-26 23:33   ` Sagi Grimberg
@ 2020-02-27 17:37     ` Logan Gunthorpe
  2020-02-27 23:13       ` Sagi Grimberg
  0 siblings, 1 reply; 21+ messages in thread
From: Logan Gunthorpe @ 2020-02-27 17:37 UTC (permalink / raw)
  To: Sagi Grimberg, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig



On 2020-02-26 4:33 p.m., Sagi Grimberg wrote:
> 
>> +    if (subsys->ver < NVME_VS(1, 2, 1)) {
>> +        pr_warn("nvme controller version is too old: %d.%d.%d,
>> advertising 1.2.1\n",
>> +            (int)NVME_MAJOR(subsys->ver),
>> +            (int)NVME_MINOR(subsys->ver),
>> +            (int)NVME_TERTIARY(subsys->ver));
>> +        subsys->ver = NVME_VS(1, 2, 1);
> 
> Umm.. is this OK? do we implement the mandatory 1.2.1 features on behalf
> of the passthru device?

This was the approach that Christoph suggested. It seemed sensible to
me. However, it would also *probably* be ok to just reject these
devices. Unless you feel strongly about this, I'll probably leave it the
way it is.

>> +    }
>> +
>> +    mutex_unlock(&subsys->lock);
>> +    return 0;
>> +
>> +out_put_ctrl:
>> +    nvme_put_ctrl(ctrl);
>> +out_unlock:
>> +    mutex_unlock(&subsys->lock);
>> +    return ret;
>> +}
>> +
>> +static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
>> +{
>> +    if (subsys->passthru_ctrl) {
>> +        xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
>> +        nvme_put_ctrl(subsys->passthru_ctrl);
>> +    }
>> +    subsys->passthru_ctrl = NULL;
>> +    subsys->ver = NVMET_DEFAULT_VS;
>> +}
> 
> Isn't it strange that a subsystem changes its version in its lifetime?

It does seem strange. However, it's not at all unprecedented. See
nvmet_subsys_attr_version_store() which gives the user direct control of
the version through configfs.

>> +
>> +void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
>> +{
>> +    mutex_lock(&subsys->lock);
>> +    __nvmet_passthru_ctrl_disable(subsys);
>> +    mutex_unlock(&subsys->lock);
>> +}
>> +
>> +void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
>> +{
>> +    mutex_lock(&subsys->lock);
>> +    __nvmet_passthru_ctrl_disable(subsys);
>> +    kfree(subsys->passthru_ctrl_path);
>> +    mutex_unlock(&subsys->lock);
> 
> Nit, any reason why the free is in the mutex?

Nope. Will fix.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers
  2020-02-27 17:37     ` Logan Gunthorpe
@ 2020-02-27 23:13       ` Sagi Grimberg
  0 siblings, 0 replies; 21+ messages in thread
From: Sagi Grimberg @ 2020-02-27 23:13 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, linux-nvme
  Cc: Chaitanya Kulkarni, Stephen Bates, Jens Axboe, Keith Busch,
	Max Gurtovoy, Christoph Hellwig


>>> +    if (subsys->ver < NVME_VS(1, 2, 1)) {
>>> +        pr_warn("nvme controller version is too old: %d.%d.%d,
>>> advertising 1.2.1\n",
>>> +            (int)NVME_MAJOR(subsys->ver),
>>> +            (int)NVME_MINOR(subsys->ver),
>>> +            (int)NVME_TERTIARY(subsys->ver));
>>> +        subsys->ver = NVME_VS(1, 2, 1);
>>
>> Umm.. is this OK? do we implement the mandatory 1.2.1 features on behalf
>> of the passthru device?
> 
> This was the approach that Christoph suggested. It seemed sensible to
> me. However, it would also *probably* be ok to just reject these
> devices. Unless you feel strongly about this, I'll probably leave it the
> way it is.

Sounds ok to me.

>>> +    }
>>> +
>>> +    mutex_unlock(&subsys->lock);
>>> +    return 0;
>>> +
>>> +out_put_ctrl:
>>> +    nvme_put_ctrl(ctrl);
>>> +out_unlock:
>>> +    mutex_unlock(&subsys->lock);
>>> +    return ret;
>>> +}
>>> +
>>> +static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
>>> +{
>>> +    if (subsys->passthru_ctrl) {
>>> +        xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
>>> +        nvme_put_ctrl(subsys->passthru_ctrl);
>>> +    }
>>> +    subsys->passthru_ctrl = NULL;
>>> +    subsys->ver = NVMET_DEFAULT_VS;
>>> +}
>>
>> Isn't it strange that a subsystem changes its version in its lifetime?
> 
> It does seem strange. However, it's not at all unprecedented. See
> nvmet_subsys_attr_version_store() which gives the user direct control of
> the version through configfs.

You're right.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 20:36 [PATCH v11 0/9] nvmet: add target passthru commands support Logan Gunthorpe
2020-02-20 20:36 ` [PATCH v11 1/9] nvme-core: Clear any SGL flags in passthru commands Logan Gunthorpe
2020-02-26 23:08   ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 2/9] nvme: Create helper function to obtain command effects Logan Gunthorpe
2020-02-26 23:09   ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 3/9] nvme: Move nvme_passthru_[start|end]() calls to common helper Logan Gunthorpe
2020-02-26 23:10   ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 4/9] nvmet-passthru: Introduce NVMet passthru Kconfig option Logan Gunthorpe
2020-02-26 23:12   ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 5/9] nvme-core: Introduce nvme_ctrl_get_by_path() Logan Gunthorpe
2020-02-20 20:36 ` [PATCH v11 6/9] nvme: Export existing nvme core functions Logan Gunthorpe
2020-02-26 23:13   ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands Logan Gunthorpe
2020-02-26 23:28   ` Sagi Grimberg
2020-02-27 17:33     ` Logan Gunthorpe
2020-02-20 20:36 ` [PATCH v11 8/9] nvmet-passthru: Add enable/disable helpers Logan Gunthorpe
2020-02-26 23:33   ` Sagi Grimberg
2020-02-27 17:37     ` Logan Gunthorpe
2020-02-27 23:13       ` Sagi Grimberg
2020-02-20 20:36 ` [PATCH v11 9/9] nvmet-configfs: Introduce passthru configfs interface Logan Gunthorpe
2020-02-26 23:35   ` Sagi Grimberg

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git