On 13.12.2021 12:54, Roger Pau Monné wrote: > On Fri, Sep 24, 2021 at 11:52:47AM +0200, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -743,18 +743,37 @@ static int __must_check iommu_flush_iotl >> return iommu_flush_iotlb(d, INVALID_DFN, 0, 0); >> } >> >> +static void queue_free_pt(struct domain *d, mfn_t mfn, unsigned int >> next_level) > > Same comment as the AMD side patch, about naming the parameter just > level.
Sure, will change. >> @@ -1901,13 +1926,15 @@ static int __must_check intel_iommu_map_ >> } >> >> page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); >> - pte = &page[dfn_x(dfn) & LEVEL_MASK]; >> + pte = &page[address_level_offset(dfn_to_daddr(dfn), level)]; >> old = *pte; >> >> dma_set_pte_addr(new, mfn_to_maddr(mfn)); >> dma_set_pte_prot(new, >> ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | >> ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); >> + if ( IOMMUF_order(flags) ) > > You seem to use level > 1 in other places to check for whether the > entry is intended to be a super-page. Is there any reason to use > IOMMUF_order here instead? "flags" is the original source of information here, so it seemed more natural to use it. The following hunk uses "level > 1" to better match the similar unmap logic as well as AMD code. Maybe I should change those to also use "flags" (or "order" in the unmap case), as that would allow re-using the local variable in the new patches in v3 doing the re-coalescing of present superpages (right now I'm using a second, not very nicely named variable there). I'll have to think about this some and check whether there are other issues if I made such a change. >> @@ -2328,6 +2361,11 @@ static int __init vtd_setup(void) >> cap_sps_2mb(iommu->cap) ? ", 2MB" : "", >> cap_sps_1gb(iommu->cap) ? ", 1GB" : ""); >> >> + if ( !cap_sps_2mb(iommu->cap) ) >> + large_sizes &= ~PAGE_SIZE_2M; >> + if ( !cap_sps_1gb(iommu->cap) ) >> + large_sizes &= ~PAGE_SIZE_1G; >> + >> #ifndef iommu_snoop >> if ( iommu_snoop && !ecap_snp_ctl(iommu->ecap) ) >> iommu_snoop = false; >> @@ -2399,6 +2437,9 @@ static int __init vtd_setup(void) >> if ( ret ) >> goto error; >> >> + ASSERT(iommu_ops.page_sizes & PAGE_SIZE_4K); > > Since you are adding the assert, it might be more complete to check no > other page sizes are set, iommu_ops.page_sizes == PAGE_SIZE_4K? Ah yes, would make sense. Let me change this. Jan
