| Message ID | 20250107172732.87044-1-florent.tomasin@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panthor: Fix invalid handling of AS_LOCKADDR | expand |
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; >
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; 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; /* * 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)); + /* * 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;
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(-)