| Message ID | 20181130150050.13762-3-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: uapi updates | expand |
On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote: > > - > -#define MSM_INFO_FLAGS (MSM_INFO_IOVA) > +/* Get or set GEM buffer info. The requested value can be passed > + * directly in 'value', or for data larger than 64b 'value' is a > + * pointer to userspace buffer, with 'len' specifying the number of > + * bytes copied into that buffer. For info returned by pointer, > + * calling the GEM_INFO ioctl with null 'value' will return the > + * required buffer size in 'len' > + */ > +#define MSM_INFO_GET_OFFSET 0x00 /* get mmap() offset, returned by value */ > +#define MSM_INFO_GET_IOVA 0x01 /* get iova, returned by value */ > > struct drm_msm_gem_info { > __u32 handle; /* in */ > - __u32 flags; /* in - combination of MSM_INFO_* flags */ > - __u64 offset; /* out, mmap() offset or iova */ > + __u32 info; /* in - one of MSM_INFO_* */ > + __u64 value; /* in or out */ > + __u32 len; /* in or out */ > }; As structure with implicit padding has the problem of possibly leaking kernel stack data. It's better to make the padding explicit here so you can zero it from the kernel. Also, as I mentioned in the other patch, you probably need a new data structure and ioctl command number to keep compatiblity with the old interface. Arnd
On Fri, Nov 30, 2018 at 10:14 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote: > > > > > - > > -#define MSM_INFO_FLAGS (MSM_INFO_IOVA) > > +/* Get or set GEM buffer info. The requested value can be passed > > + * directly in 'value', or for data larger than 64b 'value' is a > > + * pointer to userspace buffer, with 'len' specifying the number of > > + * bytes copied into that buffer. For info returned by pointer, > > + * calling the GEM_INFO ioctl with null 'value' will return the > > + * required buffer size in 'len' > > + */ > > +#define MSM_INFO_GET_OFFSET 0x00 /* get mmap() offset, returned by value */ > > +#define MSM_INFO_GET_IOVA 0x01 /* get iova, returned by value */ > > > > struct drm_msm_gem_info { > > __u32 handle; /* in */ > > - __u32 flags; /* in - combination of MSM_INFO_* flags */ > > - __u64 offset; /* out, mmap() offset or iova */ > > + __u32 info; /* in - one of MSM_INFO_* */ > > + __u64 value; /* in or out */ > > + __u32 len; /* in or out */ > > }; > > As structure with implicit padding has the problem of possibly leaking > kernel stack data. It's better to make the padding explicit here so you > can zero it from the kernel. Also, as I mentioned in the other patch, > you probably need a new data structure and ioctl command number > to keep compatiblity with the old interface. hmm, right, pad field is a good idea. As far as compat, drm_ioctl() handles zero-padding so adding new ioctl struct members at the end is safe (as long as a zero value somehow results in previous behavior) BR, -R > > Arnd
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 9f823bf8d312..913f5b3642b5 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -863,21 +863,28 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data, struct drm_gem_object *obj; int ret = 0; - if (args->flags & ~MSM_INFO_FLAGS) + switch (args->info) { + case MSM_INFO_GET_OFFSET: + case MSM_INFO_GET_IOVA: + /* value returned as immediate, not pointer, so len==0: */ + if (args->len) + return -EINVAL; + break; + default: return -EINVAL; + } obj = drm_gem_object_lookup(file, args->handle); if (!obj) return -ENOENT; - if (args->flags & MSM_INFO_IOVA) { - uint64_t iova; - - ret = msm_ioctl_gem_info_iova(dev, obj, &iova); - if (!ret) - args->offset = iova; - } else { - args->offset = msm_gem_mmap_offset(obj); + switch (args->info) { + case MSM_INFO_GET_OFFSET: + args->value = msm_gem_mmap_offset(obj); + break; + case MSM_INFO_GET_IOVA: + ret = msm_ioctl_gem_info_iova(dev, obj, &args->value); + break; } drm_gem_object_put_unlocked(obj); diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 3c3af92c4b3e..bc1757848c7c 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -105,14 +105,21 @@ struct drm_msm_gem_new { __u32 handle; /* out */ }; -#define MSM_INFO_IOVA 0x01 - -#define MSM_INFO_FLAGS (MSM_INFO_IOVA) +/* Get or set GEM buffer info. The requested value can be passed + * directly in 'value', or for data larger than 64b 'value' is a + * pointer to userspace buffer, with 'len' specifying the number of + * bytes copied into that buffer. For info returned by pointer, + * calling the GEM_INFO ioctl with null 'value' will return the + * required buffer size in 'len' + */ +#define MSM_INFO_GET_OFFSET 0x00 /* get mmap() offset, returned by value */ +#define MSM_INFO_GET_IOVA 0x01 /* get iova, returned by value */ struct drm_msm_gem_info { __u32 handle; /* in */ - __u32 flags; /* in - combination of MSM_INFO_* flags */ - __u64 offset; /* out, mmap() offset or iova */ + __u32 info; /* in - one of MSM_INFO_* */ + __u64 value; /* in or out */ + __u32 len; /* in or out */ }; #define MSM_PREP_READ 0x01
Prep work to add a way to get/set the GEM objects debug name. Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/msm/msm_drv.c | 25 ++++++++++++++++--------- include/uapi/drm/msm_drm.h | 17 ++++++++++++----- 2 files changed, 28 insertions(+), 14 deletions(-)