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

[Niranjana Vishwanathapura]

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

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

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
>