* [PATCH 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root @ 2020-06-25 14:52 Joerg Roedel 2020-06-25 14:52 ` [PATCH 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel 2020-06-25 14:52 ` [PATCH 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel 0 siblings, 2 replies; 5+ messages in thread From: Joerg Roedel @ 2020-06-25 14:52 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 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 | 39 ++++++++++++++++++++--------- 2 files changed, 28 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 1/2] iommu/amd: Add helper functions to update domain->pt_root 2020-06-25 14:52 [PATCH 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel @ 2020-06-25 14:52 ` Joerg Roedel 2020-06-25 14:52 ` [PATCH 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel 1 sibling, 0 replies; 5+ messages in thread From: Joerg Roedel @ 2020-06-25 14:52 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 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root 2020-06-25 14:52 [PATCH 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel 2020-06-25 14:52 ` [PATCH 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel @ 2020-06-25 14:52 ` Joerg Roedel 2020-06-25 15:37 ` Qian Cai 1 sibling, 1 reply; 5+ messages in thread From: Joerg Roedel @ 2020-06-25 14:52 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 | 10 ++++++++-- 2 files changed, 9 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..b0e1dc58244e 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -156,7 +156,7 @@ 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 = domain->pt_root; pgtable->root = (u64 *)(pt_root & PAGE_MASK); pgtable->mode = pt_root & 7; /* lowest 3 bits encode pgtable mode */ @@ -164,7 +164,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); + 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 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root 2020-06-25 14:52 ` [PATCH 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel @ 2020-06-25 15:37 ` Qian Cai 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 1 reply; 5+ messages in thread From: Qian Cai @ 2020-06-25 15:37 UTC (permalink / raw) To: Joerg Roedel; +Cc: iommu, Joerg Roedel, linux-kernel On Thu, Jun 25, 2020 at 04:52:27PM +0200, Joerg Roedel wrote: > 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 | 10 ++++++++-- > 2 files changed, 9 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..b0e1dc58244e 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -156,7 +156,7 @@ 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 = domain->pt_root; The pt_root might be reload later in case of register pressure where the compiler decides to not store it as a stack variable, so it needs smp_rmb() here to match to the smp_wmb() in amd_iommu_domain_set_pt_root() to make the load visiable to all CPUs. Then, smp_rmb/wmb() wouldn't be able to deal with data races, so it needs, unsigned long 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 +164,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); > + 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 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root 2020-06-25 15:37 ` Qian Cai @ 2020-06-26 8:05 ` Joerg Roedel 0 siblings, 0 replies; 5+ messages in thread From: Joerg Roedel @ 2020-06-26 8:05 UTC (permalink / raw) To: Qian Cai; +Cc: iommu, Joerg Roedel, linux-kernel Hi Qian, On Thu, Jun 25, 2020 at 11:37:20AM -0400, Qian Cai wrote: > On Thu, Jun 25, 2020 at 04:52:27PM +0200, Joerg Roedel wrote: > > - u64 pt_root = atomic64_read(&domain->pt_root); > > + unsigned long pt_root = domain->pt_root; > > The pt_root might be reload later in case of register pressure where the > compiler decides to not store it as a stack variable, so it needs > smp_rmb() here to match to the smp_wmb() in > amd_iommu_domain_set_pt_root() to make the load visiable to all CPUs. > > Then, smp_rmb/wmb() wouldn't be able to deal with data races, so it > needs, > > unsigned long 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 +164,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); > > + domain->pt_root = root; > > WRITE_ONCE(domain->pt_root, root); Thanks for your review. I addressed your comments and will send an updated version shortly. 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-26 8:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-25 14:52 [PATCH 0/2] iommu/amd: Don't use atomic64_t for domain->pt_root Joerg Roedel 2020-06-25 14:52 ` [PATCH 1/2] iommu/amd: Add helper functions to update domain->pt_root Joerg Roedel 2020-06-25 14:52 ` [PATCH 2/2] iommu/amd: Use 'unsigned long' for domain->pt_root Joerg Roedel 2020-06-25 15:37 ` Qian Cai 2020-06-26 8:05 ` 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).