Re: [PATCH v2 6/6] vfio/type1: Use mapping page mask for pfnmaps

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 19 Feb 2025 14:32:35 -0600
Mitchell Augustin <mitchell.augustin@canonical.com> 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