Re: [PATCH v10 5/5] drm/xe/xe_vm: Implement xe_vm_get_property_ioctl

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Mar 20, 2025 at 03:26:15PM +0000, Jonathan Cavitt wrote:
> Add support for userspace to request a list of observed faults
> from a specified VM.

...

> +static int xe_vm_get_property_size(struct xe_vm *vm, u32 property)
> +{
> +	int size = -EINVAL;

Mixing size and error codes is usually received with mixed feelings.

> +
> +	switch (property) {
> +	case DRM_XE_VM_GET_PROPERTY_FAULTS:
> +		spin_lock(&vm->faults.lock);
> +		size = vm->faults.len * sizeof(struct xe_vm_fault);

size_mul() and,
[1] perhaps fill it up into the pointer passed by the caller here?

> +		spin_unlock(&vm->faults.lock);
> +		break;
> +	default:
> +		break;

Do we need the default case?

> +	}
> +	return size;
> +}
> +
> +static int xe_vm_get_property_verify_size(struct xe_vm *vm, u32 property,
> +					  int expected, int actual)
> +{
> +	switch (property) {
> +	case DRM_XE_VM_GET_PROPERTY_FAULTS:

Unless we're expecting more cases (that we confidently know of), there's
not much point of single case switch.

> +		/*
> +		 * Number of faults may increase between calls to
> +		 * xe_vm_get_property_ioctl, so just report the
> +		 * number of faults the user requests if it's less
> +		 * than or equal to the number of faults in the VM
> +		 * fault array.
> +		 */
> +		if (actual < expected)
> +			return -EINVAL;
> +		break;
> +	default:
> +		if (actual != expected)
> +			return -EINVAL;
> +		break;
> +	}
> +	return 0;
> +}

...

> +static int xe_vm_get_property_fill_data(struct xe_vm *vm,
> +					struct drm_xe_vm_get_property *args)
> +{
> +	switch (args->property) {
> +	case DRM_XE_VM_GET_PROPERTY_FAULTS:
> +		return fill_faults(vm, args);
> +	default:
> +		break;

Same as above.

> +	}
> +	return -EINVAL;
> +}
> +
> +int xe_vm_get_property_ioctl(struct drm_device *drm, void *data,
> +			     struct drm_file *file)
> +{
> +	struct xe_device *xe = to_xe_device(drm);
> +	struct xe_file *xef = to_xe_file(file);
> +	struct drm_xe_vm_get_property *args = data;
> +	struct xe_vm *vm;
> +	int size, ret = 0;
> +
> +	if (XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> +		return -EINVAL;
> +
> +	vm = xe_vm_lookup(xef, args->vm_id);
> +	if (XE_IOCTL_DBG(xe, !vm))
> +		return -ENOENT;
> +
> +	size = xe_vm_get_property_size(vm, args->property);
> +
> +	if (size < 0) {
> +		ret = size;
> +		goto put_vm;
> +	} else if (!args->size && size) {
> +		args->size = size;
> +		goto put_vm;
> +	}

With [1] in place, this gymnastics can be dropped

	ret = xe_vm_get_property_size(vm, args->property, &size);
	if (ret)
		goto put_vm;

> +
> +	ret = xe_vm_get_property_verify_size(vm, args->property,
> +					     args->size, size);
> +	if (XE_IOCTL_DBG(xe, ret))
> +		goto put_vm;
> +
> +	ret = xe_vm_get_property_fill_data(vm, args);
> +
> +put_vm:
> +	xe_vm_put(vm);
> +	return ret;
> +}

Raag