iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root
@ 2020-06-26  8:05 Joerg Roedel
  2020-06-26  8:05 ` [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Joerg Roedel @ 2020-06-26  8:05 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, Joerg Roedel, linux-kernel

From: Joerg Roedel <jroedel@suse.de>

Hi,

a previous discussion pointed out that using atomic64_t for that
purpose is a bit of overkill. This patch-set replaces it with unsigned
long and introduces some helpers first to make the change more easy.

Qian, can you please test these patches in your environment? You can
trigger any race-condition there pretty reliably :)

Other than that, please review and test.

Regards,

	Joerg

Changed to v1:

	- Addressed review comments from Qian.

Joerg Roedel (2):
  iommu/amd: Add helper functions to update domain->pt_root
  iommu/amd: Use 'unsigned long' for domain->pt_root

 drivers/iommu/amd/amd_iommu_types.h |  2 +-
 drivers/iommu/amd/iommu.c           | 44 +++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 13 deletions(-)

-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root
  2020-06-26  8:05 [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel
@ 2020-06-26  8:05 ` Joerg Roedel
  2020-06-26  8:05 ` [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel
  2020-06-26 12:30 ` [PATCH v2 0/2] iommu/amd: Don't use atomic64_t " Qian Cai
  2 siblings, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2020-06-26  8:05 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, Joerg Roedel, linux-kernel

From: Joerg Roedel <jroedel@suse.de>

Do not call atomic64_set() directly to update the domain page-table
root and use two new helper functions.

This makes it easier to implement additional work necessary when
the page-table is updated.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/iommu.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 74cca1757172..5286ddcfc2f9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -162,7 +162,18 @@ static void amd_iommu_domain_get_pgtable(struct protection_domain *domain,
 	pgtable->mode = pt_root & 7; /* lowest 3 bits encode pgtable mode */
 }
 
-static u64 amd_iommu_domain_encode_pgtable(u64 *root, int mode)
+static void amd_iommu_domain_set_pt_root(struct protection_domain *domain, u64 root)
+{
+	atomic64_set(&domain->pt_root, root);
+}
+
+static void amd_iommu_domain_clr_pt_root(struct protection_domain *domain)
+{
+	amd_iommu_domain_set_pt_root(domain, 0);
+}
+
+static void amd_iommu_domain_set_pgtable(struct protection_domain *domain,
+					 u64 *root, int mode)
 {
 	u64 pt_root;
 
@@ -170,7 +181,7 @@ static u64 amd_iommu_domain_encode_pgtable(u64 *root, int mode)
 	pt_root = mode & 7;
 	pt_root |= (u64)root;
 
-	return pt_root;
+	amd_iommu_domain_set_pt_root(domain, pt_root);
 }
 
 static struct iommu_dev_data *alloc_dev_data(u16 devid)
@@ -1410,7 +1421,7 @@ static bool increase_address_space(struct protection_domain *domain,
 	struct domain_pgtable pgtable;
 	unsigned long flags;
 	bool ret = true;
-	u64 *pte, root;
+	u64 *pte;
 
 	spin_lock_irqsave(&domain->lock, flags);
 
@@ -1438,8 +1449,7 @@ static bool increase_address_space(struct protection_domain *domain,
 	 * Device Table needs to be updated and flushed before the new root can
 	 * be published.
 	 */
-	root = amd_iommu_domain_encode_pgtable(pte, pgtable.mode);
-	atomic64_set(&domain->pt_root, root);
+	amd_iommu_domain_set_pgtable(domain, pte, pgtable.mode);
 
 	ret = true;
 
@@ -2319,7 +2329,7 @@ static void protection_domain_free(struct protection_domain *domain)
 		domain_id_free(domain->id);
 
 	amd_iommu_domain_get_pgtable(domain, &pgtable);
-	atomic64_set(&domain->pt_root, 0);
+	amd_iommu_domain_clr_pt_root(domain);
 	free_pagetable(&pgtable);
 
 	kfree(domain);
@@ -2327,7 +2337,7 @@ static void protection_domain_free(struct protection_domain *domain)
 
 static int protection_domain_init(struct protection_domain *domain, int mode)
 {
-	u64 *pt_root = NULL, root;
+	u64 *pt_root = NULL;
 
 	BUG_ON(mode < PAGE_MODE_NONE || mode > PAGE_MODE_6_LEVEL);
 
@@ -2343,8 +2353,7 @@ static int protection_domain_init(struct protection_domain *domain, int mode)
 			return -ENOMEM;
 	}
 
-	root = amd_iommu_domain_encode_pgtable(pt_root, mode);
-	atomic64_set(&domain->pt_root, root);
+	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
 
 	return 0;
 }
@@ -2713,8 +2722,8 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
 	/* First save pgtable configuration*/
 	amd_iommu_domain_get_pgtable(domain, &pgtable);
 
-	/* Update data structure */
-	atomic64_set(&domain->pt_root, 0);
+	/* Remove page-table from domain */
+	amd_iommu_domain_clr_pt_root(domain);
 
 	/* Make changes visible to IOMMUs */
 	update_domain(domain);
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root
  2020-06-26  8:05 [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel
  2020-06-26  8:05 ` [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel
@ 2020-06-26  8:05 ` Joerg Roedel
  2020-06-26 12:30 ` [PATCH v2 0/2] iommu/amd: Don't use atomic64_t " Qian Cai
  2 siblings, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2020-06-26  8:05 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, Joerg Roedel, linux-kernel

From: Joerg Roedel <jroedel@suse.de>

Using atomic64_t can be quite expensive, so use unsigned long instead.
This is safe because the write becomes visible atomically.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 +-
 drivers/iommu/amd/iommu.c           | 15 +++++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 30a5d412255a..f6f102282dda 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -468,7 +468,7 @@ struct protection_domain {
 				       iommu core code */
 	spinlock_t lock;	/* mostly used to lock the page table*/
 	u16 id;			/* the domain id written to the device table */
-	atomic64_t pt_root;	/* pgtable root and pgtable mode */
+	unsigned long pt_root;	/* pgtable root and pgtable mode */
 	int glx;		/* Number of levels for GCR3 table */
 	u64 *gcr3_tbl;		/* Guest CR3 table */
 	unsigned long flags;	/* flags to find out type of domain */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5286ddcfc2f9..aec585f47646 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -156,7 +156,12 @@ static struct protection_domain *to_pdomain(struct iommu_domain *dom)
 static void amd_iommu_domain_get_pgtable(struct protection_domain *domain,
 					 struct domain_pgtable *pgtable)
 {
-	u64 pt_root = atomic64_read(&domain->pt_root);
+	unsigned long pt_root;
+
+	/* Match the barrier in amd_iommu_domain_set_pt_root() */
+	smp_rmb();
+
+	pt_root = READ_ONCE(domain->pt_root);
 
 	pgtable->root = (u64 *)(pt_root & PAGE_MASK);
 	pgtable->mode = pt_root & 7; /* lowest 3 bits encode pgtable mode */
@@ -164,7 +169,13 @@ static void amd_iommu_domain_get_pgtable(struct protection_domain *domain,
 
 static void amd_iommu_domain_set_pt_root(struct protection_domain *domain, u64 root)
 {
-	atomic64_set(&domain->pt_root, root);
+	WRITE_ONCE(domain->pt_root, root);
+
+	/*
+	 * The new value needs to be gobally visible in case pt_root gets
+	 * cleared, so that the page-table can be safely freed.
+	 */
+	smp_wmb();
 }
 
 static void amd_iommu_domain_clr_pt_root(struct protection_domain *domain)
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root
  2020-06-26  8:05 [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel
  2020-06-26  8:05 ` [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel
  2020-06-26  8:05 ` [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel
@ 2020-06-26 12:30 ` Qian Cai
  2020-06-30  9:57   ` Joerg Roedel
  2 siblings, 1 reply; 5+ messages in thread
From: Qian Cai @ 2020-06-26 12:30 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, Joerg Roedel, linux-kernel



> On Jun 26, 2020, at 4:05 AM, Joerg Roedel <joro@8bytes.org> wrote:
> 
> a previous discussion pointed out that using atomic64_t for that
> purpose is a bit of overkill. This patch-set replaces it with unsigned
> long and introduces some helpers first to make the change more easy.

BTW, from the previous discussion, Linus mentioned,
 
“
The thing is, the 64-bit atomic reads/writes are very expensive on
32-bit x86. If it was just a native pointer, it would be much cheaper
than an "atomic64_t".
“

However, here we have AMD_IOMMU depend on x86_64, so I am wondering if it makes any sense to run this code on 32-bit x86 at all?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root
  2020-06-26 12:30 ` [PATCH v2 0/2] iommu/amd: Don't use atomic64_t " Qian Cai
@ 2020-06-30  9:57   ` Joerg Roedel
  0 siblings, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2020-06-30  9:57 UTC (permalink / raw)
  To: Qian Cai; +Cc: iommu, Joerg Roedel, linux-kernel

On Fri, Jun 26, 2020 at 08:30:21AM -0400, Qian Cai wrote:
> BTW, from the previous discussion, Linus mentioned,
>  
> “
> The thing is, the 64-bit atomic reads/writes are very expensive on
> 32-bit x86. If it was just a native pointer, it would be much cheaper
> than an "atomic64_t".
> “
> 
> However, here we have AMD_IOMMU depend on x86_64, so I am wondering if
> it makes any sense to run this code on 32-bit x86 at all?

No, it doesn't, the driver is not supported on 32bit and probably never
will. I skip this patch and only apply the first one, as it is an
improvement in itself.

Regards,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-06-30  9:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26  8:05 [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel
2020-06-26  8:05 ` [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel
2020-06-26  8:05 ` [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel
2020-06-26 12:30 ` [PATCH v2 0/2] iommu/amd: Don't use atomic64_t " Qian Cai
2020-06-30  9:57   ` Joerg Roedel

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).