Re: [PATCH] drm/panthor: Fix invalid handling of AS_LOCKADDR

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

On Tue, 7 Jan 2025 17:27:31 +0000
Florent Tomasin <florent.tomasin@arm.com> wrote:

> Arm Mali GPUs require AS_LOCKADDR region to be 32KB
> aligned, and does not support a size greater than
> the one specified by the HW property:
> `GPU_MMU_FEATURES_VA_BITS()`.
> 
> NOTES:
> - The size limitation is implementation defined.
> - Invalid alignment or size can result in an HW
>   undefined behaviour.
> 
> This patch modifies `lock_region()` to retrieve
> the maximum region size based on the HW property:
> `mmu_features`, and returns an error code if the
> requested size is not compliant with the HW
> limitation.
> 
> In addition, the function will guaranty the region
> is always 32KB aligned.
> 
> Signed-off-by: Florent Tomasin <florent.tomasin@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 37 ++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index c39e3eb1c15d..e834bc4d9a52 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -533,15 +533,20 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
>  	return status;
>  }
>  
> -static void lock_region(struct panthor_device *ptdev, u32 as_nr,
> -			u64 region_start, u64 size)
> +static int lock_region(struct panthor_device *ptdev, u32 as_nr,
> +		       u64 region_start, u64 size)
>  {
> +	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> +	u64 full_va_range = 1ull << va_bits;

Looks like we have a few places where we need the full_va_range, so I'd
be in favor of adding the following helper:

static u64 mmu_va_range(const struct panthor_device *ptdev)
{
   u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);

   return 1ull << va_bits;
}


>  	u8 region_width;
>  	u64 region;
>  	u64 region_end = region_start + size;
>  
>  	if (!size)
> -		return;
> +		return 0;
> +
> +	if (drm_WARN_ON(&ptdev->base, region_end > full_va_range))
> +		return -EFAULT;

How about we keep the function void and adjust the region_end to avoid
the undefined behavior?

>  
>  	/*
>  	 * The locked region is a naturally aligned power of 2 block encoded as
> @@ -552,7 +557,8 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>  	 * zeroed and ends with the bit (and subsequent bits) set to one.
>  	 */
>  	region_width = max(fls64(region_start ^ (region_end - 1)),
> -			   const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
> +			   const_ilog2(AS_LOCK_REGION_MIN_SIZE));
> +

I agree that the name doesn't really reflect its content, and doing the
minus(1) mod when preparing the region value is clearer, but it doesn't
seem like a bug to me, and is unrelated to the other changes in this
patch (actually, it's not even mentioned in the commit message). For
all these reasons, I'd put it in a separate patch.

>  
>  	/*
>  	 * Mask off the low bits of region_start (which would be ignored by
> @@ -560,21 +566,25 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>  	 */
>  	region_start &= GENMASK_ULL(63, region_width);
>  
> -	region = region_width | region_start;
> +	region = (region_width - 1) | region_start;
>  
>  	/* Lock the region that needs to be updated */
>  	gpu_write(ptdev, AS_LOCKADDR_LO(as_nr), lower_32_bits(region));
>  	gpu_write(ptdev, AS_LOCKADDR_HI(as_nr), upper_32_bits(region));
>  	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
> +
> +	return 0;
>  }
>  
>  static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>  				      u64 iova, u64 size, u32 op)
>  {
> +	int ret = 0;
> +
>  	lockdep_assert_held(&ptdev->mmu->as.slots_lock);
>  
>  	if (as_nr < 0)
> -		return 0;
> +		return ret;
>  
>  	/*
>  	 * If the AS number is greater than zero, then we can be sure
> @@ -583,7 +593,10 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
>  	 */
>  
>  	if (op != AS_COMMAND_UNLOCK)
> -		lock_region(ptdev, as_nr, iova, size);
> +		ret = lock_region(ptdev, as_nr, iova, size);
> +
> +	if (ret)
> +		return ret;
>  
>  	/* Run the MMU operation */
>  	write_cmd(ptdev, as_nr, op);
> @@ -608,9 +621,12 @@ static int mmu_hw_do_operation(struct panthor_vm *vm,
>  static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>  				 u64 transtab, u64 transcfg, u64 memattr)
>  {
> +	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> +	u64 full_va_range = 1ull << va_bits;
>  	int ret;
>  
> -	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> +	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0,
> +					 full_va_range, AS_COMMAND_FLUSH_MEM);
>  	if (ret)
>  		return ret;
>  
> @@ -628,9 +644,12 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>  
>  static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
>  {
> +	u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> +	u64 full_va_range = 1ull << va_bits;
>  	int ret;
>  
> -	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> +	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0,
> +					 full_va_range, AS_COMMAND_FLUSH_MEM);
>  	if (ret)
>  		return ret;
>