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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Thanks, Roger.