Thanks Alex, that's super helpful and makes sense to me now! -Mitchell On Wed, Feb 26, 2025 at 11:55 AM Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > > On Wed, 19 Feb 2025 14:32:35 -0600 > Mitchell Augustin <mitchell.augustin@xxxxxxxxxxxxx> wrote: > > > > Thanks for the review and testing! > > > > Sure thing, thanks for the patch set! > > > > If you happen to have a few minutes, I'm struggling to understand the > > epfn computation and would appreciate some insight. > > Sorry, this slipped off my todo list for a few days. > > > My current understanding (very possibly incorrect): > > - epfn is intended to be the last page frame number that can be > > represented at the mapping level corresponding to addr_mask. (so, if > > addr_mask == PUD_MASK, epfn would be the highest pfn still in PUD > > level). > > Actually epfn is the first pfn of the next addr_mask level page. The > value in the parens (*pfn | (~addr_mask >> PAGE_SHIFT)) is the last pfn > within the same level page. We could do it either way, it's just a > matter of where the +1 gets added. > > > - ret should be == npages if all pfns in the requested vma are within > > the memory hierarchy level denoted by addr_mask. If npages is more > > than can be represented at that level, ret == the max number of page > > frames representable at addr_mask level. > > Yes. > > > - - (if the second case is true, that means we were not able to obtain > > all requested pages due to running out of PFNs at the current mapping > > level) > > vaddr_get_pfns() is called again if we haven't reached npage. > Specifically, from vfio_pin_pages_remote() we hit the added continue > under the !batch->size branch. If the pfnmaps are fully PUD aligned, > we'll call vaddr_get_pfns() once per PUD_SIZE, vfio_pin_pages_remote() > will only return with the full requested npage value, and we'll only > call vfio_iommu_map() once. The latter has always been true, the > difference is the number of times we iterate calling vaddr_get_pfns(). > > > If the above is all correct, what is confusing me is where the "(*pfn) > > | " comes into this equation. If epfn is meant to be the last pfn > > representable at addr_mask level of the hierarchy, wouldn't that be > > represented by (~pgmask >> PAGE_SHIFT) alone? > > (~addr_mask >> PAGE_SHIFT) gives us the last pfn relative to zero. We > want the last pfn relative to *pfn, therefore we OR in *pfn. The OR > handles any offset that *pfn might have within the addr_mask page, so > this operation always provides the last pfn of the addr_mask page > relative to *pfn. +1 because we want to calculate the number of pfns > until the next page. Thanks, > > Alex > -- Mitchell Augustin Software Engineer - Ubuntu Partner Engineering
