On Tue, Apr 29, 2025 at 02:46:25PM -0700, Nicolin Chen wrote:
> > + immap = kzalloc(sizeof(*immap), GFP_KERNEL); > > + if (!immap) > > + return -ENOMEM; > > + immap->pfn_start = base >> PAGE_SHIFT; > > + immap->pfn_end = immap->pfn_start + (size >> PAGE_SHIFT) - 1; > > + > > + rc = mtree_alloc_range(&ictx->mt_mmap, immap_id, immap, sizeof(immap), > > I believe this should be sizeof(*immap) ?
Ugh, Sorry, shouldn't this be size >> PAGE_SHIFT (num_indices to alloc) ?
mtree_load() returns a "struct iommufd_map *" pointer.
I'm not talking about mtree_load. I meant mtree_alloc_range takes in a "size" parameter, which is being passed as sizeof(imap) in this patch. IIUC, the mtree_alloc_range, via mas_empty_area, gets a range that is sufficient for the given "size".
Now in this case, "size" would be the no. of pfns which are mmap-able. By passing sizeof(immap), we're simply reserving sizeof(ptr) i.e. 8 pfns for a 64-bit machine. Whereas we really, just want to reserve a range for size >> PAGE_SHIFT pfns.
But we are not storing pfns but the immap pointer..
That doesn't seem right, the entire point of using a maple tree is to manage the pfn number space, ie the pgoff argument to mmap.
So when calling mtree_alloc_range:
int mtree_alloc_range(struct maple_tree *mt, unsigned long *startp, void *entry, unsigned long size, unsigned long min, unsigned long max, gfp_t gfp)
size should be the number of PFNs this mmap is going to use, which is not sizeof() anything
min should be 0 and max should be uh.. U32_MAX >> PAGE_SHIFT IIRC.. There is a different limit for pgof fon 32 bit mmap()
Ohh... so we are storing the raw pointer in the mtree.. I got confused with the `LONG_MAX >> PAGE_SHIFT`.. Sorry about the confusion!
Yes. We want the pointer at mtree_load(). The pfn range is for validation after mtree_load(). And we are likely to stuff more bits into the immap structure for other verifications.
Validation is fine, but you still have to reserve the whole pfn number space to get sensible non-overlapping pgoffs out of the allocator.
Jason