[Intel-xe] [PATCH 2/2] drm/xe: Move in fault mode / non-fault mode check to xe_vm_create

[Matthew Brost]

On Fri, Mar 24, 2023 at 05:25:42PM +0100, Maarten Lankhorst wrote:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> 
> On 2023-03-24 08:20, Matthew Brost wrote:
> > On Thu, Mar 23, 2023 at 11:10:17PM -0700, Niranjana Vishwanathapura wrote:
> > > On Tue, Mar 21, 2023 at 04:42:17PM -0700, Matthew Brost wrote:
> > > > The check for fault mode / non-fault mode was in the VM create IOCTL
> > > > before VM creation and not under a lock. The increment was after VM
> > > > creation under the lock. This is racey. Move both the check and
> > > > increment to xe_vm_create before actual creation and have the lock for
> > > > both of these steps.
> > > > 
> > > > Suggested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_vm.c | 45 ++++++++++++++++++++++++--------------
> > > > 1 file changed, 28 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > > index e7674612a57e..965cad81b02a 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > @@ -1060,9 +1060,27 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > > > 	struct xe_gt *gt;
> > > > 	u8 id;
> > > > 
> > > > +	err = mutex_lock_interruptible(&xe->usm.lock);
> > > > +	if (err)
> > > > +		return ERR_PTR(err);
> > > > +	if (XE_IOCTL_ERR(xe, flags & XE_VM_FLAG_FAULT_MODE &&
> > > > +			 xe_device_in_non_fault_mode(xe)) ||
> > > > +	    XE_IOCTL_ERR(xe, !(flags & XE_VM_FLAG_MIGRATION) &&
> > > > +			 xe_device_in_fault_mode(xe))) {
> > > NIT...is below simplification any better?
> > > 
> > > bool fault_mode = !!(flags & XE_VM_FLAG_FAULT_MODE);
> > > if (XE_IOCTL_ERR(xe, fault_mode != xe_device_in_fault_mode(xe))
> > > 
> > Sure.
> 
> This will not work correctly when fault_mode and non_fault_mode are both
> false (eg first context created).
> 

Yep, realized this last night too and just sent an email stating that.

Matt

> I'd jsut leave this patch as it is now.
> 
> ~Maarten
> 
> > > > +		mutex_unlock(&xe->usm.lock);
> > > > +		return ERR_PTR(-EINVAL);
> > > > +	}
> > > > +	if (flags & XE_VM_FLAG_FAULT_MODE)
> > > > +		xe->usm.num_vm_in_fault_mode++;
> > > > +	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > > > +		xe->usm.num_vm_in_non_fault_mode++;
> > > > +	mutex_unlock(&xe->usm.lock);
> > > > +
> > > > 	vm = kzalloc(sizeof(*vm), GFP_KERNEL);
> > > > -	if (!vm)
> > > > -		return ERR_PTR(-ENOMEM);
> > > > +	if (!vm) {
> > > > +		err = -ENOMEM;
> > > > +		goto err_usm;
> > > > +	}
> > > > 
> > > > 	vm->xe = xe;
> > > > 	kref_init(&vm->refcount);
> > > > @@ -1182,13 +1200,6 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > > > 	if (number_gts > 1)
> > > > 		vm->composite_fence_ctx = dma_fence_context_alloc(1);
> > > > 
> > > > -	mutex_lock(&xe->usm.lock);
> > > > -	if (flags & XE_VM_FLAG_FAULT_MODE)
> > > > -		xe->usm.num_vm_in_fault_mode++;
> > > > -	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > > > -		xe->usm.num_vm_in_non_fault_mode++;
> > > > -	mutex_unlock(&xe->usm.lock);
> > > > -
> > > > 	trace_xe_vm_create(vm);
> > > > 
> > > > 	return vm;
> > > > @@ -1220,6 +1231,14 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > > > 		xe_device_mem_access_put(xe);
> > > > 		xe_pm_runtime_put(xe);
> > > > 	}
> > > > +err_usm:
> > > > +	mutex_lock(&xe->usm.lock);
> > > > +	if (flags & XE_VM_FLAG_FAULT_MODE)
> > > > +		xe->usm.num_vm_in_fault_mode--;
> > > > +	else if (!(flags & XE_VM_FLAG_MIGRATION))
> > > > +		xe->usm.num_vm_in_non_fault_mode--;
> > > > +	mutex_unlock(&xe->usm.lock);
> > > > +
> > > Perhaps put these counts increment/decrement blocks into functions
> > > instead of duplicating them?
> > > 
> > Sure.
> > 
> > Matt
> > > Niranjana
> > > 
> > > > 	return ERR_PTR(err);
> > > > }
> > > > 
> > > > @@ -1843,14 +1862,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> > > > 			 args->flags & DRM_XE_VM_CREATE_FAULT_MODE))
> > > > 		return -EINVAL;
> > > > 
> > > > -	if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_FAULT_MODE &&
> > > > -			 xe_device_in_non_fault_mode(xe)))
> > > > -		return -EINVAL;
> > > > -
> > > > -	if (XE_IOCTL_ERR(xe, !(args->flags & DRM_XE_VM_CREATE_FAULT_MODE) &&
> > > > -			 xe_device_in_fault_mode(xe)))
> > > > -		return -EINVAL;
> > > > -
> > > > 	if (XE_IOCTL_ERR(xe, args->flags & DRM_XE_VM_CREATE_FAULT_MODE &&
> > > > 			 !xe->info.supports_usm))
> > > > 		return -EINVAL;
> > > > -- 
> > > > 2.34.1
> > > >