Re: [PATCH v4 2/2] drm/imagination: Fix page count for page table for map() interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/imagination: under-allocation of page tables due to incorrect start_addr computation
--

commit 2406b8d6a0a1f7f5c8f27c8165d80a69f8753892
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>

drm/imagination: Fix page count for page table for map() interface

This patch updates pvr_mmu_op_context_create() to accept a device address
and includes it in the page table count calculations for the map()
interface. This aims to fix mapping failures that occurred when the virtual
start address was omitted.

> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c

[ ... ]

> @@ -2347,8 +2348,9 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
>   */
>  struct pvr_mmu_op_context *
>  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
> -			  u64 sgt_offset, u64 size)
> +			  u64 device_addr, u64 sgt_offset, u64 size)
>  {
> +	u64 start_addr = device_addr + sgt_offset;

[Severity: High]
Since device_addr is the virtual address where the mapping starts, and
sgt_offset is the physical offset within the backing buffer object, does
adding the physical offset to the virtual address incorrectly shift the
preallocation start address here?

>  	int err;
>  
>  	struct pvr_mmu_op_context *op_ctx = kzalloc_obj(*op_ctx);
> @@ -2364,16 +2366,16 @@ pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
>  	if (size) {
>  		/*
>  		 * The number of page table objects we need to prealloc is
> -		 * indicated by the mapping size, start offset and the sizes
> +		 * indicated by the mapping size, start address and the sizes
>  		 * of the areas mapped per PT or PD. The range calculation is
>  		 * identical to that for the index into a table for a device
>  		 * address, so we reuse those functions here.
>  		 */
> -		const u32 l1_start_idx = pvr_page_table_l2_idx(sgt_offset);
> -		const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size);
> +		const u32 l1_start_idx = pvr_page_table_l2_idx(start_addr);
> +		const u32 l1_end_idx = pvr_page_table_l2_idx(start_addr + size);
>  		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> -		const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset);
> -		const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size);
> +		const u32 l0_start_idx = pvr_page_table_l1_idx(start_addr);
> +		const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size);

[Severity: High]
By using this shifted start_addr, could we under-allocate page tables if the
actual virtual mapping from device_addr to device_addr + size crosses a page
table boundary (like a 2MB L0 block), but the incorrectly shifted start_addr
range does not?

If we preallocate fewer tables than needed, pvr_page_table_l0_get_or_insert()
might fail to pop from the empty preallocation list during pvr_mmu_map() and
return -ENOMEM, causing valid user mapping requests to unpredictably fail.

>  		const u32 l0_count = l0_end_idx - l0_start_idx + 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-mmu_fix-v4-0-12f1a871456a@imgtec.com?part=2