From: "Tian, Kevin" <kevin.tian@intel.com>
To: Paul Durrant <paul@xen.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Paul Durrant <pdurrant@amazon.com>, Jan Beulich <jbeulich@suse.com>
Subject: RE: [PATCH v4 03/14] x86/iommu: convert VT-d code to use new page table allocator
Date: Fri, 14 Aug 2020 06:41:44 +0000 [thread overview]
Message-ID: <MWHPR11MB164570CF853F0E873BFDBB6C8C400@MWHPR11MB1645.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200804134209.8717-4-paul@xen.org>
> From: Paul Durrant <paul@xen.org>
> Sent: Tuesday, August 4, 2020 9:42 PM
>
> From: Paul Durrant <pdurrant@amazon.com>
>
> This patch converts the VT-d code to use the new IOMMU page table
> allocator
> function. This allows all the free-ing code to be removed (since it is now
> handled by the general x86 code) which reduces TLB and cache thrashing as
> well
> as shortening the code.
>
> The scope of the mapping_lock in intel_iommu_quarantine_init() has also
> been
> increased slightly; it should have always covered accesses to
> 'arch.vtd.pgd_maddr'.
>
> NOTE: The common IOMMU needs a slight modification to avoid scheduling
> the
> cleanup tasklet if the free_page_table() method is not present (since
> the tasklet will unconditionally call it).
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> Cc: Kevin Tian <kevin.tian@intel.com>
>
> v2:
> - New in v2 (split from "add common page-table allocator")
> ---
> xen/drivers/passthrough/iommu.c | 6 +-
> xen/drivers/passthrough/vtd/iommu.c | 101 ++++++++++------------------
> 2 files changed, 39 insertions(+), 68 deletions(-)
>
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 1d644844ab..2b1db8022c 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -225,8 +225,10 @@ static void iommu_teardown(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
>
> - hd->platform_ops->teardown(d);
> - tasklet_schedule(&iommu_pt_cleanup_tasklet);
> + iommu_vcall(hd->platform_ops, teardown, d);
> +
> + if ( hd->platform_ops->free_page_table )
> + tasklet_schedule(&iommu_pt_cleanup_tasklet);
> }
>
> void iommu_domain_destroy(struct domain *d)
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 94e0455a4d..607e8b5e65 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -265,10 +265,15 @@ static u64 addr_to_dma_page_maddr(struct
> domain *domain, u64 addr, int alloc)
>
> addr &= (((u64)1) << addr_width) - 1;
> ASSERT(spin_is_locked(&hd->arch.mapping_lock));
> - if ( !hd->arch.vtd.pgd_maddr &&
> - (!alloc ||
> - ((hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node)) ==
> 0)) )
> - goto out;
> + if ( !hd->arch.vtd.pgd_maddr )
> + {
> + struct page_info *pg;
> +
> + if ( !alloc || !(pg = iommu_alloc_pgtable(domain)) )
> + goto out;
> +
> + hd->arch.vtd.pgd_maddr = page_to_maddr(pg);
> + }
>
> parent = (struct dma_pte *)map_vtd_domain_page(hd-
> >arch.vtd.pgd_maddr);
> while ( level > 1 )
> @@ -279,13 +284,16 @@ static u64 addr_to_dma_page_maddr(struct
> domain *domain, u64 addr, int alloc)
> pte_maddr = dma_pte_addr(*pte);
> if ( !pte_maddr )
> {
> + struct page_info *pg;
> +
> if ( !alloc )
> break;
>
> - pte_maddr = alloc_pgtable_maddr(1, hd->node);
> - if ( !pte_maddr )
> + pg = iommu_alloc_pgtable(domain);
> + if ( !pg )
> break;
>
> + pte_maddr = page_to_maddr(pg);
> dma_set_pte_addr(*pte, pte_maddr);
>
> /*
> @@ -675,45 +683,6 @@ static void dma_pte_clear_one(struct domain
> *domain, uint64_t addr,
> unmap_vtd_domain_page(page);
> }
>
> -static void iommu_free_pagetable(u64 pt_maddr, int level)
> -{
> - struct page_info *pg = maddr_to_page(pt_maddr);
> -
> - if ( pt_maddr == 0 )
> - return;
> -
> - PFN_ORDER(pg) = level;
> - spin_lock(&iommu_pt_cleanup_lock);
> - page_list_add_tail(pg, &iommu_pt_cleanup_list);
> - spin_unlock(&iommu_pt_cleanup_lock);
> -}
> -
> -static void iommu_free_page_table(struct page_info *pg)
> -{
> - unsigned int i, next_level = PFN_ORDER(pg) - 1;
> - u64 pt_maddr = page_to_maddr(pg);
> - struct dma_pte *pt_vaddr, *pte;
> -
> - PFN_ORDER(pg) = 0;
> - pt_vaddr = (struct dma_pte *)map_vtd_domain_page(pt_maddr);
> -
> - for ( i = 0; i < PTE_NUM; i++ )
> - {
> - pte = &pt_vaddr[i];
> - if ( !dma_pte_present(*pte) )
> - continue;
> -
> - if ( next_level >= 1 )
> - iommu_free_pagetable(dma_pte_addr(*pte), next_level);
> -
> - dma_clear_pte(*pte);
> - iommu_sync_cache(pte, sizeof(struct dma_pte));
I didn't see sync_cache in the new iommu_free_pgtables. Is it intended
(i.e. original flush is meaningless) or overlooked?
Thanks
Kevin
> - }
> -
> - unmap_vtd_domain_page(pt_vaddr);
> - free_pgtable_maddr(pt_maddr);
> -}
> -
> static int iommu_set_root_entry(struct vtd_iommu *iommu)
> {
> u32 sts;
> @@ -1748,16 +1717,7 @@ static void iommu_domain_teardown(struct
> domain *d)
> xfree(mrmrr);
> }
>
> - ASSERT(is_iommu_enabled(d));
> -
> - if ( iommu_use_hap_pt(d) )
> - return;
> -
> - spin_lock(&hd->arch.mapping_lock);
> - iommu_free_pagetable(hd->arch.vtd.pgd_maddr,
> - agaw_to_level(hd->arch.vtd.agaw));
> hd->arch.vtd.pgd_maddr = 0;
> - spin_unlock(&hd->arch.mapping_lock);
> }
>
> static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn,
> @@ -2669,23 +2629,28 @@ static void vtd_dump_p2m_table(struct domain
> *d)
> static int __init intel_iommu_quarantine_init(struct domain *d)
> {
> struct domain_iommu *hd = dom_iommu(d);
> + struct page_info *pg;
> struct dma_pte *parent;
> unsigned int agaw =
> width_to_agaw(DEFAULT_DOMAIN_ADDRESS_WIDTH);
> unsigned int level = agaw_to_level(agaw);
> - int rc;
> + int rc = 0;
> +
> + spin_lock(&hd->arch.mapping_lock);
>
> if ( hd->arch.vtd.pgd_maddr )
> {
> ASSERT_UNREACHABLE();
> - return 0;
> + goto out;
> }
>
> - spin_lock(&hd->arch.mapping_lock);
> + pg = iommu_alloc_pgtable(d);
>
> - hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
> - if ( !hd->arch.vtd.pgd_maddr )
> + rc = -ENOMEM;
> + if ( !pg )
> goto out;
>
> + hd->arch.vtd.pgd_maddr = page_to_maddr(pg);
> +
> parent = map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
> while ( level )
> {
> @@ -2697,10 +2662,12 @@ static int __init
> intel_iommu_quarantine_init(struct domain *d)
> * page table pages, and the resulting allocations are always
> * zeroed.
> */
> - maddr = alloc_pgtable_maddr(1, hd->node);
> - if ( !maddr )
> - break;
> + pg = iommu_alloc_pgtable(d);
> +
> + if ( !pg )
> + goto out;
>
> + maddr = page_to_maddr(pg);
> for ( offset = 0; offset < PTE_NUM; offset++ )
> {
> struct dma_pte *pte = &parent[offset];
> @@ -2716,13 +2683,16 @@ static int __init
> intel_iommu_quarantine_init(struct domain *d)
> }
> unmap_vtd_domain_page(parent);
>
> + rc = 0;
> +
> out:
> spin_unlock(&hd->arch.mapping_lock);
>
> - rc = iommu_flush_iotlb_all(d);
> + if ( !rc )
> + rc = iommu_flush_iotlb_all(d);
>
> - /* Pages leaked in failure case */
> - return level ? -ENOMEM : rc;
> + /* Pages may be leaked in failure case */
> + return rc;
> }
>
> static struct iommu_ops __initdata vtd_ops = {
> @@ -2737,7 +2707,6 @@ static struct iommu_ops __initdata vtd_ops = {
> .map_page = intel_iommu_map_page,
> .unmap_page = intel_iommu_unmap_page,
> .lookup_page = intel_iommu_lookup_page,
> - .free_page_table = iommu_free_page_table,
> .reassign_device = reassign_device_ownership,
> .get_device_group_id = intel_iommu_group_id,
> .enable_x2apic = intel_iommu_enable_eim,
> --
> 2.20.1
next prev parent reply other threads:[~2020-08-14 6:42 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-04 13:41 [PATCH v4 00/14] IOMMU cleanup Paul Durrant
2020-08-04 13:41 ` [PATCH v4 01/14] x86/iommu: re-arrange arch_iommu to separate common fields Paul Durrant
2020-08-14 6:14 ` Tian, Kevin
2020-08-04 13:41 ` [PATCH v4 02/14] x86/iommu: add common page-table allocator Paul Durrant
2020-08-05 15:39 ` Jan Beulich
2020-08-04 13:41 ` [PATCH v4 03/14] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
2020-08-14 6:41 ` Tian, Kevin [this message]
2020-08-14 7:16 ` Durrant, Paul
2020-08-04 13:41 ` [PATCH v4 04/14] x86/iommu: convert AMD IOMMU " Paul Durrant
2020-08-04 13:42 ` [PATCH v4 05/14] iommu: remove unused iommu_ops method and tasklet Paul Durrant
2020-08-04 13:42 ` [PATCH v4 06/14] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
2020-08-05 16:06 ` Jan Beulich
2020-08-05 16:18 ` Paul Durrant
2020-08-06 11:41 ` Jan Beulich
2020-08-14 6:53 ` Tian, Kevin
2020-08-14 7:19 ` Durrant, Paul
2020-08-04 13:42 ` [PATCH v4 07/14] iommu: make map, unmap and flush all take both an order and a count Paul Durrant
2020-08-06 9:57 ` Jan Beulich
2020-08-11 11:00 ` Durrant, Paul
2020-08-14 6:57 ` Tian, Kevin
2020-08-04 13:42 ` [PATCH v4 08/14] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
2020-08-06 10:28 ` Jan Beulich
2020-08-12 9:36 ` [EXTERNAL] " Paul Durrant
2020-08-04 13:42 ` [PATCH v4 09/14] common/grant_table: batch flush I/O TLB Paul Durrant
2020-08-06 11:49 ` Jan Beulich
2020-08-04 13:42 ` [PATCH v4 10/14] iommu: remove the share_p2m operation Paul Durrant
2020-08-06 12:18 ` Jan Beulich
2020-08-14 7:04 ` Tian, Kevin
2020-08-04 13:42 ` [PATCH v4 11/14] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
2020-08-06 12:23 ` Jan Beulich
2020-08-14 7:12 ` Tian, Kevin
2020-08-04 13:42 ` [PATCH v4 12/14] vtd: use a bit field for root_entry Paul Durrant
2020-08-06 12:34 ` Jan Beulich
2020-08-12 13:13 ` Durrant, Paul
2020-08-18 8:27 ` Jan Beulich
2020-08-14 7:17 ` Tian, Kevin
2020-08-04 13:42 ` [PATCH v4 13/14] vtd: use a bit field for context_entry Paul Durrant
2020-08-06 12:46 ` Jan Beulich
2020-08-12 13:47 ` Paul Durrant
2020-08-14 7:19 ` Tian, Kevin
2020-08-04 13:42 ` [PATCH v4 14/14] vtd: use a bit field for dma_pte Paul Durrant
2020-08-06 12:53 ` Jan Beulich
2020-08-12 13:49 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR11MB164570CF853F0E873BFDBB6C8C400@MWHPR11MB1645.namprd11.prod.outlook.com \
--to=kevin.tian@intel.com \
--cc=jbeulich@suse.com \
--cc=paul@xen.org \
--cc=pdurrant@amazon.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).