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

[Boris Brezillon]

On Tue, 7 Apr 2026 12:07:27 +0100
Liviu Dudau <liviu.dudau@xxxxxxx> wrote:

> 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@xxxxxxx> 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@xxxxxxxxxxxxx>
> > > > 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);

Well, you'd still need the vm->locked_region.size > 0 check for the
size update, because vm->locked_region.size > 0 and
vm->locked_region.start == 0 is allowed. In practice it won't
happen because we reserve the first 32M of the VA space in
mesa(panvk,gallium), but that's not enforced by the kernel, so I still
believe the check should be vm->locked_region.size > 0 rather than
vm->locked_region.start > 0.

> 
> >   
> > >   
> > > > +		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.

Fair enough. Let's add a WARN_ON_ONCE() and a comment explaining why the
overlap between old and new locked region is expected.