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 <[email protected]>
---
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
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.
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.
On Thu, Oct 31, 2019, 6:39 AM Christoph Hellwig <[email protected]> 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 <[email protected]> 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.
On Thu, Oct 31, 2019 at 6:59 AM Christoph Hellwig <[email protected]> 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
----- On 31 Oct, 2019, at 14:39, Christoph Hellwig [email protected] 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
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 <[email protected]> wrote:
>
>
>
> ----- On 31 Oct, 2019, at 14:39, Christoph Hellwig [email protected] 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
----- On 4 Nov, 2019, at 15:51, Charles Machalow [email protected] 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
On Mon, Nov 04, 2019 at 03:56:57PM +0100, Marta Rybczynska wrote:
> ----- On 4 Nov, 2019, at 15:51, Charles Machalow [email protected] 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.
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 <[email protected]> wrote:
>
>
>
> ----- On 4 Nov, 2019, at 15:51, Charles Machalow [email protected] 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
----- On 4 Nov, 2019, at 16:01, Charles Machalow [email protected] 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
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 <[email protected]> wrote:
>
>
>
> ----- On 4 Nov, 2019, at 16:01, Charles Machalow [email protected] 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
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 [email protected] 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.
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.
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.