Re: [PATCH v2 2/2] drm/panthor: Fix the user MMIO offset logic for emulators

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 17 Apr 2025 16:13:49 +0100
Steven Price <steven.price@arm.com> wrote:

> On 17/04/2025 15:49, Boris Brezillon wrote:
> > Currently, we pick the MMIO offset based on the size of the pgoff_t
> > type seen by the process that manipulates the FD, such that a 32-bit
> > process can always map the user MMIO ranges. But this approach doesn't
> > work well for emulators like FEX, where the emulator is a 64-bit binary
> > which might be executing 32-bit code. In that case, the kernel thinks
> > it's the 64-bit process and assumes DRM_PANTHOR_USER_MMIO_OFFSET_64BIT
> > is in use, but the UMD library expects DRM_PANTHOR_USER_MMIO_OFFSET_32BIT,
> > because it can't mmap() anything above the pgoff_t size.
> > 
> > In order to solve that, we need a way to explicitly set the user MMIO
> > offset from the UMD, such that the kernel doesn't have to guess it
> > from the TIF_32BIT flag set on user thread. We keep the old behavior
> > if DRM_PANTHOR_SET_USER_MMIO_OFFSET is never called.
> > 
> > Changes:
> > - Drop the lock/immutable fields and allow SET_USER_MMIO_OFFSET
> >   requests to race with mmap() requests
> > - Don't do the is_user_mmio_offset test twice in panthor_mmap()
> > - Improve the uAPI docs
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Much nicer, thanks!
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> One note for merging - both this and Adrián's series are introducing the
> new 1.4 version. So we either need to switch one to 1.5 or combine the
> series.

I'll let Adrian series go first. I want to leave some time for others
to chime in anyway.

Thanks for the reviews/suggestions.

Boris

> 
> Thanks,
> Steve
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.h | 18 ++++++++
> >  drivers/gpu/drm/panthor/panthor_drv.c    | 56 +++++++++++++++++-------
> >  include/uapi/drm/panthor_drm.h           | 38 ++++++++++++++++
> >  3 files changed, 96 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index 4c27b6d85f46..6d8c2d5042f2 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -219,6 +219,24 @@ struct panthor_file {
> >  	/** @ptdev: Device attached to this file. */
> >  	struct panthor_device *ptdev;
> >  
> > +	/** @user_mmio: User MMIO related fields. */
> > +	struct {
> > +		/**
> > +		 * @offset: Offset used for user MMIO mappings.
> > +		 *
> > +		 * This offset should not be used to check the type of mapping
> > +		 * except in panthor_mmap(). After that point, MMIO mapping
> > +		 * offsets have been adjusted to match
> > +		 * DRM_PANTHOR_USER_MMIO_OFFSET and this macro should be used
> > +		 * instead.
> > +		 * Make sure this rule is followed at all times, because
> > +		 * userspace is in control of the offset, and can change the
> > +		 * value behind out back, potentially leading to erronous

Oops, typo here                 ^ our

> > +		 * branching happening in kernel space.
> > +		 */
> > +		u64 offset;
> > +	} user_mmio;