Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Thomas Hellstrom <thellstrom@vmware.com>]

On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently vmw_execbuf_ioctl() open-codes the permission checking,
> size
> extending and copying that is already done in core drm.
> 
> Kill all the duplication, adding a few comments for clarity.
> 
> Cc: "VMware Graphics" <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Tested using piglit quick using execbuf versions 1 and 2.

Tested-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>


> ---
> Thomas, VMware team,
> 
> Please give this some testing on your end. I've only tested it
> against
> mesa-master.
> 
> Thanks
> Emil
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 12 +-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++------------
> ----
>  3 files changed, 23 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index d3f108f7e52d..2cb6ae219e43 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] =
> {
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
>  		      DRM_AUTH | DRM_RENDER_ALLOW),
> -	VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> +	VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
>  		      DRM_RENDER_ALLOW),
>  	VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
>  		      DRM_RENDER_ALLOW),
> @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
>  			&vmw_ioctls[nr - DRM_COMMAND_BASE];
>  
>  		if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> -			ret = (long) drm_ioctl_permit(ioctl->flags,
> file_priv);
> -			if (unlikely(ret != 0))
> -				return ret;
> -
> -			if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> IOC_IN))
> -				goto out_io_encoding;
> -
> -			return (long) vmw_execbuf_ioctl(dev, arg,
> file_priv,
> -							_IOC_SIZE(cmd))
> ;
> +			return ioctl_func(filp, cmd, arg);
>  		} else if (nr == DRM_COMMAND_BASE +
> DRM_VMW_UPDATE_LAYOUT) {
>  			if (!drm_is_current_master(file_priv) &&
>  			    !capable(CAP_SYS_ADMIN))
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 9be2176cc260..f5bfac85f793 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct
> vmw_piter *viter)
>   * Command submission - vmwgfx_execbuf.c
>   */
>  
> -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> data,
> -			     struct drm_file *file_priv, size_t size);
> +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file_priv);
>  extern int vmw_execbuf_process(struct drm_file *file_priv,
>  			       struct vmw_private *dev_priv,
>  			       void __user *user_commands,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 2ff7ba04d8c8..767e2b99618d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> vmw_private *dev_priv)
>  	mutex_unlock(&dev_priv->cmdbuf_mutex);
>  }
>  
> -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
> -		      struct drm_file *file_priv, size_t size)
> +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> +		      struct drm_file *file_priv)
>  {
>  	struct vmw_private *dev_priv = vmw_priv(dev);
> -	struct drm_vmw_execbuf_arg arg;
> +	struct drm_vmw_execbuf_arg *arg = data;
>  	int ret;
> -	static const size_t copy_offset[] = {
> -		offsetof(struct drm_vmw_execbuf_arg, context_handle),
> -		sizeof(struct drm_vmw_execbuf_arg)};
>  	struct dma_fence *in_fence = NULL;
>  
> -	if (unlikely(size < copy_offset[0])) {
> -		VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> -			       DRM_VMW_EXECBUF);
> -		return -EINVAL;
> -	}
> -
> -	if (copy_from_user(&arg, (void __user *) data, copy_offset[0])
> != 0)
> -		return -EFAULT;
> -
>  	/*
>  	 * Extend the ioctl argument while maintaining backwards
> compatibility:
> -	 * We take different code paths depending on the value of
> arg.version.
> +	 * We take different code paths depending on the value of arg-
> >version.
> +	 *
> +	 * Note: The ioctl argument is extended and zeropadded by core
> DRM.
>  	 */
> -	if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> -		     arg.version == 0)) {
> +	if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> +		     arg->version == 0)) {
>  		VMW_DEBUG_USER("Incorrect execbuf version.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (arg.version > 1 &&
> -	    copy_from_user(&arg.context_handle,
> -			   (void __user *) (data + copy_offset[0]),
> -			   copy_offset[arg.version - 1] -
> copy_offset[0]) != 0)
> -		return -EFAULT;
> -
> -	switch (arg.version) {
> +	switch (arg->version) {
>  	case 1:
> -		arg.context_handle = (uint32_t) -1;
> +		/* For v1 core DRM have extended + zeropadded the data
> */
> +		arg->context_handle = (uint32_t) -1;
>  		break;
>  	case 2:
>  	default:
> +		/* For v2 and later core DRM would have correctly
> copied it */
>  		break;
>  	}
>  
>  	/* If imported a fence FD from elsewhere, then wait on it */
> -	if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> -		in_fence = sync_file_get_fence(arg.imported_fence_fd);
> +	if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> +		in_fence = sync_file_get_fence(arg->imported_fence_fd);
>  
>  		if (!in_fence) {
>  			VMW_DEBUG_USER("Cannot get imported fence\n");
> @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> unsigned long data,
>  		return ret;
>  
>  	ret = vmw_execbuf_process(file_priv, dev_priv,
> -				  (void __user *)(unsigned
> long)arg.commands,
> -				  NULL, arg.command_size,
> arg.throttle_us,
> -				  arg.context_handle,
> -				  (void __user *)(unsigned
> long)arg.fence_rep,
> -				  NULL, arg.flags);
> +				  (void __user *)(unsigned long)arg-
> >commands,
> +				  NULL, arg->command_size, arg-
> >throttle_us,
> +				  arg->context_handle,
> +				  (void __user *)(unsigned long)arg-
> >fence_rep,
> +				  NULL, arg->flags);
>  
>  	ttm_read_unlock(&dev_priv->reservation_sem);
>  	if (unlikely(ret != 0))
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel