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 <[email protected]>
---
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
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.
----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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
On Mon, Aug 19, 2019 at 12:06:23AM -0700, Marta Rybczynska wrote:
> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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.
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 [email protected] 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
>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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?
On Mon, Aug 19, 2019 at 11:56:28AM -0700, Sagi Grimberg wrote:
>
> >> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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?
>>>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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...
On Mon, Aug 19, 2019 at 02:17:44PM -0700, Sagi Grimberg wrote:
>
> >>>> ----- On 16 Aug, 2019, at 15:16, Christoph Hellwig [email protected] 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.
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 [email protected] 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.
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.
----- On 22 Aug, 2019, at 02:06, Christoph Hellwig [email protected] 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
On Mon, Aug 26, 2019 at 01:20:28PM +0200, Marta Rybczynska wrote:
> Do you mean something like this?
Yes.