Re: [PATCH v4 16/21] VT-d: free all-empty page tables

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

On Wed, May 18, 2022 at 12:26:03PM +0200, Jan Beulich wrote:
> On 10.05.2022 16:30, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:42:50AM +0200, Jan Beulich wrote:
> >> @@ -837,9 +843,31 @@ static int dma_pte_clear_one(struct doma
> >>  
> >>      old = *pte;
> >>      dma_clear_pte(*pte);
> >> +    iommu_sync_cache(pte, sizeof(*pte));
> >> +
> >> +    while ( pt_update_contig_markers(&page->val,
> >> +                                     address_level_offset(addr, level),
> >> +                                     level, PTE_kind_null) &&
> >> +            ++level < min_pt_levels )
> >> +    {
> >> +        struct page_info *pg = maddr_to_page(pg_maddr);
> >> +
> >> +        unmap_vtd_domain_page(page);
> >> +
> >> +        pg_maddr = addr_to_dma_page_maddr(domain, addr, level, 
> >> flush_flags,
> >> +                                          false);
> >> +        BUG_ON(pg_maddr < PAGE_SIZE);
> >> +
> >> +        page = map_vtd_domain_page(pg_maddr);
> >> +        pte = &page[address_level_offset(addr, level)];
> >> +        dma_clear_pte(*pte);
> >> +        iommu_sync_cache(pte, sizeof(*pte));
> >> +
> >> +        *flush_flags |= IOMMU_FLUSHF_all;
> >> +        iommu_queue_free_pgtable(hd, pg);
> >> +    }
> > 
> > I think I'm setting myself for trouble, but do we need to sync cache
> > the lower lever entries if higher level ones are to be changed.
> > 
> > IOW, would it be fine to just flush the highest level modified PTE?
> > As the lower lever ones won't be reachable anyway.
> 
> I definitely want to err on the safe side here. If later we can
> prove that some cache flush is unneeded, I'd be happy to see it
> go away.

Hm, so it's not only about adding more cache flushes, but moving them
inside of the locked region: previously the only cache flush was done
outside of the locked region.

I guess I can't convince myself why we would need to flush cache of
entries that are to be removed, and that also point to pages scheduled
to be freed.

> >> @@ -2182,8 +2210,21 @@ static int __must_check cf_check intel_i
> >>      }
> >>  
> >>      *pte = new;
> >> -
> >>      iommu_sync_cache(pte, sizeof(struct dma_pte));
> >> +
> >> +    /*
> >> +     * While the (ab)use of PTE_kind_table here allows to save some work 
> >> in
> >> +     * the function, the main motivation for it is that it avoids a so far
> >> +     * unexplained hang during boot (while preparing Dom0) on a Westmere
> >> +     * based laptop.
> >> +     */
> >> +    pt_update_contig_markers(&page->val,
> >> +                             address_level_offset(dfn_to_daddr(dfn), 
> >> level),
> >> +                             level,
> >> +                             (hd->platform_ops->page_sizes &
> >> +                              (1UL << level_to_offset_bits(level + 1))
> >> +                              ? PTE_kind_leaf : PTE_kind_table));
> > 
> > So this works because on what we believe to be affected models the
> > only supported page sizes are 4K?
> 
> Yes.
> 
> > Do we want to do the same with AMD if we don't allow 512G super pages?
> 
> Why? They don't have a similar flaw.

So the question was mostly whether we should also avoid the
pt_update_contig_markers for 1G entries, because we won't coalesce
them into a 512G anyway.  IOW avoid the overhead of updating the
contig markers if we know that the resulting super-page is not
supported by ->page_sizes.

Thanks, Roger.