On Wed, Feb 26, 2025 at 10:55:56PM +0000, Jonathan Cavitt wrote:
> Add support for userspace to get various properties from a specified VM.
> The currently supported properties are:
>
> - The number of engine resets the VM has observed
> - The number of exec queue bans the VM has observed, up to the last 50
> relevant ones, and how many of those were caused by faults.
>
> The latter request also includes information on the exec queue bans,
> such as the ID of the banned exec queue, whether the ban was caused by a
> pagefault or not, and the address and address type of the associated
> fault (if one exists).
>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> Suggested-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> ---
> drivers/gpu/drm/xe/xe_device.c | 2 +
> drivers/gpu/drm/xe/xe_vm.c | 106 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_vm.h | 2 +
> include/uapi/drm/xe_drm.h | 73 +++++++++++++++++++++++
> 4 files changed, 183 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 9454b51f7ad8..3a509a69062c 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -193,6 +193,8 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
> DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(XE_OBSERVATION, xe_observation_ioctl, DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(XE_VM_GET_PROPERTY, xe_vm_get_property_ioctl,
> + DRM_RENDER_ALLOW),
> };
>
> static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 3e88652670e6..047908eb9ff7 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3258,6 +3258,112 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> return err;
> }
>
> +static u32 xe_vm_get_property_size(struct xe_vm *vm, u32 property)
> +{
> + u32 size = -EINVAL;
> +
> + switch (property) {
> + case DRM_XE_VM_GET_PROPERTY_FAULTS:
> + spin_lock(&vm->bans.lock);
> + size = vm->bans.len * sizeof(struct drm_xe_ban);
> + spin_unlock(&vm->bans.lock);
> + size += sizeof(struct drm_xe_faults);
> + break;
> + case DRM_XE_VM_GET_PROPERTY_NUM_RESETS:
> + size = sizeof(u64);
> + break;
> + default:
> + break;
> + }
> +
> + return size;
> +}
> +
> +static enum drm_xe_fault_address_type
> +xe_pagefault_access_type_to_address_type(struct xe_vm *vm, struct xe_pagefault *pf)
> +{
> + if (!pf)
> + return 0;
> +
> + vma = lookup_vma(vm, pf->page_addr);
The VMA state is not stable; for example, it can change between the time
of the fault and the time this IOCTL is called. Therefore, we need an
intermediate structure fully populated at fault time, which is then
converted to user output upon IOCTL.
> + if (!vma)
> + return DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT;
> + if (xe_vma_read_only(vma) && pf->access_type != XE_PAGEFAULT_ACCESS_TYPE_READ)
> + return DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT;
> + return 0;
> +}
> +
> +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;
> + u32 size;
> +
> + 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);
For both this size calculation and the population if statements below,
I'd lean toward using vfunc tables for the implementations based on the
property index. Maybe overkill, though—not a strongly held opinion.
> + if (size < 0) {
> + return size;
> + } else if (!args->size) {
> + args->size = size;
> + return 0;
> + } else if (args->size != size) {
> + return -EINVAL;
> + }
> +
> + if (args->property == DRM_XE_VM_GET_PROPERTY_FAULTS) {
> + struct drm_xe_faults __user *usr_ptr = u64_to_user_ptr(args->data);
> + struct drm_xe_faults fault_list;
> + struct drm_xe_ban *ban;
> + struct xe_exec_queue_ban_entry *entry;
> + int i = 0;
> +
> + if (copy_from_user(&fault_list, usr_ptr, size))
> + return -EFAULT;
> +
> + fault_list.num_faults = 0;
> +
> + spin_lock(&vm->bans.lock);
> + list_for_each_entry(entry, &vm->bans.list, list) {
> + struct xe_pagefault *pf = entry->pf;
> +
> + ban = &fault_list.list[i++];
> + ban->exec_queue_id = entry->exec_queue_id;
> + ban->faulted = !!pf ? 1 : 0;
> + ban->address = pf ? pf->page_addr : 0;
> + ban->address_type = xe_pagefault_access_type_to_address_type(vm, pf);
> + ban->address_type = pf ? pf->fault_type : 0;
> + fault_list.num_faults += ban->faulted;
> + }
> + spin_unlock(&vm->bans.lock);
> +
> + fault_list.num_bans = i;
> +
> + if (copy_to_user(usr_ptr, &fault_list, size))
> + return -EFAULT;
> +
> + } else if (args->property == DRM_XE_VM_GET_PROPERTY_NUM_RESETS) {
> + u64 __user *usr_ptr = u64_to_user_ptr(args->data);
> + u64 num_resets = atomic_read(&vm->reset_count);
> +
> + if (copy_to_user(usr_ptr, &num_resets, size))
> + return -EFAULT;
> +
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /**
> * xe_vm_bind_kernel_bo - bind a kernel BO to a VM
> * @vm: VM to bind the BO to
> diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h
> index 78dbc5d57cd3..84653539d8db 100644
> --- a/drivers/gpu/drm/xe/xe_vm.h
> +++ b/drivers/gpu/drm/xe/xe_vm.h
> @@ -184,6 +184,8 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> int xe_vm_bind_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> +int xe_vm_get_property_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
>
> void xe_vm_close_and_put(struct xe_vm *vm);
>
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 76a462fae05f..00328d8a15dd 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -81,6 +81,7 @@ extern "C" {
> * - &DRM_IOCTL_XE_EXEC
> * - &DRM_IOCTL_XE_WAIT_USER_FENCE
> * - &DRM_IOCTL_XE_OBSERVATION
> + * - &DRM_IOCTL_XE_VM_GET_BANS
s/DRM_IOCTL_XE_VM_GET_BANS/DRM_IOCTL_XE_VM_GET_PROPERTY
> */
>
> /*
> @@ -102,6 +103,7 @@ extern "C" {
> #define DRM_XE_EXEC 0x09
> #define DRM_XE_WAIT_USER_FENCE 0x0a
> #define DRM_XE_OBSERVATION 0x0b
> +#define DRM_XE_VM_GET_PROPERTY 0x0c
>
> /* Must be kept compact -- no holes */
>
> @@ -117,6 +119,7 @@ extern "C" {
> #define DRM_IOCTL_XE_EXEC DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> #define DRM_IOCTL_XE_WAIT_USER_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> #define DRM_IOCTL_XE_OBSERVATION DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION, struct drm_xe_observation_param)
> +#define DRM_IOCTL_XE_VM_GET_PROPERTY DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_GET_PROPERTY, struct drm_xe_vm_get_property)
s/DRM_IOW/DRM_IOWR as we write back the size.
For reference: DRM_IOW does not copy back the modified IOCTL structure
(e.g., struct drm_xe_vm_get_property), while DRM_IOWR does.
>
> /**
> * DOC: Xe IOCTL Extensions
> @@ -1166,6 +1169,76 @@ struct drm_xe_vm_bind {
> __u64 reserved[2];
> };
>
> +/** Types of fault address */
> +enum drm_xe_fault_address_type {
> + DRM_XE_FAULT_ADDRESS_TYPE_NONE_EXT,
> + DRM_XE_FAULT_ADDRESS_TYPE_READ_INVALID_EXT,
> + DRM_XE_FAULT_ADDRESS_TYPE_WRITE_INVALID_EXT,
> +};
No enum uAPI in header files—use defines instead. Enums can change at
the compiler's discretion, while defines cannot.
> +
> +struct drm_xe_ban {
> + /** @exec_queue_id: ID of banned exec queue */
> + __u32 exec_queue_id;
I don't think we can reliably associate a page fault with an
exec_queue_id at the moment, given my above statement about having to
capture all state at the time of the page fault. Maybe we could with
some tricks between the page fault and the IOMMU CAT error G2H?
Regardless, let's ask the UMD we are targeting [1] if this information
would be helpful. It would seemingly have to be vendor-specific
information, not part of the generic Vk information.
Additionally, it might be good to ask what other vendor-specific
information, if any, we'd need here based on what the current page fault
interface supports.
[1] https://registry.khronos.org/vulkan/specs/latest/man/html/VK_EXT_device_fault.html
> + /** @faulted: Whether or not the ban has an associated pagefault. 0 is no, 1 is yes */
> + __u32 faulted;
> + /** @address: Address of the fault, if relevant */
> + __u64 address;
> + /** @address_type: enum drm_xe_fault_address_type, if relevant */
> + __u32 address_type;
We likely need a fault_size field to support VkDeviceSize
addressPrecision; as defined here [2]. I believe we can extract this
information from pagefault.fault_level.
[2] https://registry.khronos.org/vulkan/specs/latest/man/html/VkDeviceFaultAddressInfoEXT.html
> + /** @pad: MBZ */
> + __u32 pad;
> + /** @reserved: MBZ */
> + __u64 reserved[3];
> +};
> +
> +struct drm_xe_faults {
> + /** @num_faults: Number of faults observed on the VM */
> + __u32 num_faults;
> + /** @num_bans: Number of bans observed on the VM */
> + __u32 num_bans;
I don't think num_bans and num_faults really provide any benefit for
supporting [1]. The requirement for [1] is device faults—nothing more.
With that in mind, I'd lean toward an array of a single structure
(returned in drm_xe_vm_get_property.data, number of faults can be
inferred from the returned size) reporting all faults, with each entry
containing all the fault information. If another use case arises for
reporting all banned queues, we can add a property for that.
> + /** @reserved: MBZ */
> + __u64 reserved[2];
> + /** @list: Dynamic sized array of drm_xe_ban bans */
> + struct drm_xe_ban list[];
list[0] would be the prefered way.
> +};
> +
> +/**
> + * struct drm_xe_vm_get_property - Input of &DRM_IOCTL_XE_VM_GET_PROPERTY
> + *
> + * The user provides a VM ID and a property to query to this ioctl,
> + * and the ioctl returns the size of the return value. Calling the
> + * ioctl again with memory reserved for the data will save the
> + * requested property data to the data pointer.
> + *
> + * The valid properties are:
> + * - %DRM_XE_VM_GET_PROPERTY_FAULTS : Property is a drm_xe_faults struct of dynamic size
> + * - %DRM_XE_VM_GET_PROPERTY_NUM_RESETS: Property is a scalar
We need to consider where the number of resets requirement is coming
from. As far as I know, the Vk extension [1] we are targeting does not
need this information. I'm unsure about the compute UMD requirements at
the moment, so let's focus on supporting the Vk extension first.
Any uAPI must also have a UMD component, so focusing on one issue at a
time makes sense.
> + */
> +struct drm_xe_vm_get_property {
> + /** @extensions: Pointer to the first extension struct, if any */
> + __u64 extensions;
> +
> + /** @vm_id: The ID of the VM to query the properties of */
> + __u32 vm_id;
> +
> +#define DRM_XE_VM_GET_PROPERTY_FAULTS 0
> +#define DRM_XE_VM_GET_PROPERTY_NUM_RESETS 1
> + /** @property: The property to get */
> + __u32 property;
> +
> + /** @size: Size of returned property @data */
> + __u32 size;
> +
> + /** @pad: MBZ */
> + __u32 pad;
> +
> + /** @reserved: MBZ */
> + __u64 reserved[2];
I'd put the reserved bits at the end.
> +
> + /** @data: Pointer storing return data */
> + __u64 data;
union {
__u64 data;
__u64 ptr;
};
We would simply return 'data' for properties that fit in a u64 and
perform the size=0, return size process for properties that require a
user allocation.
This may not be relevant at the moment if we drop
DRM_XE_VM_GET_PROPERTY_NUM_RESET.
Matt
> +};
> +
> /**
> * struct drm_xe_exec_queue_create - Input of &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
> *
> --
> 2.43.0
>
