[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 16/21] VT-d: free all-empty page tables
On 10.05.2022 16:30, Roger Pau Monné wrote: > On Mon, Apr 25, 2022 at 10:42:50AM +0200, Jan Beulich wrote: >> When a page table ends up with no present entries left, it can be >> replaced by a non-present entry at the next higher level. The page table >> itself can then be scheduled for freeing. >> >> Note that while its output isn't used there yet, >> pt_update_contig_markers() right away needs to be called in all places >> where entries get updated, not just the one where entries get cleared. >> >> Note further that while pt_update_contig_markers() updates perhaps >> several PTEs within the table, since these are changes to "avail" bits >> only I do not think that cache flushing would be needed afterwards. Such >> cache flushing (of entire pages, unless adding yet more logic to me more >> selective) would be quite noticable performance-wise (very prominent >> during Dom0 boot). >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >> --- >> v4: Re-base over changes earlier in the series. >> v3: Properly bound loop. Re-base over changes earlier in the series. >> v2: New. >> --- >> The hang during boot on my Latitude E6410 (see the respective code >> comment) was pretty close after iommu_enable_translation(). No errors, >> no watchdog would kick in, just sometimes the first few pixel lines of >> the next log message's (XEN) prefix would have made it out to the screen >> (and there's no serial there). It's been a lot of experimenting until I >> figured the workaround (which I consider ugly, but halfway acceptable). >> I've been trying hard to make sure the workaround wouldn't be masking a >> real issue, yet I'm still wary of it possibly doing so ... My best guess >> at this point is that on these old IOMMUs the ignored bits 52...61 >> aren't really ignored for present entries, but also aren't "reserved" >> enough to trigger faults. This guess is from having tried to set other >> bits in this range (unconditionally, and with the workaround here in >> place), which yielded the same behavior. > > Should we take Kevin's Reviewed-by as a heads up that bits 52..61 on > some? IOMMUs are not usable? > > Would be good if we could get a more formal response I think. A more formal response would be nice, but given the age of the affected hardware I don't expect anything more will be done there by Intel. >> @@ -405,6 +408,9 @@ static uint64_t addr_to_dma_page_maddr(s >> >> write_atomic(&pte->val, new_pte.val); >> iommu_sync_cache(pte, sizeof(struct dma_pte)); >> + pt_update_contig_markers(&parent->val, >> + address_level_offset(addr, level), > > I think (unless previous patches in the series have changed this) > there already is an 'offset' local variable that you could use. The variable is clobbered by "IOMMU/x86: prefill newly allocate page tables". >> @@ -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. >> @@ -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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |