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
