[PATCH] nvme: change nvme_passthru_cmd64's result field.

[PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

Changing nvme_passthru_cmd64's result field to be backwards compatible
with the nvme_passthru_cmd/nvme_admin_cmd struct in terms of the result
field. With this change the first 32 bits of result in either case
point to CQE DW0. This allows userspace tools to use the new structure
when using the old ADMIN/IO_CMD ioctls or new ADMIN/IO_CMD64 ioctls.

Signed-off-by: Charles Machalow <csm10495@gmail.com>
---
 drivers/nvme/host/core.c        | 4 ++--
 include/uapi/linux/nvme_ioctl.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fa7ba09dc..74a7cc2dd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1453,11 +1453,11 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	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);
+			0, (u64 *)&cmd.result, timeout);
 	nvme_passthru_end(ctrl, effects);
 
 	if (status >= 0) {
-		if (put_user(cmd.result, &ucmd->result))
+		if (put_user(*(u64 *)&cmd.result, (u64 *)&ucmd->result))
 			return -EFAULT;
 	}
 
diff --git a/include/uapi/linux/nvme_ioctl.h b/include/uapi/linux/nvme_ioctl.h
index e168dc59e..4cb07bd6d 100644
--- a/include/uapi/linux/nvme_ioctl.h
+++ b/include/uapi/linux/nvme_ioctl.h
@@ -63,7 +63,7 @@ struct nvme_passthru_cmd64 {
 	__u32	cdw14;
 	__u32	cdw15;
 	__u32	timeout_ms;
-	__u64	result;
+	__u32	result[2];
 };
 
 #define nvme_admin_cmd nvme_passthru_cmd
-- 
2.17.1

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Christoph Hellwig]

On Wed, Oct 30, 2019 at 10:03:38PM -0700, Charles Machalow wrote:
> Changing nvme_passthru_cmd64's result field to be backwards compatible
> with the nvme_passthru_cmd/nvme_admin_cmd struct in terms of the result
> field. With this change the first 32 bits of result in either case
> point to CQE DW0. This allows userspace tools to use the new structure
> when using the old ADMIN/IO_CMD ioctls or new ADMIN/IO_CMD64 ioctls.

All that casting is a pretty bad idea.  please just add an explicit
reserved field before the result, and check that it always is zero
in the ioctl handler.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Christoph Hellwig]

On Thu, Oct 31, 2019 at 06:55:33AM -0700, Charles Machalow wrote:
> Not quite sure what you mean by check for zero in the ioctl handler. I like
> the idea of being able to use the same struct for either the original or 64
> ioctls from userspace. I don't believe adding the explicit rsvd field
> allows that.

You might like the idea, but it fundamentally is a bad idea.  For example
you completely break architectures that do not support unaligned loads
and stores.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

On Thu, Oct 31, 2019, 6:39 AM Christoph Hellwig <hch@lst.de> wrote

> All that casting is a pretty bad idea.  please just add an explicit
> reserved field before the result, and check that it always is zero
> in the ioctl handler.

Not quite sure what you mean by check for zero in the ioctl handler. I
like the idea of being able to use the same struct for either the
original or 64 ioctls from userspace. I don't believe adding the
explicit rsvd field allows that

- Charlie Scott Machalow

On Thu, Oct 31, 2019 at 6:39 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Oct 30, 2019 at 10:03:38PM -0700, Charles Machalow wrote:
> > Changing nvme_passthru_cmd64's result field to be backwards compatible
> > with the nvme_passthru_cmd/nvme_admin_cmd struct in terms of the result
> > field. With this change the first 32 bits of result in either case
> > point to CQE DW0. This allows userspace tools to use the new structure
> > when using the old ADMIN/IO_CMD ioctls or new ADMIN/IO_CMD64 ioctls.
>
> All that casting is a pretty bad idea.  please just add an explicit
> reserved field before the result, and check that it always is zero
> in the ioctl handler.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

On Thu, Oct 31, 2019 at 6:59 AM Christoph Hellwig <hch@lst.de> wrote:
> You might like the idea, but it fundamentally is a bad idea.  For example
> you completely break architectures that do not support unaligned loads
> and stores.

Would you also be against the idea of memcpy the u32 array into a u64 then
passing a pointer to that local variable lower?

- Charlie Scott Machalow

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Marta Rybczynska]

----- On 31 Oct, 2019, at 14:39, Christoph Hellwig hch@lst.de wrote:

> On Wed, Oct 30, 2019 at 10:03:38PM -0700, Charles Machalow wrote:
>> Changing nvme_passthru_cmd64's result field to be backwards compatible
>> with the nvme_passthru_cmd/nvme_admin_cmd struct in terms of the result
>> field. With this change the first 32 bits of result in either case
>> point to CQE DW0. This allows userspace tools to use the new structure
>> when using the old ADMIN/IO_CMD ioctls or new ADMIN/IO_CMD64 ioctls.
> 
> All that casting is a pretty bad idea.  please just add an explicit
> reserved field before the result, and check that it always is zero
> in the ioctl handler.

That would change the size of a structure in UAPI, won't it? 
I wanted to avoid it when adding the *64 ioctls and that's why
I added separate ones instead of extending the ones that exist.

Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

For this one yes, UAPI size changes. Though I believe this IOCTL
hasn't been in a released Kernel yet (just RC). Technically it may be
changeable as a fix until the next Kernel is released. I do think its
a useful enough
change to warrant a late fix.

- Charlie Scott Machalow

On Mon, Nov 4, 2019 at 6:34 AM Marta Rybczynska <mrybczyn@kalray.eu> wrote:
>
>
>
> ----- On 31 Oct, 2019, at 14:39, Christoph Hellwig hch@lst.de wrote:
>
> > On Wed, Oct 30, 2019 at 10:03:38PM -0700, Charles Machalow wrote:
> >> Changing nvme_passthru_cmd64's result field to be backwards compatible
> >> with the nvme_passthru_cmd/nvme_admin_cmd struct in terms of the result
> >> field. With this change the first 32 bits of result in either case
> >> point to CQE DW0. This allows userspace tools to use the new structure
> >> when using the old ADMIN/IO_CMD ioctls or new ADMIN/IO_CMD64 ioctls.
> >
> > All that casting is a pretty bad idea.  please just add an explicit
> > reserved field before the result, and check that it always is zero
> > in the ioctl handler.
>
> That would change the size of a structure in UAPI, won't it?
> I wanted to avoid it when adding the *64 ioctls and that's why
> I added separate ones instead of extending the ones that exist.
>
> Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Marta Rybczynska]

----- On 4 Nov, 2019, at 15:51, Charles Machalow csm10495@gmail.com wrote:

> For this one yes, UAPI size changes. Though I believe this IOCTL
> hasn't been in a released Kernel yet (just RC). Technically it may be
> changeable as a fix until the next Kernel is released. I do think its
> a useful enough
> change to warrant a late fix.

The old one is in UAPI for years. The new one is not yet, right. I'm OK
to change the new structure. To have compatibility you would have to use
the new structure (at least its size) in the user space code. This is
what you'd liek to do?

Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Keith Busch]

On Mon, Nov 04, 2019 at 03:56:57PM +0100, Marta Rybczynska wrote:
> ----- On 4 Nov, 2019, at 15:51, Charles Machalow csm10495@gmail.com wrote:
> 
> > For this one yes, UAPI size changes. Though I believe this IOCTL
> > hasn't been in a released Kernel yet (just RC). Technically it may be
> > changeable as a fix until the next Kernel is released. I do think its
> > a useful enough
> > change to warrant a late fix.
> 
> The old one is in UAPI for years. The new one is not yet, right. I'm OK
> to change the new structure. To have compatibility you would have to use
> the new structure (at least its size) in the user space code. This is
> what you'd liek to do?

Charles is proposing only to modify the recently introduced 64-bit ioctl
struct without touching the existing 32 bit version. He just wanted the
lower 32-bits of the 64-bit result to occupy the same space as the 32-bit
ioctl's result. That space in the 64-bit version is currently occupied
by an implicit struct padding.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

Yes. The idea is just to change the 64 IOCTL structure so it lines up
with the old ones so that the same struct can be used from userspace.
Right now the first 32 of 64's result doesn't line up with the old
result field.

- Charlie Scott Machalow

On Mon, Nov 4, 2019 at 6:56 AM Marta Rybczynska <mrybczyn@kalray.eu> wrote:
>
>
>
> ----- On 4 Nov, 2019, at 15:51, Charles Machalow csm10495@gmail.com wrote:
>
> > For this one yes, UAPI size changes. Though I believe this IOCTL
> > hasn't been in a released Kernel yet (just RC). Technically it may be
> > changeable as a fix until the next Kernel is released. I do think its
> > a useful enough
> > change to warrant a late fix.
>
> The old one is in UAPI for years. The new one is not yet, right. I'm OK
> to change the new structure. To have compatibility you would have to use
> the new structure (at least its size) in the user space code. This is
> what you'd liek to do?
>
> Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Marta Rybczynska]

----- On 4 Nov, 2019, at 16:01, Charles Machalow csm10495@gmail.com wrote:

> Yes. The idea is just to change the 64 IOCTL structure so it lines up
> with the old ones so that the same struct can be used from userspace.
> Right now the first 32 of 64's result doesn't line up with the old
> result field.
> 
> - Charlie Scott Machalow

OK, then this will work on all architectures I know:

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;
        __u32   rsvd2;
	__u64	result;
};

Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Charles Machalow]

The thing with that structure is if you use it with the old IOCTL, the
result will go into rsvd2 instead of the first 32 bits of result.

- Charlie Scott Machalow

On Mon, Nov 4, 2019 at 7:16 AM Marta Rybczynska <mrybczyn@kalray.eu> wrote:
>
>
>
> ----- On 4 Nov, 2019, at 16:01, Charles Machalow csm10495@gmail.com wrote:
>
> > Yes. The idea is just to change the 64 IOCTL structure so it lines up
> > with the old ones so that the same struct can be used from userspace.
> > Right now the first 32 of 64's result doesn't line up with the old
> > result field.
> >
> > - Charlie Scott Machalow
>
> OK, then this will work on all architectures I know:
>
> 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;
>         __u32   rsvd2;
>         __u64   result;
> };
>
> Marta

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Christoph Hellwig]

On Tue, Nov 05, 2019 at 12:01:51AM +0900, Keith Busch wrote:
> On Mon, Nov 04, 2019 at 03:56:57PM +0100, Marta Rybczynska wrote:
> > ----- On 4 Nov, 2019, at 15:51, Charles Machalow csm10495@gmail.com wrote:
> > 
> > > For this one yes, UAPI size changes. Though I believe this IOCTL
> > > hasn't been in a released Kernel yet (just RC). Technically it may be
> > > changeable as a fix until the next Kernel is released. I do think its
> > > a useful enough
> > > change to warrant a late fix.
> > 
> > The old one is in UAPI for years. The new one is not yet, right. I'm OK
> > to change the new structure. To have compatibility you would have to use
> > the new structure (at least its size) in the user space code. This is
> > what you'd liek to do?
> 
> Charles is proposing only to modify the recently introduced 64-bit ioctl
> struct without touching the existing 32 bit version. He just wanted the
> lower 32-bits of the 64-bit result to occupy the same space as the 32-bit
> ioctl's result. That space in the 64-bit version is currently occupied
> by an implicit struct padding.

Except on 32-bit x86, which does not have the padding.  Which is why
the current layout is so bad, as it breaks 32-it x86 compat.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Christoph Hellwig]

On Mon, Nov 04, 2019 at 07:20:43AM -0800, Charles Machalow wrote:
> The thing with that structure is if you use it with the old IOCTL, the
> result will go into rsvd2 instead of the first 32 bits of result.

But if you use the old ioctls on the new structure you can at least
expect that.  And with the added explicit padding it will at least
do the right thing on 32-bit x86 as well.

Re: [PATCH] nvme: change nvme_passthru_cmd64's result field.

[Keith Busch]

On Mon, Nov 04, 2019 at 04:29:16PM +0100, Christoph Hellwig wrote:
> 
> Except on 32-bit x86, which does not have the padding.  Which is why
> the current layout is so bad, as it breaks 32-it x86 compat.

Yeah, fair enough.

Charles, let's just define an explicit padding rsvd field and use the
appropriate struct for the different ioctl's.