| Message ID | 1501003615-15274-13-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, all. Any comments? On Tue, Jul 25, 2017 at 8:26 PM, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Reduce the scope of the TODO by squashing single-page stuff with > multi-page one. Next target is to use large pages whenever possible > in the case that hardware supports them. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Kevin Tian <kevin.tian@intel.com> > > --- > Changes in v1: > - > > Changes in v2: > - > --- > xen/drivers/passthrough/vtd/iommu.c | 138 +++++++++++++++++------------------- > 1 file changed, 67 insertions(+), 71 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c > index 45d1f36..d20b2f9 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1750,15 +1750,24 @@ static void iommu_domain_teardown(struct domain *d) > spin_unlock(&hd->arch.mapping_lock); > } > > -static int __must_check intel_iommu_map_page(struct domain *d, > - unsigned long gfn, > - unsigned long mfn, > - unsigned int flags) > +static int __must_check intel_iommu_unmap_pages(struct domain *d, > + unsigned long gfn, > + unsigned int order); > + > +/* > + * TODO: Optimize by using large pages whenever possible in the case > + * that hardware supports them. > + */ > +static int __must_check intel_iommu_map_pages(struct domain *d, > + unsigned long gfn, > + unsigned long mfn, > + unsigned int order, > + unsigned int flags) > { > struct domain_iommu *hd = dom_iommu(d); > - struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; > - u64 pg_maddr; > int rc = 0; > + unsigned long orig_gfn = gfn; > + unsigned long i; > > /* Do nothing if VT-d shares EPT page table */ > if ( iommu_use_hap_pt(d) ) > @@ -1768,78 +1777,60 @@ static int __must_check intel_iommu_map_page(struct domain *d, > if ( iommu_passthrough && is_hardware_domain(d) ) > return 0; > > - spin_lock(&hd->arch.mapping_lock); > - > - pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); > - if ( pg_maddr == 0 ) > + for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ ) > { > - spin_unlock(&hd->arch.mapping_lock); > - return -ENOMEM; > - } > - page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); > - pte = page + (gfn & LEVEL_MASK); > - old = *pte; > - dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); > - dma_set_pte_prot(new, > - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > + struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; > + u64 pg_maddr; > > - /* Set the SNP on leaf page table if Snoop Control available */ > - if ( iommu_snoop ) > - dma_set_pte_snp(new); > + spin_lock(&hd->arch.mapping_lock); > > - if ( old.val == new.val ) > - { > + pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); > + if ( pg_maddr == 0 ) > + { > + spin_unlock(&hd->arch.mapping_lock); > + rc = -ENOMEM; > + goto err; > + } > + page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); > + pte = page + (gfn & LEVEL_MASK); > + old = *pte; > + dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); > + dma_set_pte_prot(new, > + ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > + ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > + > + /* Set the SNP on leaf page table if Snoop Control available */ > + if ( iommu_snoop ) > + dma_set_pte_snp(new); > + > + if ( old.val == new.val ) > + { > + spin_unlock(&hd->arch.mapping_lock); > + unmap_vtd_domain_page(page); > + continue; > + } > + *pte = new; > + > + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > spin_unlock(&hd->arch.mapping_lock); > unmap_vtd_domain_page(page); > - return 0; > - } > - *pte = new; > - > - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > - spin_unlock(&hd->arch.mapping_lock); > - unmap_vtd_domain_page(page); > > - if ( !this_cpu(iommu_dont_flush_iotlb) ) > - rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); > - > - return rc; > -} > - > -static int __must_check intel_iommu_unmap_page(struct domain *d, > - unsigned long gfn) > -{ > - /* Do nothing if hardware domain and iommu supports pass thru. */ > - if ( iommu_passthrough && is_hardware_domain(d) ) > - return 0; > - > - return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); > -} > - > -/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ > -static int __must_check intel_iommu_map_pages(struct domain *d, > - unsigned long gfn, > - unsigned long mfn, > - unsigned int order, > - unsigned int flags) > -{ > - unsigned long i; > - int rc = 0; > - > - for ( i = 0; i < (1UL << order); i++ ) > - { > - rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); > - if ( unlikely(rc) ) > + if ( !this_cpu(iommu_dont_flush_iotlb) ) > { > - while ( i-- ) > - /* If statement to satisfy __must_check. */ > - if ( intel_iommu_unmap_page(d, gfn + i) ) > - continue; > - > - break; > + rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); > + if ( rc ) > + goto err; > } > } > > + return 0; > + > +err: > + while ( i-- ) > + /* If statement to satisfy __must_check. */ > + if ( intel_iommu_unmap_pages(d, orig_gfn + i, 0) ) > + continue; > + > return rc; > } > > @@ -1847,12 +1838,17 @@ static int __must_check intel_iommu_unmap_pages(struct domain *d, > unsigned long gfn, > unsigned int order) > { > - unsigned long i; > int rc = 0; > + unsigned long i; > + > + /* Do nothing if hardware domain and iommu supports pass thru. */ > + if ( iommu_passthrough && is_hardware_domain(d) ) > + return 0; > > - for ( i = 0; i < (1UL << order); i++ ) > + for ( i = 0; i < (1UL << order); i++, gfn++ ) > { > - int ret = intel_iommu_unmap_page(d, gfn + i); > + int ret = dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); > + > if ( !rc ) > rc = ret; > } > -- > 2.7.4 >
Hi. Gentle reminder. On Mon, Aug 21, 2017 at 7:44 PM, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote: > Hi, all. > > Any comments? > > On Tue, Jul 25, 2017 at 8:26 PM, Oleksandr Tyshchenko > <olekstysh@gmail.com> wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Reduce the scope of the TODO by squashing single-page stuff with >> multi-page one. Next target is to use large pages whenever possible >> in the case that hardware supports them. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Kevin Tian <kevin.tian@intel.com> >> >> --- >> Changes in v1: >> - >> >> Changes in v2: >> - >> --- >> xen/drivers/passthrough/vtd/iommu.c | 138 +++++++++++++++++------------------- >> 1 file changed, 67 insertions(+), 71 deletions(-) >> >> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c >> index 45d1f36..d20b2f9 100644 >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -1750,15 +1750,24 @@ static void iommu_domain_teardown(struct domain *d) >> spin_unlock(&hd->arch.mapping_lock); >> } >> >> -static int __must_check intel_iommu_map_page(struct domain *d, >> - unsigned long gfn, >> - unsigned long mfn, >> - unsigned int flags) >> +static int __must_check intel_iommu_unmap_pages(struct domain *d, >> + unsigned long gfn, >> + unsigned int order); >> + >> +/* >> + * TODO: Optimize by using large pages whenever possible in the case >> + * that hardware supports them. >> + */ >> +static int __must_check intel_iommu_map_pages(struct domain *d, >> + unsigned long gfn, >> + unsigned long mfn, >> + unsigned int order, >> + unsigned int flags) >> { >> struct domain_iommu *hd = dom_iommu(d); >> - struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; >> - u64 pg_maddr; >> int rc = 0; >> + unsigned long orig_gfn = gfn; >> + unsigned long i; >> >> /* Do nothing if VT-d shares EPT page table */ >> if ( iommu_use_hap_pt(d) ) >> @@ -1768,78 +1777,60 @@ static int __must_check intel_iommu_map_page(struct domain *d, >> if ( iommu_passthrough && is_hardware_domain(d) ) >> return 0; >> >> - spin_lock(&hd->arch.mapping_lock); >> - >> - pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); >> - if ( pg_maddr == 0 ) >> + for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ ) >> { >> - spin_unlock(&hd->arch.mapping_lock); >> - return -ENOMEM; >> - } >> - page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); >> - pte = page + (gfn & LEVEL_MASK); >> - old = *pte; >> - dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); >> - dma_set_pte_prot(new, >> - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | >> - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); >> + struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; >> + u64 pg_maddr; >> >> - /* Set the SNP on leaf page table if Snoop Control available */ >> - if ( iommu_snoop ) >> - dma_set_pte_snp(new); >> + spin_lock(&hd->arch.mapping_lock); >> >> - if ( old.val == new.val ) >> - { >> + pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); >> + if ( pg_maddr == 0 ) >> + { >> + spin_unlock(&hd->arch.mapping_lock); >> + rc = -ENOMEM; >> + goto err; >> + } >> + page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); >> + pte = page + (gfn & LEVEL_MASK); >> + old = *pte; >> + dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); >> + dma_set_pte_prot(new, >> + ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | >> + ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); >> + >> + /* Set the SNP on leaf page table if Snoop Control available */ >> + if ( iommu_snoop ) >> + dma_set_pte_snp(new); >> + >> + if ( old.val == new.val ) >> + { >> + spin_unlock(&hd->arch.mapping_lock); >> + unmap_vtd_domain_page(page); >> + continue; >> + } >> + *pte = new; >> + >> + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> spin_unlock(&hd->arch.mapping_lock); >> unmap_vtd_domain_page(page); >> - return 0; >> - } >> - *pte = new; >> - >> - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> - spin_unlock(&hd->arch.mapping_lock); >> - unmap_vtd_domain_page(page); >> >> - if ( !this_cpu(iommu_dont_flush_iotlb) ) >> - rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); >> - >> - return rc; >> -} >> - >> -static int __must_check intel_iommu_unmap_page(struct domain *d, >> - unsigned long gfn) >> -{ >> - /* Do nothing if hardware domain and iommu supports pass thru. */ >> - if ( iommu_passthrough && is_hardware_domain(d) ) >> - return 0; >> - >> - return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); >> -} >> - >> -/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ >> -static int __must_check intel_iommu_map_pages(struct domain *d, >> - unsigned long gfn, >> - unsigned long mfn, >> - unsigned int order, >> - unsigned int flags) >> -{ >> - unsigned long i; >> - int rc = 0; >> - >> - for ( i = 0; i < (1UL << order); i++ ) >> - { >> - rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); >> - if ( unlikely(rc) ) >> + if ( !this_cpu(iommu_dont_flush_iotlb) ) >> { >> - while ( i-- ) >> - /* If statement to satisfy __must_check. */ >> - if ( intel_iommu_unmap_page(d, gfn + i) ) >> - continue; >> - >> - break; >> + rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); >> + if ( rc ) >> + goto err; >> } >> } >> >> + return 0; >> + >> +err: >> + while ( i-- ) >> + /* If statement to satisfy __must_check. */ >> + if ( intel_iommu_unmap_pages(d, orig_gfn + i, 0) ) >> + continue; >> + >> return rc; >> } >> >> @@ -1847,12 +1838,17 @@ static int __must_check intel_iommu_unmap_pages(struct domain *d, >> unsigned long gfn, >> unsigned int order) >> { >> - unsigned long i; >> int rc = 0; >> + unsigned long i; >> + >> + /* Do nothing if hardware domain and iommu supports pass thru. */ >> + if ( iommu_passthrough && is_hardware_domain(d) ) >> + return 0; >> >> - for ( i = 0; i < (1UL << order); i++ ) >> + for ( i = 0; i < (1UL << order); i++, gfn++ ) >> { >> - int ret = intel_iommu_unmap_page(d, gfn + i); >> + int ret = dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); >> + >> if ( !rc ) >> rc = ret; >> } >> -- >> 2.7.4 >> > > > > -- > Regards, > > Oleksandr Tyshchenko
this patch alone looks OK to me. but I haven't found time to review the whole series to judge whether below change is necessary. > From: Oleksandr Tyshchenko [mailto:olekstysh@gmail.com] > Sent: Tuesday, September 12, 2017 10:44 PM > > Hi. > > Gentle reminder. > > On Mon, Aug 21, 2017 at 7:44 PM, Oleksandr Tyshchenko > <olekstysh@gmail.com> wrote: > > Hi, all. > > > > Any comments? > > > > On Tue, Jul 25, 2017 at 8:26 PM, Oleksandr Tyshchenko > > <olekstysh@gmail.com> wrote: > >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > >> > >> Reduce the scope of the TODO by squashing single-page stuff with > >> multi-page one. Next target is to use large pages whenever possible > >> in the case that hardware supports them. > >> > >> Signed-off-by: Oleksandr Tyshchenko > <oleksandr_tyshchenko@epam.com> > >> CC: Jan Beulich <jbeulich@suse.com> > >> CC: Kevin Tian <kevin.tian@intel.com> > >> > >> --- > >> Changes in v1: > >> - > >> > >> Changes in v2: > >> - > >> --- > >> xen/drivers/passthrough/vtd/iommu.c | 138 +++++++++++++++++-------- > ----------- > >> 1 file changed, 67 insertions(+), 71 deletions(-) > >> > >> diff --git a/xen/drivers/passthrough/vtd/iommu.c > b/xen/drivers/passthrough/vtd/iommu.c > >> index 45d1f36..d20b2f9 100644 > >> --- a/xen/drivers/passthrough/vtd/iommu.c > >> +++ b/xen/drivers/passthrough/vtd/iommu.c > >> @@ -1750,15 +1750,24 @@ static void > iommu_domain_teardown(struct domain *d) > >> spin_unlock(&hd->arch.mapping_lock); > >> } > >> > >> -static int __must_check intel_iommu_map_page(struct domain *d, > >> - unsigned long gfn, > >> - unsigned long mfn, > >> - unsigned int flags) > >> +static int __must_check intel_iommu_unmap_pages(struct domain *d, > >> + unsigned long gfn, > >> + unsigned int order); > >> + > >> +/* > >> + * TODO: Optimize by using large pages whenever possible in the case > >> + * that hardware supports them. > >> + */ > >> +static int __must_check intel_iommu_map_pages(struct domain *d, > >> + unsigned long gfn, > >> + unsigned long mfn, > >> + unsigned int order, > >> + unsigned int flags) > >> { > >> struct domain_iommu *hd = dom_iommu(d); > >> - struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; > >> - u64 pg_maddr; > >> int rc = 0; > >> + unsigned long orig_gfn = gfn; > >> + unsigned long i; > >> > >> /* Do nothing if VT-d shares EPT page table */ > >> if ( iommu_use_hap_pt(d) ) > >> @@ -1768,78 +1777,60 @@ static int __must_check > intel_iommu_map_page(struct domain *d, > >> if ( iommu_passthrough && is_hardware_domain(d) ) > >> return 0; > >> > >> - spin_lock(&hd->arch.mapping_lock); > >> - > >> - pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << > PAGE_SHIFT_4K, 1); > >> - if ( pg_maddr == 0 ) > >> + for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ ) > >> { > >> - spin_unlock(&hd->arch.mapping_lock); > >> - return -ENOMEM; > >> - } > >> - page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); > >> - pte = page + (gfn & LEVEL_MASK); > >> - old = *pte; > >> - dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); > >> - dma_set_pte_prot(new, > >> - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > >> - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > >> + struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; > >> + u64 pg_maddr; > >> > >> - /* Set the SNP on leaf page table if Snoop Control available */ > >> - if ( iommu_snoop ) > >> - dma_set_pte_snp(new); > >> + spin_lock(&hd->arch.mapping_lock); > >> > >> - if ( old.val == new.val ) > >> - { > >> + pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << > PAGE_SHIFT_4K, 1); > >> + if ( pg_maddr == 0 ) > >> + { > >> + spin_unlock(&hd->arch.mapping_lock); > >> + rc = -ENOMEM; > >> + goto err; > >> + } > >> + page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); > >> + pte = page + (gfn & LEVEL_MASK); > >> + old = *pte; > >> + dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); > >> + dma_set_pte_prot(new, > >> + ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > >> + ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > >> + > >> + /* Set the SNP on leaf page table if Snoop Control available */ > >> + if ( iommu_snoop ) > >> + dma_set_pte_snp(new); > >> + > >> + if ( old.val == new.val ) > >> + { > >> + spin_unlock(&hd->arch.mapping_lock); > >> + unmap_vtd_domain_page(page); > >> + continue; > >> + } > >> + *pte = new; > >> + > >> + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > >> spin_unlock(&hd->arch.mapping_lock); > >> unmap_vtd_domain_page(page); > >> - return 0; > >> - } > >> - *pte = new; > >> - > >> - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); > >> - spin_unlock(&hd->arch.mapping_lock); > >> - unmap_vtd_domain_page(page); > >> > >> - if ( !this_cpu(iommu_dont_flush_iotlb) ) > >> - rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); > >> - > >> - return rc; > >> -} > >> - > >> -static int __must_check intel_iommu_unmap_page(struct domain *d, > >> - unsigned long gfn) > >> -{ > >> - /* Do nothing if hardware domain and iommu supports pass thru. */ > >> - if ( iommu_passthrough && is_hardware_domain(d) ) > >> - return 0; > >> - > >> - return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); > >> -} > >> - > >> -/* TODO: Optimize by squashing map_pages/unmap_pages with > map_page/unmap_page */ > >> -static int __must_check intel_iommu_map_pages(struct domain *d, > >> - unsigned long gfn, > >> - unsigned long mfn, > >> - unsigned int order, > >> - unsigned int flags) > >> -{ > >> - unsigned long i; > >> - int rc = 0; > >> - > >> - for ( i = 0; i < (1UL << order); i++ ) > >> - { > >> - rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); > >> - if ( unlikely(rc) ) > >> + if ( !this_cpu(iommu_dont_flush_iotlb) ) > >> { > >> - while ( i-- ) > >> - /* If statement to satisfy __must_check. */ > >> - if ( intel_iommu_unmap_page(d, gfn + i) ) > >> - continue; > >> - > >> - break; > >> + rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); > >> + if ( rc ) > >> + goto err; > >> } > >> } > >> > >> + return 0; > >> + > >> +err: > >> + while ( i-- ) > >> + /* If statement to satisfy __must_check. */ > >> + if ( intel_iommu_unmap_pages(d, orig_gfn + i, 0) ) > >> + continue; > >> + > >> return rc; > >> } > >> > >> @@ -1847,12 +1838,17 @@ static int __must_check > intel_iommu_unmap_pages(struct domain *d, > >> unsigned long gfn, > >> unsigned int order) > >> { > >> - unsigned long i; > >> int rc = 0; > >> + unsigned long i; > >> + > >> + /* Do nothing if hardware domain and iommu supports pass thru. */ > >> + if ( iommu_passthrough && is_hardware_domain(d) ) > >> + return 0; > >> > >> - for ( i = 0; i < (1UL << order); i++ ) > >> + for ( i = 0; i < (1UL << order); i++, gfn++ ) > >> { > >> - int ret = intel_iommu_unmap_page(d, gfn + i); > >> + int ret = dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); > >> + > >> if ( !rc ) > >> rc = ret; > >> } > >> -- > >> 2.7.4 > >> > > > > > > > > -- > > Regards, > > > > Oleksandr Tyshchenko > > > > -- > Regards, > > Oleksandr Tyshchenko
Hi, Kevin On Wed, Sep 20, 2017 at 11:54 AM, Tian, Kevin <kevin.tian@intel.com> wrote: > this patch alone looks OK to me. but I haven't found time to review > the whole series to judge whether below change is necessary. Let me explain. Here [1] I touched common IOMMU code, and as the result I had to modify all existing IOMMU platform drivers and left TODO regarding future optimization. I did this patch as I was interested in adding IPMMU-VMSA IOMMU on ARM which supported super-pages. Current patch as well as the following [2] are attempts to *reduce* the scope of the TODO for x86-related IOMMUs. This is a maximum I could do without knowledge in subject. But the ideal patch would be to use large pages whenever possible in the case that hardware supports them. [1] [PATCH v2 02/13] iommu: Add extra order argument to the IOMMU APIs and platform callbacks https://www.mail-archive.com/xen-devel@lists.xen.org/msg115910.html [2] [PATCH v2 13/13] [RFC] iommu: AMD-Vi: Squash map_pages/unmap_pages with map_page/unmap_page https://patchwork.kernel.org/patch/9862571/ > >> From: Oleksandr Tyshchenko [mailto:olekstysh@gmail.com] >> Sent: Tuesday, September 12, 2017 10:44 PM >> >> Hi. >> >> Gentle reminder. >> >> On Mon, Aug 21, 2017 at 7:44 PM, Oleksandr Tyshchenko >> <olekstysh@gmail.com> wrote: >> > Hi, all. >> > >> > Any comments? >> > >> > On Tue, Jul 25, 2017 at 8:26 PM, Oleksandr Tyshchenko >> > <olekstysh@gmail.com> wrote: >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> >> >> Reduce the scope of the TODO by squashing single-page stuff with >> >> multi-page one. Next target is to use large pages whenever possible >> >> in the case that hardware supports them. >> >> >> >> Signed-off-by: Oleksandr Tyshchenko >> <oleksandr_tyshchenko@epam.com> >> >> CC: Jan Beulich <jbeulich@suse.com> >> >> CC: Kevin Tian <kevin.tian@intel.com> >> >> >> >> --- >> >> Changes in v1: >> >> - >> >> >> >> Changes in v2: >> >> - >> >> --- >> >> xen/drivers/passthrough/vtd/iommu.c | 138 +++++++++++++++++-------- >> ----------- >> >> 1 file changed, 67 insertions(+), 71 deletions(-) >> >> >> >> diff --git a/xen/drivers/passthrough/vtd/iommu.c >> b/xen/drivers/passthrough/vtd/iommu.c >> >> index 45d1f36..d20b2f9 100644 >> >> --- a/xen/drivers/passthrough/vtd/iommu.c >> >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> >> @@ -1750,15 +1750,24 @@ static void >> iommu_domain_teardown(struct domain *d) >> >> spin_unlock(&hd->arch.mapping_lock); >> >> } >> >> >> >> -static int __must_check intel_iommu_map_page(struct domain *d, >> >> - unsigned long gfn, >> >> - unsigned long mfn, >> >> - unsigned int flags) >> >> +static int __must_check intel_iommu_unmap_pages(struct domain *d, >> >> + unsigned long gfn, >> >> + unsigned int order); >> >> + >> >> +/* >> >> + * TODO: Optimize by using large pages whenever possible in the case >> >> + * that hardware supports them. >> >> + */ >> >> +static int __must_check intel_iommu_map_pages(struct domain *d, >> >> + unsigned long gfn, >> >> + unsigned long mfn, >> >> + unsigned int order, >> >> + unsigned int flags) >> >> { >> >> struct domain_iommu *hd = dom_iommu(d); >> >> - struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; >> >> - u64 pg_maddr; >> >> int rc = 0; >> >> + unsigned long orig_gfn = gfn; >> >> + unsigned long i; >> >> >> >> /* Do nothing if VT-d shares EPT page table */ >> >> if ( iommu_use_hap_pt(d) ) >> >> @@ -1768,78 +1777,60 @@ static int __must_check >> intel_iommu_map_page(struct domain *d, >> >> if ( iommu_passthrough && is_hardware_domain(d) ) >> >> return 0; >> >> >> >> - spin_lock(&hd->arch.mapping_lock); >> >> - >> >> - pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << >> PAGE_SHIFT_4K, 1); >> >> - if ( pg_maddr == 0 ) >> >> + for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ ) >> >> { >> >> - spin_unlock(&hd->arch.mapping_lock); >> >> - return -ENOMEM; >> >> - } >> >> - page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); >> >> - pte = page + (gfn & LEVEL_MASK); >> >> - old = *pte; >> >> - dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); >> >> - dma_set_pte_prot(new, >> >> - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | >> >> - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); >> >> + struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; >> >> + u64 pg_maddr; >> >> >> >> - /* Set the SNP on leaf page table if Snoop Control available */ >> >> - if ( iommu_snoop ) >> >> - dma_set_pte_snp(new); >> >> + spin_lock(&hd->arch.mapping_lock); >> >> >> >> - if ( old.val == new.val ) >> >> - { >> >> + pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << >> PAGE_SHIFT_4K, 1); >> >> + if ( pg_maddr == 0 ) >> >> + { >> >> + spin_unlock(&hd->arch.mapping_lock); >> >> + rc = -ENOMEM; >> >> + goto err; >> >> + } >> >> + page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); >> >> + pte = page + (gfn & LEVEL_MASK); >> >> + old = *pte; >> >> + dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); >> >> + dma_set_pte_prot(new, >> >> + ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | >> >> + ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); >> >> + >> >> + /* Set the SNP on leaf page table if Snoop Control available */ >> >> + if ( iommu_snoop ) >> >> + dma_set_pte_snp(new); >> >> + >> >> + if ( old.val == new.val ) >> >> + { >> >> + spin_unlock(&hd->arch.mapping_lock); >> >> + unmap_vtd_domain_page(page); >> >> + continue; >> >> + } >> >> + *pte = new; >> >> + >> >> + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> >> spin_unlock(&hd->arch.mapping_lock); >> >> unmap_vtd_domain_page(page); >> >> - return 0; >> >> - } >> >> - *pte = new; >> >> - >> >> - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); >> >> - spin_unlock(&hd->arch.mapping_lock); >> >> - unmap_vtd_domain_page(page); >> >> >> >> - if ( !this_cpu(iommu_dont_flush_iotlb) ) >> >> - rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); >> >> - >> >> - return rc; >> >> -} >> >> - >> >> -static int __must_check intel_iommu_unmap_page(struct domain *d, >> >> - unsigned long gfn) >> >> -{ >> >> - /* Do nothing if hardware domain and iommu supports pass thru. */ >> >> - if ( iommu_passthrough && is_hardware_domain(d) ) >> >> - return 0; >> >> - >> >> - return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); >> >> -} >> >> - >> >> -/* TODO: Optimize by squashing map_pages/unmap_pages with >> map_page/unmap_page */ >> >> -static int __must_check intel_iommu_map_pages(struct domain *d, >> >> - unsigned long gfn, >> >> - unsigned long mfn, >> >> - unsigned int order, >> >> - unsigned int flags) >> >> -{ >> >> - unsigned long i; >> >> - int rc = 0; >> >> - >> >> - for ( i = 0; i < (1UL << order); i++ ) >> >> - { >> >> - rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); >> >> - if ( unlikely(rc) ) >> >> + if ( !this_cpu(iommu_dont_flush_iotlb) ) >> >> { >> >> - while ( i-- ) >> >> - /* If statement to satisfy __must_check. */ >> >> - if ( intel_iommu_unmap_page(d, gfn + i) ) >> >> - continue; >> >> - >> >> - break; >> >> + rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); >> >> + if ( rc ) >> >> + goto err; >> >> } >> >> } >> >> >> >> + return 0; >> >> + >> >> +err: >> >> + while ( i-- ) >> >> + /* If statement to satisfy __must_check. */ >> >> + if ( intel_iommu_unmap_pages(d, orig_gfn + i, 0) ) >> >> + continue; >> >> + >> >> return rc; >> >> } >> >> >> >> @@ -1847,12 +1838,17 @@ static int __must_check >> intel_iommu_unmap_pages(struct domain *d, >> >> unsigned long gfn, >> >> unsigned int order) >> >> { >> >> - unsigned long i; >> >> int rc = 0; >> >> + unsigned long i; >> >> + >> >> + /* Do nothing if hardware domain and iommu supports pass thru. */ >> >> + if ( iommu_passthrough && is_hardware_domain(d) ) >> >> + return 0; >> >> >> >> - for ( i = 0; i < (1UL << order); i++ ) >> >> + for ( i = 0; i < (1UL << order); i++, gfn++ ) >> >> { >> >> - int ret = intel_iommu_unmap_page(d, gfn + i); >> >> + int ret = dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); >> >> + >> >> if ( !rc ) >> >> rc = ret; >> >> } >> >> -- >> >> 2.7.4 >> >> >> > >> > >> > >> > -- >> > Regards, >> > >> > Oleksandr Tyshchenko >> >> >> >> -- >> Regards, >> >> Oleksandr Tyshchenko
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 45d1f36..d20b2f9 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1750,15 +1750,24 @@ static void iommu_domain_teardown(struct domain *d) spin_unlock(&hd->arch.mapping_lock); } -static int __must_check intel_iommu_map_page(struct domain *d, - unsigned long gfn, - unsigned long mfn, - unsigned int flags) +static int __must_check intel_iommu_unmap_pages(struct domain *d, + unsigned long gfn, + unsigned int order); + +/* + * TODO: Optimize by using large pages whenever possible in the case + * that hardware supports them. + */ +static int __must_check intel_iommu_map_pages(struct domain *d, + unsigned long gfn, + unsigned long mfn, + unsigned int order, + unsigned int flags) { struct domain_iommu *hd = dom_iommu(d); - struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; - u64 pg_maddr; int rc = 0; + unsigned long orig_gfn = gfn; + unsigned long i; /* Do nothing if VT-d shares EPT page table */ if ( iommu_use_hap_pt(d) ) @@ -1768,78 +1777,60 @@ static int __must_check intel_iommu_map_page(struct domain *d, if ( iommu_passthrough && is_hardware_domain(d) ) return 0; - spin_lock(&hd->arch.mapping_lock); - - pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); - if ( pg_maddr == 0 ) + for ( i = 0; i < (1UL << order); i++, gfn++, mfn++ ) { - spin_unlock(&hd->arch.mapping_lock); - return -ENOMEM; - } - page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); - pte = page + (gfn & LEVEL_MASK); - old = *pte; - dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); - dma_set_pte_prot(new, - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); + struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; + u64 pg_maddr; - /* Set the SNP on leaf page table if Snoop Control available */ - if ( iommu_snoop ) - dma_set_pte_snp(new); + spin_lock(&hd->arch.mapping_lock); - if ( old.val == new.val ) - { + pg_maddr = addr_to_dma_page_maddr(d, (paddr_t)gfn << PAGE_SHIFT_4K, 1); + if ( pg_maddr == 0 ) + { + spin_unlock(&hd->arch.mapping_lock); + rc = -ENOMEM; + goto err; + } + page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); + pte = page + (gfn & LEVEL_MASK); + old = *pte; + dma_set_pte_addr(new, (paddr_t)mfn << PAGE_SHIFT_4K); + dma_set_pte_prot(new, + ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | + ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); + + /* Set the SNP on leaf page table if Snoop Control available */ + if ( iommu_snoop ) + dma_set_pte_snp(new); + + if ( old.val == new.val ) + { + spin_unlock(&hd->arch.mapping_lock); + unmap_vtd_domain_page(page); + continue; + } + *pte = new; + + iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); spin_unlock(&hd->arch.mapping_lock); unmap_vtd_domain_page(page); - return 0; - } - *pte = new; - - iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); - spin_unlock(&hd->arch.mapping_lock); - unmap_vtd_domain_page(page); - if ( !this_cpu(iommu_dont_flush_iotlb) ) - rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); - - return rc; -} - -static int __must_check intel_iommu_unmap_page(struct domain *d, - unsigned long gfn) -{ - /* Do nothing if hardware domain and iommu supports pass thru. */ - if ( iommu_passthrough && is_hardware_domain(d) ) - return 0; - - return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); -} - -/* TODO: Optimize by squashing map_pages/unmap_pages with map_page/unmap_page */ -static int __must_check intel_iommu_map_pages(struct domain *d, - unsigned long gfn, - unsigned long mfn, - unsigned int order, - unsigned int flags) -{ - unsigned long i; - int rc = 0; - - for ( i = 0; i < (1UL << order); i++ ) - { - rc = intel_iommu_map_page(d, gfn + i, mfn + i, flags); - if ( unlikely(rc) ) + if ( !this_cpu(iommu_dont_flush_iotlb) ) { - while ( i-- ) - /* If statement to satisfy __must_check. */ - if ( intel_iommu_unmap_page(d, gfn + i) ) - continue; - - break; + rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1); + if ( rc ) + goto err; } } + return 0; + +err: + while ( i-- ) + /* If statement to satisfy __must_check. */ + if ( intel_iommu_unmap_pages(d, orig_gfn + i, 0) ) + continue; + return rc; } @@ -1847,12 +1838,17 @@ static int __must_check intel_iommu_unmap_pages(struct domain *d, unsigned long gfn, unsigned int order) { - unsigned long i; int rc = 0; + unsigned long i; + + /* Do nothing if hardware domain and iommu supports pass thru. */ + if ( iommu_passthrough && is_hardware_domain(d) ) + return 0; - for ( i = 0; i < (1UL << order); i++ ) + for ( i = 0; i < (1UL << order); i++, gfn++ ) { - int ret = intel_iommu_unmap_page(d, gfn + i); + int ret = dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); + if ( !rc ) rc = ret; }