Re: [PATCH v2 02/18] VT-d: have callers specify the target level for page table walks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2021 11:04, Jan Beulich wrote:
> On 24.09.2021 16:45, Roger Pau Monné wrote:
>> On Fri, Sep 24, 2021 at 11:42:13AM +0200, Jan Beulich wrote:
>>> In order to be able to insert/remove super-pages we need to allow
>>> callers of the walking function to specify at which point to stop the
>>> walk.
>>>
>>> For intel_iommu_lookup_page() integrate the last level access into
>>> the main walking function.
>>>
>>> dma_pte_clear_one() gets only partly adjusted for now: Error handling
>>> and order parameter get put in place, but the order parameter remains
>>> ignored (just like intel_iommu_map_page()'s order part of the flags).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> I have to admit that I don't understand why domain_pgd_maddr() wants to
>>> populate all page table levels for DFN 0.
>>
>> I think it would be enough to create up to the level requested by the
>> caller?
>>
>> Seems like a lazy way to always assert that the level requested by the
>> caller would be present.
> 
> The caller doesn't request any level here. What the caller passes in
> is the number of levels the respective IOMMU can deal with (varying
> of which across all IOMMUs is somewhat funny anyway). Hence I _guess_
> that it would really be sufficient to install as many levels as are
> necessary for the loop at the end of the function to complete
> successfully. Full depth population then would have happened only
> because until here addr_to_dma_page_maddr() didn't have a way to
> limit the number of levels. But then the comment is misleading. As
> I'm merely guessing here, I'm still hoping for Kevin to have (and
> share) some insight.

I've extended this post-commit-message comment to:

I have to admit that I don't understand why domain_pgd_maddr() wants to
populate all page table levels for DFN 0. I _guess_ that despite the
comment there what is needed is really only population down to
nr_pt_levels (such that the loop at the end of the function would
succeed). Problem is that this gets done only upon first allocating the
root table, yet the loop may later get executed with a smaller
nr_pt_levels. IOW population would need to be done down to the smallest
of all possible iommu->nr_pt_levels. As per a comment in iommu_alloc()
this can be between 2 and 4, yet once again the code there isn't fully
in line with the comment, going all the way down to 0.

Jan