Hi Gyeyoung,
On Thu, 2026-10-01 at 22:39 +0900, Gyeyoung Baek wrote:
> Commit 0a8224058a58 ("drm/imagination: Fix page count for page table for
> map() interface") passed the device address to
> pvr_mmu_op_context_create(), but the preallocation is still sized from
> device_addr + sgt_offset with an exclusive end. The offset into the
> object's pages has no place in a device-virtual range, and the exclusive
> end allocates one table too many when the range ends on a table
> boundary.
>
> Count the tables from device_addr and the inclusive end of the range.
Somewhere in the description, could you also point out that the previous MMU
page count calculation could underflow when a mapping crossed a page table
boundary, that would lead to the driver preallocating a huge number of MMU
pages, exhausting system memory (so more or less what Sashiko pointed out in v1
of your VM_BIND series and in one of Brajesh's recent patches), and that the new
calculation also fixes that? I think it's ok to fix these together, or anyway
I'm fine with it in this case, but up to you if you'd rather split them.
Due to all of these and the fact that these errors are easy to trigger from
userspace, I think this patch needs Cc: [email protected] below.
>
> Fixes: 0a8224058a58 ("drm/imagination: Fix page count for page table for
> map() interface")
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Gyeyoung Baek <[email protected]>
> ---
> drivers/gpu/drm/imagination/pvr_mmu.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c
> b/drivers/gpu/drm/imagination/pvr_mmu.c
> index 62eae7fcd5a..67e73d7d5d0 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -2337,7 +2337,7 @@ void pvr_mmu_op_context_destroy(struct
> pvr_mmu_op_context *op_ctx)
> * @ctx: MMU context associated with owning VM context.
> * @sgt: Scatter gather table containing pages pinned for use by this
> context.
> * @device_addr: Virtual device address at the start of the requested
> mapping.
> - * @sgt_offset: Start offset of the requested device-virtual memory mapping.
> + * @sgt_offset: Offset into @sgt of the start of the requested mapping.
nit: could you also update the other similar description inside struct
pvr_mmu_op_context?
With these updated, you can also add my
Reviewed-by: Alessio Belle <[email protected]>
Thanks,
Alessio
> * @size: Size in bytes of the requested device-virtual memory mapping. For
> an
> * unmapping, this should be zero so that no page tables are allocated.
> *
> @@ -2350,7 +2350,6 @@ struct pvr_mmu_op_context *
> pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
> u64 device_addr, u64 sgt_offset, u64 size)
> {
> - u64 start_addr = device_addr + sgt_offset;
> int err;
>
> struct pvr_mmu_op_context *op_ctx = kzalloc_obj(*op_ctx);
> @@ -2365,18 +2364,18 @@ 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 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.
> + * The page tables needed are set by the device-virtual range
> + * being mapped: one level 1 table per 1GiB region and one
> + * level 0 table per 2MiB region the range touches. Tables that
> + * already exist are not consumed, so this is an upper bound.
> */
> - 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(start_addr);
> - const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size);
> - const u32 l0_count = l0_end_idx - l0_start_idx + 1;
> + const u64 last_addr = device_addr + size - 1;
> + const u64 l1_count =
> + (last_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) -
> + (device_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) + 1;
> + const u64 l0_count =
> + (last_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) -
> + (device_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) + 1;
>
> /*
> * Alloc and push page table entries until we have enough of
>
