Re: [PATCH v6 07/10] drm/tyr: add VM-related ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/tyr: resource leak in Vm::map_bo_range() when VM is
  unusable
- [Medium] drm/tyr: partial 2 MiB block unmap in
  GpuVmData::sm_step_remap() fails
- [Low] drm/tyr: inline IdPool in VmPool::new() caps 32-bit VM IDs at 31
--

commit 30d6021aeaa9e7d9a97a8dfc27efff7e1fbee34d
Author: Alvin Sun <alvin.sun@linux.dev>

drm/tyr: add VM-related ioctls

This commit implements the PANTHOR_VM_CREATE, PANTHOR_VM_DESTROY,
PANTHOR_VM_BIND, and PANTHOR_VM_GET_STATE ioctls for the Tyr driver, wiring
them up to per-file VM pool management and synchronous VM bind operations.

> diff --git a/drivers/gpu/drm/tyr/file.rs b/drivers/gpu/drm/tyr/file.rs
> index 933a365cb016e..49b22e7ee4179 100644
> --- a/drivers/gpu/drm/tyr/file.rs
> +++ b/drivers/gpu/drm/tyr/file.rs

[ ... ]

> +fn vm_bind_exec_op(vm: &Vm<'_>, file: &TyrDrmFile, op: &VmBindOp) -> Result {

[ ... ]

> +    if (op.va | op.size | op.bo_offset) & (u64::SZ_4K - 1) != 0 {
> +        dev_dbg!(vm.dev(), "VM_BIND op not GPU-page-aligned\n");
> +        return Err(EINVAL);
> +    }
> +
> +    match VmBindOpType::try_from(op.flags) {
> +        Ok(VmBindOpType::Map) => {

[ ... ]

> +            vm.map_bo_range(&bo, op.bo_offset, op.size, op.va, map_flags)
> +        }
> +        Ok(VmBindOpType::Unmap) => {
> +            // Unmap must not carry map-specific flags or BO references.
> +            if op.flags & !VmBindOpType::MASK != 0 || op.bo_handle != 0 || op.bo_offset != 0 {
> +                dev_dbg!(
> +                    vm.dev(),
> +                    "VM_BIND UNMAP carries flags/BO refs: flags={:#x} bo_handle={} bo_offset={}\n",
> +                    op.flags,
> +                    op.bo_handle,
> +                    op.bo_offset
> +                );
> +                return Err(EINVAL);
> +            }
> +            vm.unmap_range(op.va, op.size)
> +        }

[Severity: Medium]
Can a 4 KiB-granularity partial unmap or overlapping remap here split a 2 MiB
block mapping and trigger WARN_ONCE in __arm_lpae_unmap()?

VmAsData::new() enables SZ_4K | SZ_2M in pgsize_bitmap, and pt_map() installs
2 MiB leaf block PTEs whenever both IOVA and physical address are 2 MiB-aligned
and at least 2 MiB remains.  When userspace issues a partial unmap or
overlapping map inside that 2 MiB block, GpuVmData::sm_step_remap() calls
pt_unmap() directly on the unaligned sub-range:

drivers/gpu/drm/tyr/vm.rs:GpuVmData::sm_step_remap() {
    ...
        if unmap_length > 0 {
            let region = unmap_start..(unmap_start + unmap_length);
            pt_unmap(context.dev, context.pt, region.clone()).inspect_err(|e| {
    ...
}

For a sub-block range, pt_unmap() calls IoPageTable::unmap_pages() with
pgsize = SZ_4K against the 2 MiB block PTE, which hits
WARN_ONCE(true, "Unmap of a partial large IOPTE is not allowed") in
__arm_lpae_unmap() and returns 0, failing VM_BIND with EINVAL (whereas C
panthor aligns the unmap range to 2 MiB boundaries and remaps the retained
4 KiB pages of op.prev and op.next).

> diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
> index 44f5046037895..11bc8e2c9d0be 100644
> --- a/drivers/gpu/drm/tyr/vm.rs
> +++ b/drivers/gpu/drm/tyr/vm.rs

[ ... ]

> @@ -664,6 +712,17 @@ pub(crate) fn map_bo_range(
>          };
>          let result = {
>              let mut gpuvm_unique = self.gpuvm_unique.lock();
> +            // Check under the GPUVM lock, which the kill path also takes to
> +            // unmap, so this serializes against a concurrent teardown.
> +            if self.is_unusable() {
> +                dev_err!(
> +                    self.dev,
> +                    "Failed to map VA {:#x}..{:#x}: VM is unusable\n",
> +                    req.region.start,
> +                    req.region.end
> +                );
> +                return Err(EINVAL);
> +            }
>              self.exec_op(gpuvm_unique.as_mut().get_mut(), req, &mut resources)
>          };
>          // We flush the defer cleanup list now. Things will be different in

[Severity: Medium]
Does returning early here leak the GpuVmBo, the GEM BO, and the GpuVm when a
concurrent PANTHOR_VM_DESTROY kills the VM between self.gpuvm.obtain() and
self.gpuvm_unique.lock()?

Earlier in Vm::map_bo_range(), req.op_type holds the ARef<GpuVmBo> returned by
self.gpuvm.obtain(bo, ()), which takes references on both the GpuVm and the
BO:

drivers/gpu/drm/tyr/vm.rs:Vm::map_bo_range() {
    ...
        let req = VmOpRequest {
            op_type: VmOpType::Map(VmMapArgs {
                vm_bo: self.gpuvm.obtain(bo, ())?,
                flags,
                bo_offset,
            }),
            region: va..va_end,
        };
    ...
}

When this early return unwinds req, dropping GpuVmBo calls
drm_gpuvm_bo_put_deferred(), which only queues the GpuVmBo onto gpuvm->bo_defer
while skipping self.gpuvm.deferred_cleanup() at the bottom of the function.
Because Vm::kill() has already finished its own unmap_range() before releasing
gpuvm_unique and the VM has been removed from VmPool, deferred_cleanup() will
never run again on this VM, permanently leaking the GpuVmBo, the BO's backing
pages, and the GpuVm.  Dropping req andgpuvm_unique before calling
self.gpuvm.deferred_cleanup() (or deferring self.gpuvm.obtain() until after
the check) would avoid this.

> @@ -1099,15 +1158,12 @@ pub(crate) struct VmPool<'drm> {
>  /// Error returned when a VM cannot be added to the pool. The caller
>  /// owns the teardown of the VM in [`AddError::vm`].
>  pub(crate) struct AddError<'drm> {
> -    #[expect(dead_code)]
>      pub(crate) error: Error,
> -    #[expect(dead_code)]
>      pub(crate) vm: Arc<Vm<'drm>>,
>  }
>  
>  impl<'drm> VmPool<'drm> {
>      /// Creates a new [`VmPool`].
> -    #[expect(dead_code)]
>      pub(crate) fn new(dev: &'drm Device<Bound>) -> impl PinInit<Self> {
>          let ids = IdPool::new();
>          pin_init!(Self {
> @@ -1123,7 +1179,6 @@ pub(crate) fn new(dev: &'drm Device<Bound>) -> impl PinInit<Self> {
>      /// returned in an [`AddError`] so the caller owns the teardown.
>      // TODO: allocate IDs with the XArray directly (once it grows range
>      // allocation, the equivalent of C's `XA_LIMIT`) and drop the IdPool.
> -    #[expect(dead_code)]
>      pub(crate) fn add(&self, vm: Arc<Vm<'drm>>) -> Result<u32, AddError<'drm>> {
>          let id = (|| {
>              let mut ids = self.ids.lock();

[Severity: Low]
On 32-bit architectures (ARM is enabled in Kconfig), does IdPool::new() limit
each file to 31 VMs instead of MAX_VMS_PER_FILE (32)?

IdPool::new() creates an inline bitmap of usize::BITS bits (indices 0..=31 on
32-bit), and VmPool::add() calls ids.find_unused_id(1) without growing the
pool:

drivers/gpu/drm/tyr/vm.rs:VmPool::add() {
    ...
            let mut ids = self.ids.lock();
            let unused = ids.find_unused_id(1).ok_or(ENOSPC)?;
            if unused.as_usize() > MAX_VMS_PER_FILE {
                return Err(ENOSPC);
            }
    ...
}

Because index 0 is skipped and index 32 is outside the 32-bit inline bitmap,
creating the 32nd VM on a file fails with ENOSPC.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-tyr-ioctls-v6-0-b2896bf2b3c9@kylinos.cn?part=7