Re: [PATCH v3 1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 26, 2024 at 12:12:03PM +0100, Boris Brezillon wrote:
> When mapping an IO region, the pseudo-file offset is dependent on the
> userspace architecture. panthor_device_mmio_offset() abstracts that
> away for us by turning a userspace MMIO offset into its kernel
> equivalent, but we were not updating vm_area_struct::vm_pgoff
> accordingly, leading us to attach the MMIO region to the wrong file
> offset.
> 
> This has implications when we start mixing 64 bit and 32 bit apps, but
> that's only really a problem when we start having more that 2^43 bytes of
> memory allocated, which is very unlikely to happen.
> 
> What's more problematic is the fact this turns our
> unmap_mapping_range(DRM_PANTHOR_USER_MMIO_OFFSET) calls, which are
> supposed to kill the MMIO mapping when entering suspend, into NOPs.
> Which means we either keep the dummy flush_id mapping active at all
> times, or we risk a BUS_FAULT if the MMIO region was mapped, and the
> GPU is suspended after that.
> 
> Solve that by patching vm_pgoff early in panthor_mmap(). With
> this in place, we no longer need the panthor_device_mmio_offset()
> helper.
> 
> v3:
> - No changes
> 
> v2:
> - Kill panthor_device_mmio_offset()
> 
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Reported-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> Reported-by: Lukas F. Hartmann <lukas@xxxxxxxxx>
> Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/10835
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Reviewed-by: Steven Price <steven.price@xxxxxxx>

Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  8 ++++----
>  drivers/gpu/drm/panthor/panthor_device.h | 24 ------------------------
>  drivers/gpu/drm/panthor/panthor_drv.c    | 17 ++++++++++++++++-
>  3 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index bfe8da4a6e4c..3ddc4ba0acbe 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -334,7 +334,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct panthor_device *ptdev = vma->vm_private_data;
> -	u64 id = (u64)vma->vm_pgoff << PAGE_SHIFT;
> +	u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
>  	unsigned long pfn;
>  	pgprot_t pgprot;
>  	vm_fault_t ret;
> @@ -347,7 +347,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
>  	mutex_lock(&ptdev->pm.mmio_lock);
>  	active = atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE;
>  
> -	switch (panthor_device_mmio_offset(id)) {
> +	switch (offset) {
>  	case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
>  		if (active)
>  			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> @@ -378,9 +378,9 @@ static const struct vm_operations_struct panthor_mmio_vm_ops = {
>  
>  int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *vma)
>  {
> -	u64 id = (u64)vma->vm_pgoff << PAGE_SHIFT;
> +	u64 offset = (u64)vma->vm_pgoff << PAGE_SHIFT;
>  
> -	switch (panthor_device_mmio_offset(id)) {
> +	switch (offset) {
>  	case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
>  		if (vma->vm_end - vma->vm_start != PAGE_SIZE ||
>  		    (vma->vm_flags & (VM_WRITE | VM_EXEC)))
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..7ee4987a3796 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -365,30 +365,6 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev,			\
>  					 pirq);							\
>  }
>  
> -/**
> - * panthor_device_mmio_offset() - Turn a user MMIO offset into a kernel one
> - * @offset: Offset to convert.
> - *
> - * With 32-bit systems being limited by the 32-bit representation of mmap2's
> - * pgoffset field, we need to make the MMIO offset arch specific. This function
> - * converts a user MMIO offset into something the kernel driver understands.
> - *
> - * If the kernel and userspace architecture match, the offset is unchanged. If
> - * the kernel is 64-bit and userspace is 32-bit, the offset is adjusted to match
> - * 64-bit offsets. 32-bit kernel with 64-bit userspace is impossible.
> - *
> - * Return: Adjusted offset.
> - */
> -static inline u64 panthor_device_mmio_offset(u64 offset)
> -{
> -#ifdef CONFIG_ARM64
> -	if (test_tsk_thread_flag(current, TIF_32BIT))
> -		offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT - DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
> -#endif
> -
> -	return offset;
> -}
> -
>  extern struct workqueue_struct *panthor_cleanup_wq;
>  
>  #endif
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index ff484506229f..0097a01d0fc7 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1327,7 +1327,22 @@ static int panthor_mmap(struct file *filp, struct vm_area_struct *vma)
>  	if (!drm_dev_enter(file->minor->dev, &cookie))
>  		return -ENODEV;
>  
> -	if (panthor_device_mmio_offset(offset) >= DRM_PANTHOR_USER_MMIO_OFFSET)
> +#ifdef CONFIG_ARM64
> +	/*
> +	 * With 32-bit systems being limited by the 32-bit representation of
> +	 * mmap2's pgoffset field, we need to make the MMIO offset arch
> +	 * specific. This converts a user MMIO offset into something the kernel
> +	 * driver understands.
> +	 */
> +	if (test_tsk_thread_flag(current, TIF_32BIT) &&
> +	    offset >= DRM_PANTHOR_USER_MMIO_OFFSET_32BIT) {
> +		offset += DRM_PANTHOR_USER_MMIO_OFFSET_64BIT -
> +			  DRM_PANTHOR_USER_MMIO_OFFSET_32BIT;
> +		vma->vm_pgoff = offset >> PAGE_SHIFT;
> +	}
> +#endif
> +
> +	if (offset >= DRM_PANTHOR_USER_MMIO_OFFSET)
>  		ret = panthor_device_mmap_io(ptdev, vma);
>  	else
>  		ret = drm_gem_mmap(filp, vma);
> -- 
> 2.44.0
> 

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux