On Mon, May 05, 2025 at 01:55:52PM -0300, Jason Gunthorpe wrote: > 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() >
This is what I was thinking as well.. why use a maple tree if we aren't allocating a range to manage pfns.. I was still thinking about this in v3 which made me hold back from acking this. I'm glad we clarified this! > > > 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 Thanks Praan
