Re: [PATCH v6 04/10] drm/tyr: add per-file VM pool

[Deborah Brouwer <deborah.brouwer@collabora.com>]

On Mon, Nov 09, 2026 at 12:48:15AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] drm/tyr: off-by-one IdPool capacity in VmPool::new() on
>   32-bit builds
> --
> 
> commit 6b8d6be457daf3e5e905977c364059ce3d3249e4
> Author: Alvin Sun <alvin.sun@linux.dev>
> 
> drm/tyr: add per-file VM pool
> 
> This commit introduces VmPool to manage up to 32 GPU address spaces per open
> file using an IdPool and an XArray, tearing down remaining VMs when the pool
> is dropped.
> 
> > diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
> > index 1dd11b2b12532..6f5518652a03f 100644
> > --- a/drivers/gpu/drm/tyr/vm.rs
> > +++ b/drivers/gpu/drm/tyr/vm.rs
> 
> [ ... ]
> 
> > @@ -947,3 +953,135 @@ fn pt_unmap(dev: &Device, pt: &IoPageTable<'_, ARM64LPAES1>, range: Range<u64>)
> >  
> >      Ok(())
> >  }
> > +
> > +/// Maximum number of VMs a single file may hold, matching panthor's
> > +/// `PANTHOR_MAX_VMS_PER_FILE`.
> > +const MAX_VMS_PER_FILE: usize = 32;
> 
> [ ... ]
> 
> > +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 {
> > +            dev,
> > +            ids <- new_mutex!(ids),
> > +            vms <- XArray::new(AllocKind::Alloc),
> > +        })
> > +    }
> > +
> > +    /// Stores the VM and returns the allocated ID.
> > +    ///
> > +    /// On failure - ID space exhausted or a store failure - the VM is
> > +    /// 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();
> > +            let unused = ids.find_unused_id(1).ok_or(ENOSPC)?;
> > +            if unused.as_usize() > MAX_VMS_PER_FILE {
> > +                return Err(ENOSPC);
> > +            }
> > +            Ok(unused.acquire())
> > +        })();
> 
> [Severity: Medium]
> Can VmPool::add() fail with ENOSPC on the 32nd VM on 32-bit builds?
> 
> IdPool::new() creates an inline bitmap of length usize::BITS, which is 32 on
> 32-bit platforms (indices 0..=31).  Because find_unused_id(1) skips bit 0,
> allocating 32 VMs requires bit index 32, so only 31 VMs (IDs 1..=31) can be
> allocated before find_unused_id(1) returns None.  Searching from bit 0 and
> adding 1 to form the 1-based VM ID (subtracting 1 on release_id()) would fit
> all 32 VMs within indices 0..=31.

Yes, this makes sense, on 32-bit builds the bitmap will stop at bit 31
and since we won't use bit 0, this gives us only 31 possible vm ids.

Instead of adding an off-by-one offset which seems like it could add
more confusion, what do you think of using the IdPool::with_capacity()
function? Something like:
  let ids = IdPool::with_capacity(MAX_VMS_PER_FILE + 1, GFP_KERNEL)?;

We would need to adjust the error handlings since this
is fallible but it might be simpler overall.

Deborah

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