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.
