linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] nvme: allow 64-bit results in passthru commands
@ 2019-08-16  9:47 Marta Rybczynska
  2019-08-16 13:16 ` Christoph Hellwig
  2019-08-22  0:06 ` Christoph Hellwig
  0 siblings, 2 replies; 13+ messages in thread
From: Marta Rybczynska @ 2019-08-16  9:47 UTC (permalink / raw)
  To: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel
  Cc: Samuel Jones, Guillaume Missonnier

It is not possible to get 64-bit results from the passthru commands,
what prevents from getting for the Capabilities (CAP) property value.

As a result, it is not possible to implement IOL's NVMe Conformance
test 4.3 Case 1 for Fabrics targets [1] (page 123).

This issue has been already discussed [2], but without a solution.

This patch solves the problem by adding new ioctls with a new
passthru structure, including 64-bit results. The older ioctls stay
unchanged.

[1] https://www.iol.unh.edu/sites/default/files/testsuites/nvme/UNH-IOL_NVMe_Conformance_Test_Suite_v11.0.pdf
[2] http://lists.infradead.org/pipermail/linux-nvme/2018-June/018791.html

Signed-off-by: Marta Rybczynska <marta.rybczynska@kalray.eu>
---
 drivers/nvme/host/core.c        | 66 ++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/nvme_ioctl.h | 23 ++++++++++++++
 2 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8f3fbe5..2ddb095 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -849,7 +849,7 @@ static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 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,
-		u32 meta_seed, u32 *result, unsigned timeout)
+		u32 meta_seed, u64 *result, unsigned timeout)
 {
 	bool write = nvme_is_write(cmd);
 	struct nvme_ns *ns = q->queuedata;
@@ -890,7 +890,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	else
 		ret = nvme_req(req)->status;
 	if (result)
-		*result = le32_to_cpu(nvme_req(req)->result.u32);
+		*result = le64_to_cpu(nvme_req(req)->result.u64);
 	if (meta && !ret && !write) {
 		if (copy_to_user(meta_buffer, meta, meta_len))
 			ret = -EFAULT;
@@ -1331,6 +1331,54 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct nvme_command c;
 	unsigned timeout = 0;
 	u32 effects;
+	u64 result;
+	int status;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
+		return -EFAULT;
+	if (cmd.flags)
+		return -EINVAL;
+
+	memset(&c, 0, sizeof(c));
+	c.common.opcode = cmd.opcode;
+	c.common.flags = cmd.flags;
+	c.common.nsid = cpu_to_le32(cmd.nsid);
+	c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
+	c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
+	c.common.cdw10 = cpu_to_le32(cmd.cdw10);
+	c.common.cdw11 = cpu_to_le32(cmd.cdw11);
+	c.common.cdw12 = cpu_to_le32(cmd.cdw12);
+	c.common.cdw13 = cpu_to_le32(cmd.cdw13);
+	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
+	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
+
+	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))
+			return -EFAULT;
+	}
+
+	return status;
+}
+
+static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			struct nvme_passthru_cmd64 __user *ucmd)
+{
+	struct nvme_passthru_cmd64 cmd;
+	struct nvme_command c;
+	unsigned timeout = 0;
+	u32 effects;
 	int status;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1401,6 +1449,13 @@ static void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx)
 		srcu_read_unlock(&head->srcu, idx);
 }
 
+static bool is_admin_cmd(unsigned int cmd)
+{
+	if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
+		return true;
+	return false;
+}
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
@@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	 * seperately and drop the ns SRCU reference early.  This avoids a
 	 * deadlock when deleting namespaces using the passthrough interface.
 	 */
-	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
+	if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {
 		struct nvme_ctrl *ctrl = ns->ctrl;
 
 		nvme_get_ctrl(ns->ctrl);
 		nvme_put_ns_from_disk(head, srcu_idx);
 
-		if (cmd == NVME_IOCTL_ADMIN_CMD)
+		if (is_admin_cmd(cmd))
 			ret = nvme_user_cmd(ctrl, NULL, argp);
 		else
 			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
@@ -1444,6 +1499,9 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	case NVME_IOCTL_SUBMIT_IO:
 		ret = nvme_submit_io(ns, argp);
 		break;
+	case NVME_IOCTL_IO64_CMD:
+		ret = nvme_user_cmd64(ns->ctrl, ns, argp);
+		break;
 	default:
 		if (ns->ndev)
 			ret = nvme_nvm_ioctl(ns, cmd, arg);
diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index 1c215ea..e168dc5 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -45,6 +45,27 @@ struct nvme_passthru_cmd {
 	__u32	result;
 };
 
+struct nvme_passthru_cmd64 {
+	__u8	opcode;
+	__u8	flags;
+	__u16	rsvd1;
+	__u32	nsid;
+	__u32	cdw2;
+	__u32	cdw3;
+	__u64	metadata;
+	__u64	addr;
+	__u32	metadata_len;
+	__u32	data_len;
+	__u32	cdw10;
+	__u32	cdw11;
+	__u32	cdw12;
+	__u32	cdw13;
+	__u32	cdw14;
+	__u32	cdw15;
+	__u32	timeout_ms;
+	__u64	result;
+};
+
 #define nvme_admin_cmd nvme_passthru_cmd
 
 #define NVME_IOCTL_ID		_IO('N', 0x40)
@@ -54,5 +75,7 @@ struct nvme_passthru_cmd {
 #define NVME_IOCTL_RESET	_IO('N', 0x44)
 #define NVME_IOCTL_SUBSYS_RESET	_IO('N', 0x45)
 #define NVME_IOCTL_RESCAN	_IO('N', 0x46)
+#define NVME_IOCTL_ADMIN64_CMD	_IOWR('N', 0x47, struct nvme_passthru_cmd64)
+#define NVME_IOCTL_IO64_CMD	_IOWR('N', 0x48, struct nvme_passthru_cmd64)
 
 #endif /* _UAPI_LINUX_NVME_IOCTL_H */
-- 
1.8.3.1


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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-16  9:47 [PATCH v2] nvme: allow 64-bit results in passthru commands Marta Rybczynska
@ 2019-08-16 13:16 ` Christoph Hellwig
  2019-08-19  7:06   ` Marta Rybczynska
  2019-08-22  0:06 ` Christoph Hellwig
  1 sibling, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2019-08-16 13:16 UTC (permalink / raw)
  To: Marta Rybczynska
  Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Samuel Jones,
	Guillaume Missonnier

Sorry for not replying to the earlier version, and thanks for doing
this work.

I wonder if instead of using our own structure we'd just use
a full nvme SQE for the input and CQE for that output.  Even if we
reserve a few fields that means we are ready for any newly used
field (at least until the SQE/CQE sizes are expanded..).

On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
> It is not possible to get 64-bit results from the passthru commands,
> what prevents from getting for the Capabilities (CAP) property value.
> 
> As a result, it is not possible to implement IOL's NVMe Conformance
> test 4.3 Case 1 for Fabrics targets [1] (page 123).

Not that I'm not sure passing through fabrics commands is an all that
good idea.  But we have pending NVMe TPs that use 64-bit result
values as well, so this seems like a good idea in general.

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-16 13:16 ` Christoph Hellwig
@ 2019-08-19  7:06   ` Marta Rybczynska
  2019-08-19 14:49     ` Keith Busch
  0 siblings, 1 reply; 13+ messages in thread
From: Marta Rybczynska @ 2019-08-19  7:06 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: kbusch, axboe, Sagi Grimberg, linux-nvme, linux-kernel,
	Samuel Jones, Guillaume Missonnier



----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:

> Sorry for not replying to the earlier version, and thanks for doing
> this work.
> 
> I wonder if instead of using our own structure we'd just use
> a full nvme SQE for the input and CQE for that output.  Even if we
> reserve a few fields that means we are ready for any newly used
> field (at least until the SQE/CQE sizes are expanded..).

We could do that, nvme_command and nvme_completion are already UAPI.
On the other hand that would mean not filling out certain fields like
command_id. Can do an approach like this.
> 
> On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
>> It is not possible to get 64-bit results from the passthru commands,
>> what prevents from getting for the Capabilities (CAP) property value.
>> 
>> As a result, it is not possible to implement IOL's NVMe Conformance
>> test 4.3 Case 1 for Fabrics targets [1] (page 123).
> 
> Not that I'm not sure passing through fabrics commands is an all that
> good idea.  But we have pending NVMe TPs that use 64-bit result
> values as well, so this seems like a good idea in general.

I'm not sure how those tests could be implemented otherwise today.
The goal is to send an arbitrary command to the target and the passthru
command is AFAIK the only solution. If we have something better
I'm sure they will be ready to use it.

Regards,
Marta

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19  7:06   ` Marta Rybczynska
@ 2019-08-19 14:49     ` Keith Busch
  2019-08-19 15:57       ` James Smart
                         ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Keith Busch @ 2019-08-19 14:49 UTC (permalink / raw)
  To: Marta Rybczynska
  Cc: Christoph Hellwig, axboe, Sagi Grimberg, linux-nvme,
	linux-kernel, Samuel Jones, Guillaume Missonnier

On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote:
> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
> > Sorry for not replying to the earlier version, and thanks for doing
> > this work.
> > 
> > I wonder if instead of using our own structure we'd just use
> > a full nvme SQE for the input and CQE for that output.  Even if we
> > reserve a few fields that means we are ready for any newly used
> > field (at least until the SQE/CQE sizes are expanded..).
> 
> We could do that, nvme_command and nvme_completion are already UAPI.
> On the other hand that would mean not filling out certain fields like
> command_id. Can do an approach like this.

Well, we need to pass user space addresses and lengths, which isn't
captured in struct nvme_command.

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 14:49     ` Keith Busch
@ 2019-08-19 15:57       ` James Smart
  2019-08-19 18:56       ` Sagi Grimberg
  2019-08-21 23:59       ` Christoph Hellwig
  2 siblings, 0 replies; 13+ messages in thread
From: James Smart @ 2019-08-19 15:57 UTC (permalink / raw)
  To: Keith Busch, Marta Rybczynska
  Cc: Sagi Grimberg, Samuel Jones, Guillaume Missonnier, linux-kernel,
	linux-nvme, axboe, Christoph Hellwig



On 8/19/2019 7:49 AM, Keith Busch wrote:
> On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote:
>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
>>> Sorry for not replying to the earlier version, and thanks for doing
>>> this work.
>>>
>>> I wonder if instead of using our own structure we'd just use
>>> a full nvme SQE for the input and CQE for that output.  Even if we
>>> reserve a few fields that means we are ready for any newly used
>>> field (at least until the SQE/CQE sizes are expanded..).
>> We could do that, nvme_command and nvme_completion are already UAPI.
>> On the other hand that would mean not filling out certain fields like
>> command_id. Can do an approach like this.
> Well, we need to pass user space addresses and lengths, which isn't
> captured in struct nvme_command.
>
This is going to be fun.  It's going to have to be a cooperative effort 
between app and transport. There will always need to be some parts of 
the SQE filled out by the transport like SGL, command type/subtype bits, 
as well as dealing with buffers as Keith states. Command ID is another 
of those fields.

-- james



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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 14:49     ` Keith Busch
  2019-08-19 15:57       ` James Smart
@ 2019-08-19 18:56       ` Sagi Grimberg
  2019-08-19 18:57         ` Keith Busch
  2019-08-21 23:59       ` Christoph Hellwig
  2 siblings, 1 reply; 13+ messages in thread
From: Sagi Grimberg @ 2019-08-19 18:56 UTC (permalink / raw)
  To: Keith Busch, Marta Rybczynska
  Cc: Christoph Hellwig, axboe, linux-nvme, linux-kernel, Samuel Jones,
	Guillaume Missonnier


>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
>>> Sorry for not replying to the earlier version, and thanks for doing
>>> this work.
>>>
>>> I wonder if instead of using our own structure we'd just use
>>> a full nvme SQE for the input and CQE for that output.  Even if we
>>> reserve a few fields that means we are ready for any newly used
>>> field (at least until the SQE/CQE sizes are expanded..).
>>
>> We could do that, nvme_command and nvme_completion are already UAPI.
>> On the other hand that would mean not filling out certain fields like
>> command_id. Can do an approach like this.
> 
> Well, we need to pass user space addresses and lengths, which isn't
> captured in struct nvme_command.

Isn't simply having a 64 variant simpler?

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 18:56       ` Sagi Grimberg
@ 2019-08-19 18:57         ` Keith Busch
  2019-08-19 21:17           ` Sagi Grimberg
  0 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2019-08-19 18:57 UTC (permalink / raw)
  To: Sagi Grimberg
  Cc: Marta Rybczynska, Christoph Hellwig, axboe, linux-nvme,
	linux-kernel, Samuel Jones, Guillaume Missonnier

On Mon, Aug 19, 2019 at 11:56:28AM -0700, Sagi Grimberg wrote:
> 
> >> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
> >>> Sorry for not replying to the earlier version, and thanks for doing
> >>> this work.
> >>>
> >>> I wonder if instead of using our own structure we'd just use
> >>> a full nvme SQE for the input and CQE for that output.  Even if we
> >>> reserve a few fields that means we are ready for any newly used
> >>> field (at least until the SQE/CQE sizes are expanded..).
> >>
> >> We could do that, nvme_command and nvme_completion are already UAPI.
> >> On the other hand that would mean not filling out certain fields like
> >> command_id. Can do an approach like this.
> > 
> > Well, we need to pass user space addresses and lengths, which isn't
> > captured in struct nvme_command.
> 
> Isn't simply having a 64 variant simpler?

Could you provide more details on what you mean by this?

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 18:57         ` Keith Busch
@ 2019-08-19 21:17           ` Sagi Grimberg
  2019-08-19 21:21             ` Keith Busch
  0 siblings, 1 reply; 13+ messages in thread
From: Sagi Grimberg @ 2019-08-19 21:17 UTC (permalink / raw)
  To: Keith Busch
  Cc: Marta Rybczynska, Christoph Hellwig, axboe, linux-nvme,
	linux-kernel, Samuel Jones, Guillaume Missonnier


>>>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
>>>>> Sorry for not replying to the earlier version, and thanks for doing
>>>>> this work.
>>>>>
>>>>> I wonder if instead of using our own structure we'd just use
>>>>> a full nvme SQE for the input and CQE for that output.  Even if we
>>>>> reserve a few fields that means we are ready for any newly used
>>>>> field (at least until the SQE/CQE sizes are expanded..).
>>>>
>>>> We could do that, nvme_command and nvme_completion are already UAPI.
>>>> On the other hand that would mean not filling out certain fields like
>>>> command_id. Can do an approach like this.
>>>
>>> Well, we need to pass user space addresses and lengths, which isn't
>>> captured in struct nvme_command.
>>
>> Isn't simply having a 64 variant simpler?
> 
> Could you provide more details on what you mean by this?

Why would we need to pass addresses and lengths if userspace is
sending the 64 variant when it is expecting a 64 result?

Or maybe I'm missing something...

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 21:17           ` Sagi Grimberg
@ 2019-08-19 21:21             ` Keith Busch
  0 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2019-08-19 21:21 UTC (permalink / raw)
  To: Sagi Grimberg
  Cc: Marta Rybczynska, Christoph Hellwig, axboe, linux-nvme,
	linux-kernel, Samuel Jones, Guillaume Missonnier

On Mon, Aug 19, 2019 at 02:17:44PM -0700, Sagi Grimberg wrote:
> 
> >>>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
> >>>>> Sorry for not replying to the earlier version, and thanks for doing
> >>>>> this work.
> >>>>>
> >>>>> I wonder if instead of using our own structure we'd just use
> >>>>> a full nvme SQE for the input and CQE for that output.  Even if we
> >>>>> reserve a few fields that means we are ready for any newly used
> >>>>> field (at least until the SQE/CQE sizes are expanded..).
> >>>>
> >>>> We could do that, nvme_command and nvme_completion are already UAPI.
> >>>> On the other hand that would mean not filling out certain fields like
> >>>> command_id. Can do an approach like this.
> >>>
> >>> Well, we need to pass user space addresses and lengths, which isn't
> >>> captured in struct nvme_command.
> >>
> >> Isn't simply having a 64 variant simpler?
> > 
> > Could you provide more details on what you mean by this?
> 
> Why would we need to pass addresses and lengths if userspace is
> sending the 64 variant when it is expecting a 64 result?
> 
> Or maybe I'm missing something...

The recommendation was to have user space provide an SQE, i.e. 'struct
nvme_command', as input to the driver and receive 'struct nvme_completion'
in response. I am only pointing out that 'struct nvme_command' is
inappropriate for user space.

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-19 14:49     ` Keith Busch
  2019-08-19 15:57       ` James Smart
  2019-08-19 18:56       ` Sagi Grimberg
@ 2019-08-21 23:59       ` Christoph Hellwig
  2 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2019-08-21 23:59 UTC (permalink / raw)
  To: Keith Busch
  Cc: Marta Rybczynska, Christoph Hellwig, axboe, Sagi Grimberg,
	linux-nvme, linux-kernel, Samuel Jones, Guillaume Missonnier

On Mon, Aug 19, 2019 at 08:49:22AM -0600, Keith Busch wrote:
> On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote:
> > ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig hch@lst.de wrote:
> > > Sorry for not replying to the earlier version, and thanks for doing
> > > this work.
> > > 
> > > I wonder if instead of using our own structure we'd just use
> > > a full nvme SQE for the input and CQE for that output.  Even if we
> > > reserve a few fields that means we are ready for any newly used
> > > field (at least until the SQE/CQE sizes are expanded..).
> > 
> > We could do that, nvme_command and nvme_completion are already UAPI.
> > On the other hand that would mean not filling out certain fields like
> > command_id. Can do an approach like this.
> 
> Well, we need to pass user space addresses and lengths, which isn't
> captured in struct nvme_command.

Well, the address would fit into the data pointer.  But yes, the lack
of a command length concept in nvme makes this idea a mess and not
really workable.

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-16  9:47 [PATCH v2] nvme: allow 64-bit results in passthru commands Marta Rybczynska
  2019-08-16 13:16 ` Christoph Hellwig
@ 2019-08-22  0:06 ` Christoph Hellwig
  2019-08-26 11:20   ` Marta Rybczynska
  1 sibling, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2019-08-22  0:06 UTC (permalink / raw)
  To: Marta Rybczynska
  Cc: kbusch, axboe, hch, sagi, linux-nvme, linux-kernel, Samuel Jones,
	Guillaume Missonnier

On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
> It is not possible to get 64-bit results from the passthru commands,
> what prevents from getting for the Capabilities (CAP) property value.
> 
> As a result, it is not possible to implement IOL's NVMe Conformance
> test 4.3 Case 1 for Fabrics targets [1] (page 123).
> 
> This issue has been already discussed [2], but without a solution.
> 
> This patch solves the problem by adding new ioctls with a new
> passthru structure, including 64-bit results. The older ioctls stay
> unchanged.

Ok, with my idea not being suitable I think I'm fine with this approach, a
little nitpick below:

> +static bool is_admin_cmd(unsigned int cmd)
> +{
> +	if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
> +		return true;
> +	return false;
> +}

No need for the inner braces.  But I'm actually not sure the current
code structure is very suitable for extending it.

> +
>  static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>  		unsigned int cmd, unsigned long arg)
>  {
> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>  	 * seperately and drop the ns SRCU reference early.  This avoids a
>  	 * deadlock when deleting namespaces using the passthrough interface.
>  	 */
> -	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
> +	if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {

So maybe for this check we should have a is_ctrl_iocl() helper instead
that includes the is_sed_ioctl check.

>  		struct nvme_ctrl *ctrl = ns->ctrl;
>  
>  		nvme_get_ctrl(ns->ctrl);
>  		nvme_put_ns_from_disk(head, srcu_idx);
>  
> -		if (cmd == NVME_IOCTL_ADMIN_CMD)
> +		if (is_admin_cmd(cmd))
>  			ret = nvme_user_cmd(ctrl, NULL, argp);
>  		else
>  			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);

And then we can move this whole branch into a helper function,
which then switches on the ioctl cmd, with sed_ioctl as the fallback.

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-22  0:06 ` Christoph Hellwig
@ 2019-08-26 11:20   ` Marta Rybczynska
  2019-09-02  8:29     ` Christoph Hellwig
  0 siblings, 1 reply; 13+ messages in thread
From: Marta Rybczynska @ 2019-08-26 11:20 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: kbusch, axboe, Sagi Grimberg, linux-nvme, linux-kernel,
	Samuel Jones, Guillaume Missonnier



----- On 22 Aug, 2019, at 02:06, Christoph Hellwig hch@lst.de wrote:

> On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
>> It is not possible to get 64-bit results from the passthru commands,
>> what prevents from getting for the Capabilities (CAP) property value.
>> 
>> As a result, it is not possible to implement IOL's NVMe Conformance
>> test 4.3 Case 1 for Fabrics targets [1] (page 123).
>> 
>> This issue has been already discussed [2], but without a solution.
>> 
>> This patch solves the problem by adding new ioctls with a new
>> passthru structure, including 64-bit results. The older ioctls stay
>> unchanged.
> 
> Ok, with my idea not being suitable I think I'm fine with this approach, a
> little nitpick below:
> 
>> +static bool is_admin_cmd(unsigned int cmd)
>> +{
>> +	if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
>> +		return true;
>> +	return false;
>> +}
> 
> No need for the inner braces.  But I'm actually not sure the current
> code structure is very suitable for extending it.
> 
>> +
>>  static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>>  		unsigned int cmd, unsigned long arg)
>>  {
>> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t
>> mode,
>>  	 * seperately and drop the ns SRCU reference early.  This avoids a
>>  	 * deadlock when deleting namespaces using the passthrough interface.
>>  	 */
>> -	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
>> +	if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {
> 
> So maybe for this check we should have a is_ctrl_iocl() helper instead
> that includes the is_sed_ioctl check.
> 
>>  		struct nvme_ctrl *ctrl = ns->ctrl;
>>  
>>  		nvme_get_ctrl(ns->ctrl);
>>  		nvme_put_ns_from_disk(head, srcu_idx);
>>  
>> -		if (cmd == NVME_IOCTL_ADMIN_CMD)
>> +		if (is_admin_cmd(cmd))
>>  			ret = nvme_user_cmd(ctrl, NULL, argp);
>>  		else
>>  			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
> 
> And then we can move this whole branch into a helper function,
> which then switches on the ioctl cmd, with sed_ioctl as the fallback.

Do you mean something like this?

+static bool is_ctrl_ioctl(unsigned int cmd)
+{
+	if (cmd == NVME_IOCTL_ADMIN_CMD || cmd == NVME_IOCTL_ADMIN64_CMD)
+		return true;
+	if (is_sed_ioctl(cmd))
+		return true;
+	return false;
+}
+
+static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
+				  void __user *argp,
+				  struct nvme_ns_head *head,
+				  int srcu_idx)
+{
+	struct nvme_ctrl *ctrl = ns->ctrl;
+	int ret;
+
+	nvme_get_ctrl(ns->ctrl);
+	nvme_put_ns_from_disk(head, srcu_idx);
+
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		ret = nvme_user_cmd(ctrl, NULL, argp);
+		break;
+	case NVME_IOCTL_ADMIN64_CMD:
+		ret = nvme_user_cmd64(ctrl, NULL, argp);
+		break;
+	default:
+		ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
+		break;
+	}
+	nvme_put_ctrl(ctrl);
+	return ret;
+}
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
@@ -1418,20 +1501,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	 * seperately and drop the ns SRCU reference early.  This avoids a
 	 * deadlock when deleting namespaces using the passthrough interface.
 	 */
-	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
-		struct nvme_ctrl *ctrl = ns->ctrl;
-
-		nvme_get_ctrl(ns->ctrl);
-		nvme_put_ns_from_disk(head, srcu_idx);
-
-		if (cmd == NVME_IOCTL_ADMIN_CMD)
-			ret = nvme_user_cmd(ctrl, NULL, argp);
-		else
-			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
-
-		nvme_put_ctrl(ctrl);
-		return ret;
-	}
+	if (is_ctrl_ioctl(cmd))
+		return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
 
 	switch (cmd) {
 	case NVME_IOCTL_ID:

Regards,
Marta

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

* Re: [PATCH v2] nvme: allow 64-bit results in passthru commands
  2019-08-26 11:20   ` Marta Rybczynska
@ 2019-09-02  8:29     ` Christoph Hellwig
  0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2019-09-02  8:29 UTC (permalink / raw)
  To: Marta Rybczynska
  Cc: Christoph Hellwig, kbusch, axboe, Sagi Grimberg, linux-nvme,
	linux-kernel, Samuel Jones, Guillaume Missonnier

On Mon, Aug 26, 2019 at 01:20:28PM +0200, Marta Rybczynska wrote:
> Do you mean something like this?

Yes.

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

end of thread, other threads:[~2019-09-02  8:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16  9:47 [PATCH v2] nvme: allow 64-bit results in passthru commands Marta Rybczynska
2019-08-16 13:16 ` Christoph Hellwig
2019-08-19  7:06   ` Marta Rybczynska
2019-08-19 14:49     ` Keith Busch
2019-08-19 15:57       ` James Smart
2019-08-19 18:56       ` Sagi Grimberg
2019-08-19 18:57         ` Keith Busch
2019-08-19 21:17           ` Sagi Grimberg
2019-08-19 21:21             ` Keith Busch
2019-08-21 23:59       ` Christoph Hellwig
2019-08-22  0:06 ` Christoph Hellwig
2019-08-26 11:20   ` Marta Rybczynska
2019-09-02  8:29     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).