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

[Maarten Lankhorst]

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).

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
>>>