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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.11.2021 12:56, Roger Pau Monné wrote:
> On Mon, Sep 27, 2021 at 11:04:26AM +0200, 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:
>>>> -    parent = (struct dma_pte 
>>>> *)map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
>>>> -    while ( level > 1 )
>>>> +    pte_maddr = hd->arch.vtd.pgd_maddr;
>>>> +    parent = map_vtd_domain_page(pte_maddr);
>>>> +    while ( level > target )
>>>>      {
>>>>          offset = address_level_offset(addr, level);
>>>>          pte = &parent[offset];
>>>>  
>>>>          pte_maddr = dma_pte_addr(*pte);
>>>> -        if ( !pte_maddr )
>>>> +        if ( !dma_pte_present(*pte) || (level > 1 && 
>>>> dma_pte_superpage(*pte)) )
>>>>          {
>>>>              struct page_info *pg;
>>>> +            /*
>>>> +             * Higher level tables always set r/w, last level page table
>>>> +             * controls read/write.
>>>> +             */
>>>> +            struct dma_pte new_pte = { DMA_PTE_PROT };
>>>>  
>>>>              if ( !alloc )
>>>> -                break;
>>>> +            {
>>>> +                pte_maddr = 0;
>>>> +                if ( !dma_pte_present(*pte) )
>>>> +                    break;
>>>> +
>>>> +                /*
>>>> +                 * When the leaf entry was requested, pass back the full 
>>>> PTE,
>>>> +                 * with the address adjusted to account for the residual 
>>>> of
>>>> +                 * the walk.
>>>> +                 */
>>>> +                pte_maddr = pte->val +
>>>
>>> Wouldn't it be better to use dma_pte_addr(*pte) rather than accessing
>>> pte->val, and then you could drop the PAGE_MASK?
>>>
>>> Or is the addr parameter not guaranteed to be page aligned?
>>
>> addr is page aligned, but may not be superpage aligned. Yet that's not
>> the point here. As per the comment at the top of the function (and as
>> per the needs of intel_iommu_lookup_page()) we want to return a proper
>> (even if fake) PTE here, i.e. in particular including the access
>> control bits. Is "full" in the comment not sufficient to express this?
> 
> I see. I guess I got confused by the function name. It would be better
> called addr_to_dma_pte?

That wouldn't match its new purpose either. It can return an address
_or_ a full PTE, as per - as said - the comment being added at the
top of the function.

Jan