* [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root @ 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root @ 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 0 replies; 10+ messages in thread From: Joerg Roedel @ 2020-06-26 8:05 UTC (permalink / raw) To: Joerg Roedel Cc: iommu, linux-kernel, Suravee Suthikulpanit, Qian Cai, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root 2020-06-26 8:05 ` Joerg Roedel @ 2020-06-26 8:05 ` Joerg Roedel -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [PATCH v2 1/2] iommu/amd: Add helper functions to update domain->pt_root @ 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 0 replies; 10+ messages in thread From: Joerg Roedel @ 2020-06-26 8:05 UTC (permalink / raw) To: Joerg Roedel Cc: iommu, linux-kernel, Suravee Suthikulpanit, Qian Cai, Joerg Roedel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root 2020-06-26 8:05 ` Joerg Roedel @ 2020-06-26 8:05 ` Joerg Roedel -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root @ 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 0 replies; 10+ messages in thread From: Joerg Roedel @ 2020-06-26 8:05 UTC (permalink / raw) To: Joerg Roedel Cc: iommu, linux-kernel, Suravee Suthikulpanit, Qian Cai, Joerg Roedel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [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 12:30 ` Qian Cai -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root @ 2020-06-26 12:30 ` Qian Cai 0 siblings, 0 replies; 10+ messages in thread From: Qian Cai @ 2020-06-26 12:30 UTC (permalink / raw) To: Joerg Roedel; +Cc: iommu, linux-kernel, Suravee Suthikulpanit, Joerg Roedel > 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root 2020-06-26 12:30 ` Qian Cai @ 2020-06-30 9:57 ` Joerg Roedel -1 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH v2 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root @ 2020-06-30 9:57 ` Joerg Roedel 0 siblings, 0 replies; 10+ messages in thread From: Joerg Roedel @ 2020-06-30 9:57 UTC (permalink / raw) To: Qian Cai; +Cc: iommu, linux-kernel, Suravee Suthikulpanit, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-06-30 9:57 UTC | newest] Thread overview: 10+ 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 ` 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 8:05 ` [PATCH v2 2/2] iommu/amd: Use 'unsigned long' for 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 2020-06-26 12:30 ` Qian Cai 2020-06-30 9:57 ` Joerg Roedel 2020-06-30 9:57 ` Joerg Roedel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.