Re: [PATCH 1/2] drm/panthor: Extend VM locked region for remap case to be a superset

[Liviu Dudau <liviu.dudau@arm.com>]

On Tue, Apr 07, 2026 at 12:43:53PM +0200, Boris Brezillon wrote:
> On Tue, 7 Apr 2026 11:24:52 +0100
> Liviu Dudau <liviu.dudau@arm.com> wrote:
> 
> > On Fri, Apr 03, 2026 at 06:21:11PM +0100, Adrián Larumbe wrote:
> > > In the event of an sm_step_remap() that leads to a partial unmap of a
> > > transparent huge page, the new locked region required by an extended unmap
> > > might not be a superset of the original one. Then, if it leaves a portion
> > > of the initially requested one out, the ensuing map will trigger a warning.
> > > 
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > Fixes: 8e7460eac786 ("drm/panthor: Support partial unmaps of huge pages")
> > > ---
> > >  drivers/gpu/drm/panthor/panthor_mmu.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > > index fa8b31df85c9..2b96359d3b94 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > > @@ -1709,6 +1709,19 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
> > >  	    start + size <= vm->locked_region.start + vm->locked_region.size)
> > >  		return 0;
> > >  
> > > +	/* sm_step_remap() may need a locked region that isn't a strict superset
> > > +	 * of the original one because of having to extend unmap boundaries beyond
> > > +	 * it to deal with partial unmaps of transparent huge pages. What we want
> > > +	 * in those cases is to lock the union of both regions.
> > > +	 */
> > > +	if (vm->locked_region.size) {  
> > 
> > Why is this check needed? We're updating the vm->locked_region.size later anyway, and I think
> > we can cope with a locked region being of zero size when we are called, unless we consider that
> > to be a bug and we should check earlier for a zero value.
> 
> It's here to detect if this is the initial lock (==0), or the one
> that's done in sm_step_remap() (!=0). If we drop this conditional, the
> adjusted start will always be zero on the initial lock, because both
> vm->locked_region.start and vm->locked_region.size are zero in that
> case (see panthor_vm_unlock_region()).

It makes sense to test the vm->locked_region.start being zero, not the vm->locked_region.size.

In your suggested update of the math, I would go:

	if (vm->locked_region.start)
		start = min(start, vm->locked_region.start);

> 
> > 
> > > +		u64 end = start + size;  
> > 
> > Like Boris pointed out, the calculations can be optimized so that we don't need this line.
> > 
> > > +
> > > +		start = min(start, vm->locked_region.start);
> > > +		size = max(vm->locked_region.start +
> > > +			   vm->locked_region.size, end) - start;  
> > 
> > If we have something like:
> > 
> >  ..... [start .. start+size] ...... [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size] ....
> 
> First off, that's not supposed to happen.

Yeah, I was thinking from a defensive coding perspective where this function gets attacked.

>  The 3 cases that exist now are:
> 
> [start .. start+size]
> 		[vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]
> 
> or
> 
> 									[start .. start+size]
> [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]
> 
> or
> 
> [start					..					 start+size]
> 	[vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]
> 
> 
> > 
> > we end up locking
> > 
> >  ..... [start ................................................. vm->locked_region.start + vm->locked_region.size] ....
> > 
> > is that intended?
> 
> We could add a WARN_ON() is there's no overlap between
> the previously locked region and the new one, but I'm
> not convinced this is something for panthor_vm_unlock_region() to
> enforce. Looks more like something the caller should check.

The only caller that might be exposed is panthor_vm_evict_bo_mappings_locked() and it doesn't look like
it could benefit from having the range check. I get it that it is not an expected scenario, just wanted
to double check.

Best regards,
Liviu

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯