* [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 related [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 related [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