IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: Jerry Snitselaar <jsnitsel@redhat.com>
To: Joerg Roedel <joro@8bytes.org>
Cc: Filippo Sironi <sironi@amazon.de>,
	iommu@lists.linux-foundation.org, jroedel@suse.de
Subject: Re: [PATCH 1/6] iommu/amd: Remove domain->updated
Date: Wed, 25 Sep 2019 23:18:04 -0700
Message-ID: <20190926061804.jaletrg437j4j2yh@cantor> (raw)
In-Reply-To: <20190925132300.3038-2-joro@8bytes.org>

On Wed Sep 25 19, Joerg Roedel wrote:
>From: Joerg Roedel <jroedel@suse.de>
>
>This struct member was used to track whether a domain
>change requires updates to the device-table and IOMMU cache
>flushes. The problem is, that access to this field is racy
>since locking in the common mapping code-paths has been
>eliminated.
>
>Move the updated field to the stack to get rid of all
>potential races and remove the field from the struct.
>
>Fixes: 92d420ec028d ("iommu/amd: Relax locking in dma_ops path")
>Signed-off-by: Joerg Roedel <jroedel@suse.de>
>---
> drivers/iommu/amd_iommu.c       | 49 +++++++++++++++++----------------
> drivers/iommu/amd_iommu_types.h |  1 -
> 2 files changed, 25 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>index 7bdce3b10f3d..042854bbc5bc 100644
>--- a/drivers/iommu/amd_iommu.c
>+++ b/drivers/iommu/amd_iommu.c
>@@ -1458,10 +1458,11 @@ static void free_pagetable(struct protection_domain *domain)
>  * another level increases the size of the address space by 9 bits to a size up
>  * to 64 bits.
>  */
>-static void increase_address_space(struct protection_domain *domain,
>+static bool increase_address_space(struct protection_domain *domain,
> 				   gfp_t gfp)
> {
> 	unsigned long flags;
>+	bool ret = false;
> 	u64 *pte;
>
> 	spin_lock_irqsave(&domain->lock, flags);
>@@ -1478,19 +1479,21 @@ static void increase_address_space(struct protection_domain *domain,
> 					iommu_virt_to_phys(domain->pt_root));
> 	domain->pt_root  = pte;
> 	domain->mode    += 1;
>-	domain->updated  = true;
>+
>+	ret = true;
>
> out:
> 	spin_unlock_irqrestore(&domain->lock, flags);
>
>-	return;
>+	return ret;
> }
>
> static u64 *alloc_pte(struct protection_domain *domain,
> 		      unsigned long address,
> 		      unsigned long page_size,
> 		      u64 **pte_page,
>-		      gfp_t gfp)
>+		      gfp_t gfp,
>+		      bool *updated)
> {
> 	int level, end_lvl;
> 	u64 *pte, *page;
>@@ -1498,7 +1501,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 	BUG_ON(!is_power_of_2(page_size));
>
> 	while (address > PM_LEVEL_SIZE(domain->mode))
>-		increase_address_space(domain, gfp);
>+		*updated = increase_address_space(domain, gfp) || *updated;
>
> 	level   = domain->mode - 1;
> 	pte     = &domain->pt_root[PM_LEVEL_INDEX(level, address)];
>@@ -1530,7 +1533,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 			for (i = 0; i < count; ++i)
> 				cmpxchg64(&lpte[i], __pte, 0ULL);
>
>-			domain->updated = true;
>+			*updated = true;
> 			continue;
> 		}
>
>@@ -1547,7 +1550,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
> 			if (cmpxchg64(pte, __pte, __npte) != __pte)
> 				free_page((unsigned long)page);
> 			else if (IOMMU_PTE_PRESENT(__pte))
>-				domain->updated = true;
>+				*updated = true;
>
> 			continue;
> 		}
>@@ -1656,28 +1659,29 @@ static int iommu_map_page(struct protection_domain *dom,
> 			  gfp_t gfp)
> {
> 	struct page *freelist = NULL;
>+	bool updated = false;
> 	u64 __pte, *pte;
>-	int i, count;
>+	int ret, i, count;
>
> 	BUG_ON(!IS_ALIGNED(bus_addr, page_size));
> 	BUG_ON(!IS_ALIGNED(phys_addr, page_size));
>
>+	ret = -EINVAL;
> 	if (!(prot & IOMMU_PROT_MASK))
>-		return -EINVAL;
>+		goto out;
>
> 	count = PAGE_SIZE_PTE_COUNT(page_size);
>-	pte   = alloc_pte(dom, bus_addr, page_size, NULL, gfp);
>+	pte   = alloc_pte(dom, bus_addr, page_size, NULL, gfp, &updated);
>
>-	if (!pte) {
>-		update_domain(dom);
>-		return -ENOMEM;
>-	}
>+	ret = -ENOMEM;
>+	if (!pte)
>+		goto out;
>
> 	for (i = 0; i < count; ++i)
> 		freelist = free_clear_pte(&pte[i], pte[i], freelist);
>
> 	if (freelist != NULL)
>-		dom->updated = true;
>+		updated = true;
>
> 	if (count > 1) {
> 		__pte = PAGE_SIZE_PTE(__sme_set(phys_addr), page_size);
>@@ -1693,12 +1697,16 @@ static int iommu_map_page(struct protection_domain *dom,
> 	for (i = 0; i < count; ++i)
> 		pte[i] = __pte;
>
>-	update_domain(dom);
>+	ret = 0;
>+
>+out:
>+	if (updated)
>+		update_domain(dom);
>
> 	/* Everything flushed out, free pages now */
> 	free_page_list(freelist);
>
>-	return 0;
>+	return ret;
> }
>
> static unsigned long iommu_unmap_page(struct protection_domain *dom,
>@@ -2399,15 +2407,10 @@ static void update_device_table(struct protection_domain *domain)
>
> static void update_domain(struct protection_domain *domain)
> {
>-	if (!domain->updated)
>-		return;
>-
> 	update_device_table(domain);
>
> 	domain_flush_devices(domain);
> 	domain_flush_tlb_pde(domain);
>-
>-	domain->updated = false;
> }
>
> static int dir2prot(enum dma_data_direction direction)
>@@ -3333,7 +3336,6 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
>
> 	/* Update data structure */
> 	domain->mode    = PAGE_MODE_NONE;
>-	domain->updated = true;
>
> 	/* Make changes visible to IOMMUs */
> 	update_domain(domain);
>@@ -3379,7 +3381,6 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
>
> 	domain->glx      = levels;
> 	domain->flags   |= PD_IOMMUV2_MASK;
>-	domain->updated  = true;
>
> 	update_domain(domain);
>
>diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
>index 9ac229e92b07..0186501ab971 100644
>--- a/drivers/iommu/amd_iommu_types.h
>+++ b/drivers/iommu/amd_iommu_types.h
>@@ -475,7 +475,6 @@ struct protection_domain {
> 	int glx;		/* Number of levels for GCR3 table */
> 	u64 *gcr3_tbl;		/* Guest CR3 table */
> 	unsigned long flags;	/* flags to find out type of domain */
>-	bool updated;		/* complete domain flush required */
> 	unsigned dev_cnt;	/* devices assigned to this domain */
> 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
> };
>-- 
>2.17.1
>
>_______________________________________________
>iommu mailing list
>iommu@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  parent reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-25 13:22 [PATCH 0/6] iommu/amd: Locking Fixes Joerg Roedel
2019-09-25 13:22 ` [PATCH 1/6] iommu/amd: Remove domain->updated Joerg Roedel
2019-09-25 15:45   ` Sironi, Filippo via iommu
2019-09-26  6:18   ` Jerry Snitselaar [this message]
2019-09-25 13:22 ` [PATCH 2/6] iommu/amd: Remove amd_iommu_devtable_lock Joerg Roedel
2019-09-25 15:50   ` Sironi, Filippo via iommu
2019-09-25 15:52     ` Sironi, Filippo via iommu
2019-09-26  6:24   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 3/6] iommu/amd: Take domain->lock for complete attach/detach path Joerg Roedel
2019-09-25 15:53   ` Sironi, Filippo via iommu
2019-09-26  6:34   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 4/6] iommu/amd: Check for busy devices earlier in attach_device() Joerg Roedel
2019-09-25 15:55   ` Sironi, Filippo via iommu
2019-09-26  6:37   ` Jerry Snitselaar
2019-09-25 13:22 ` [PATCH 5/6] iommu/amd: Lock dev_data in attach/detach code paths Joerg Roedel
2019-09-25 15:56   ` Sironi, Filippo via iommu
2019-09-26  6:41   ` Jerry Snitselaar
2019-09-25 13:23 ` [PATCH 6/6] iommu/amd: Lock code paths traversing protection_domain->dev_list Joerg Roedel
2019-09-25 15:58   ` Sironi, Filippo via iommu
2019-09-26  6:48   ` Jerry Snitselaar
2019-09-26  0:25 ` [PATCH 0/6] iommu/amd: Locking Fixes Jerry Snitselaar
2019-09-26  5:46   ` Joerg Roedel

Reply instructions:

You may reply publically 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=20190926061804.jaletrg437j4j2yh@cantor \
    --to=jsnitsel@redhat.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=jroedel@suse.de \
    --cc=sironi@amazon.de \
    /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

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git