* [PATCH] iommu/vt-d: Fix the size calculation of pasid table @ 2016-09-12 2:49 Xunlei Pang 2016-09-19 12:18 ` Joerg Roedel 0 siblings, 1 reply; 8+ messages in thread From: Xunlei Pang @ 2016-09-12 2:49 UTC (permalink / raw) To: Joerg Roedel; +Cc: iommu, linux-kernel, Xunlei Pang According to the vt-d spec, the size of pasid (state) entry is 8B which equals 3 in power of 2, the number of pasid (state) entries is (ecap_pss + 1) in power of 2. Thus the right size of pasid (state) table in power of 2 should be ecap_pss(iommu->ecap) plus "1+3=4" other than 7. Signed-off-by: Xunlei Pang <xlpang@redhat.com> --- drivers/iommu/intel-svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c index 8ebb353..cfa75c2 100644 --- a/drivers/iommu/intel-svm.c +++ b/drivers/iommu/intel-svm.c @@ -39,7 +39,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) struct page *pages; int order; - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; + order = ecap_pss(iommu->ecap) + 4 - PAGE_SHIFT; if (order < 0) order = 0; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-09-12 2:49 [PATCH] iommu/vt-d: Fix the size calculation of pasid table Xunlei Pang @ 2016-09-19 12:18 ` Joerg Roedel 2016-10-10 13:00 ` Xunlei Pang 2016-10-12 12:17 ` David Woodhouse 0 siblings, 2 replies; 8+ messages in thread From: Joerg Roedel @ 2016-09-19 12:18 UTC (permalink / raw) To: Xunlei Pang; +Cc: iommu, linux-kernel, David Woodhouse [Cc'ing David] On Mon, Sep 12, 2016 at 10:49:11AM +0800, Xunlei Pang wrote: > According to the vt-d spec, the size of pasid (state) entry is 8B > which equals 3 in power of 2, the number of pasid (state) entries > is (ecap_pss + 1) in power of 2. > > Thus the right size of pasid (state) table in power of 2 should be > ecap_pss(iommu->ecap) plus "1+3=4" other than 7. > > Signed-off-by: Xunlei Pang <xlpang@redhat.com> > --- > drivers/iommu/intel-svm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c > index 8ebb353..cfa75c2 100644 > --- a/drivers/iommu/intel-svm.c > +++ b/drivers/iommu/intel-svm.c > @@ -39,7 +39,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) > struct page *pages; > int order; > > - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; > + order = ecap_pss(iommu->ecap) + 4 - PAGE_SHIFT; > if (order < 0) > order = 0; The patch seems to be correct, but I'll let David comment on it first. Joerg ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-09-19 12:18 ` Joerg Roedel @ 2016-10-10 13:00 ` Xunlei Pang 2016-10-12 12:17 ` David Woodhouse 1 sibling, 0 replies; 8+ messages in thread From: Xunlei Pang @ 2016-10-10 13:00 UTC (permalink / raw) To: David Woodhouse; +Cc: Joerg Roedel, Xunlei Pang, iommu, linux-kernel Ping David for confirmation On 2016/09/19 at 20:18, Joerg Roedel wrote: > [Cc'ing David] > > On Mon, Sep 12, 2016 at 10:49:11AM +0800, Xunlei Pang wrote: >> According to the vt-d spec, the size of pasid (state) entry is 8B >> which equals 3 in power of 2, the number of pasid (state) entries >> is (ecap_pss + 1) in power of 2. >> >> Thus the right size of pasid (state) table in power of 2 should be >> ecap_pss(iommu->ecap) plus "1+3=4" other than 7. >> >> Signed-off-by: Xunlei Pang <xlpang@redhat.com> >> --- >> drivers/iommu/intel-svm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c >> index 8ebb353..cfa75c2 100644 >> --- a/drivers/iommu/intel-svm.c >> +++ b/drivers/iommu/intel-svm.c >> @@ -39,7 +39,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) >> struct page *pages; >> int order; >> >> - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; >> + order = ecap_pss(iommu->ecap) + 4 - PAGE_SHIFT; >> if (order < 0) >> order = 0; > The patch seems to be correct, but I'll let David comment on it first. > > > > Joerg > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-09-19 12:18 ` Joerg Roedel 2016-10-10 13:00 ` Xunlei Pang @ 2016-10-12 12:17 ` David Woodhouse 2016-10-30 12:18 ` David Woodhouse 1 sibling, 1 reply; 8+ messages in thread From: David Woodhouse @ 2016-10-12 12:17 UTC (permalink / raw) To: Joerg Roedel, Xunlei Pang; +Cc: iommu, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1417 bytes --] On Mon, 2016-09-19 at 14:18 +0200, Joerg Roedel wrote: > [Cc'ing David] > > On Mon, Sep 12, 2016 at 10:49:11AM +0800, Xunlei Pang wrote: > > > > According to the vt-d spec, the size of pasid (state) entry is 8B > > which equals 3 in power of 2, the number of pasid (state) entries > > is (ecap_pss + 1) in power of 2. > > > > Thus the right size of pasid (state) table in power of 2 should be > > ecap_pss(iommu->ecap) plus "1+3=4" other than 7. > > > > Signed-off-by: Xunlei Pang <xlpang@redhat.com> > > --- > > drivers/iommu/intel-svm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c > > index 8ebb353..cfa75c2 100644 > > --- a/drivers/iommu/intel-svm.c > > +++ b/drivers/iommu/intel-svm.c > > @@ -39,7 +39,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) > > struct page *pages; > > int order; > > > > - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; > > + order = ecap_pss(iommu->ecap) + 4 - PAGE_SHIFT; > > if (order < 0) > > order = 0; > > The patch seems to be correct, but I'll let David comment on it first. Yes, that looks correct. I think we may also need to limit it, because full 20-bit PASID support means we'll attempt an order 11 allocation. But that's certainly correct for now Acked-by: David Woodhouse <dwmw2@infradead.org> -- dwmw2 [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 5760 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-10-12 12:17 ` David Woodhouse @ 2016-10-30 12:18 ` David Woodhouse 2016-10-31 11:30 ` Xunlei Pang 2016-11-10 10:45 ` Joerg Roedel 0 siblings, 2 replies; 8+ messages in thread From: David Woodhouse @ 2016-10-30 12:18 UTC (permalink / raw) To: Joerg Roedel, Xunlei Pang; +Cc: iommu, linux-kernel, Mika Kuoppala [-- Attachment #1: Type: text/plain, Size: 4462 bytes --] On Wed, 2016-10-12 at 13:17 +0100, David Woodhouse wrote: > Yes, that looks correct. I think we may also need to limit it, because > full 20-bit PASID support means we'll attempt an order 11 allocation. > But that's certainly correct for now Actually, not quite correct. You fixed the allocation but not the free. And Mika had reported that even the *correct* allocation was failing because it was too large. So I've done it differently (untested)... ----- Subject: [PATCH] iommu/vt-d: Fix PASID table allocation Somehow I ended up with an off-by-three error in calculating the size of the PASID and PASID State tables, which triggers allocations failures as those tables unfortunately have to be physically contiguous. In fact, even the *correct* maximum size of 8MiB is problematic and is wont to lead to allocation failures. Since I have extracted a promise that this *will* be fixed in hardware, I'm happy to limit it on the current hardware to a maximum of 0x20000 PASIDs, which gives us 1MiB tables — still not ideal, but better than before. Reported by Mika Kuoppala <mika.kuoppala@linux.intel.com> and also by Xunlei Pang <xlpang@redhat.com> who submitted a simpler patch to fix only the allocation (and not the free) to the "correct" limit... which was still problematic. Signed-off-by: David Woodhouse <dwmw2@infradead.org> --- drivers/iommu/intel-svm.c | 28 +++++++++++++++++----------- include/linux/intel-iommu.h | 1 + 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c index 8ebb353..cb72e00 100644 --- a/drivers/iommu/intel-svm.c +++ b/drivers/iommu/intel-svm.c @@ -39,10 +39,18 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) struct page *pages; int order; - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; - if (order < 0) - order = 0; - + /* Start at 2 because it's defined as 1^(1+PSS) */ + iommu->pasid_max = 2 << ecap_pss(iommu->ecap); + + /* Eventually I'm promised we will get a multi-level PASID table + * and it won't have to be physically contiguous. Until then, + * limit the size because 8MiB contiguous allocations can be hard + * to come by. The limit of 0x20000, which is 1MiB for each of + * the PASID and PASID-state tables, is somewhat arbitrary. */ + if (iommu->pasid_max > 0x20000) + iommu->pasid_max = 0x20000; + + order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max); pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); if (!pages) { pr_warn("IOMMU: %s: Failed to allocate PASID table\n", @@ -53,6 +61,8 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order); if (ecap_dis(iommu->ecap)) { + /* Just making it explicit... */ + BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry)); pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); if (pages) iommu->pasid_state_table = page_address(pages); @@ -68,11 +78,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) int intel_svm_free_pasid_tables(struct intel_iommu *iommu) { - int order; - - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; - if (order < 0) - order = 0; + int order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max); if (iommu->pasid_table) { free_pages((unsigned long)iommu->pasid_table, order); @@ -371,8 +377,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ } svm->iommu = iommu; - if (pasid_max > 2 << ecap_pss(iommu->ecap)) - pasid_max = 2 << ecap_pss(iommu->ecap); + if (pasid_max > iommu->pasid_max) + pasid_max = iommu->pasid_max; /* Do not use PASID 0 in caching mode (virtualised IOMMU) */ ret = idr_alloc(&iommu->pasid_idr, svm, diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index 2d9b6500..d49e26c 100644 --- a/include/linux/intel-iommu.h +++ b/include/linux/intel-iommu.h @@ -429,6 +429,7 @@ struct intel_iommu { struct page_req_dsc *prq; unsigned char prq_name[16]; /* Name for PRQ interrupt */ struct idr pasid_idr; + u32 pasid_max; #endif struct q_inval *qi; /* Queued invalidation info */ u32 *iommu_state; /* Store iommu states between suspend and resume.*/ -- 2.5.5 -- dwmw2 [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 5760 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-10-30 12:18 ` David Woodhouse @ 2016-10-31 11:30 ` Xunlei Pang 2016-11-10 10:45 ` Joerg Roedel 1 sibling, 0 replies; 8+ messages in thread From: Xunlei Pang @ 2016-10-31 11:30 UTC (permalink / raw) To: David Woodhouse, Joerg Roedel, Xunlei Pang Cc: iommu, linux-kernel, Mika Kuoppala On 2016/10/30 at 20:18, David Woodhouse wrote: > On Wed, 2016-10-12 at 13:17 +0100, David Woodhouse wrote: >> Yes, that looks correct. I think we may also need to limit it, because >> full 20-bit PASID support means we'll attempt an order 11 allocation. >> But that's certainly correct for now > Actually, not quite correct. You fixed the allocation but not the free. > And Mika had reported that even the *correct* allocation was failing > because it was too large. So I've done it differently (untested)... Yes, your fix looks correct. Thanks, Xunlei > ----- > Subject: [PATCH] iommu/vt-d: Fix PASID table allocation > > Somehow I ended up with an off-by-three error in calculating the size of > the PASID and PASID State tables, which triggers allocations failures as > those tables unfortunately have to be physically contiguous. > > In fact, even the *correct* maximum size of 8MiB is problematic and is > wont to lead to allocation failures. Since I have extracted a promise > that this *will* be fixed in hardware, I'm happy to limit it on the > current hardware to a maximum of 0x20000 PASIDs, which gives us 1MiB > tables — still not ideal, but better than before. > > Reported by Mika Kuoppala <mika.kuoppala@linux.intel.com> and also by > Xunlei Pang <xlpang@redhat.com> who submitted a simpler patch to fix > only the allocation (and not the free) to the "correct" limit... which > was still problematic. > > Signed-off-by: David Woodhouse <dwmw2@infradead.org> > --- > drivers/iommu/intel-svm.c | 28 +++++++++++++++++----------- > include/linux/intel-iommu.h | 1 + > 2 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c > index 8ebb353..cb72e00 100644 > --- a/drivers/iommu/intel-svm.c > +++ b/drivers/iommu/intel-svm.c > @@ -39,10 +39,18 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) > struct page *pages; > int order; > > - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; > - if (order < 0) > - order = 0; > - > + /* Start at 2 because it's defined as 1^(1+PSS) */ > + iommu->pasid_max = 2 << ecap_pss(iommu->ecap); > + > + /* Eventually I'm promised we will get a multi-level PASID table > + * and it won't have to be physically contiguous. Until then, > + * limit the size because 8MiB contiguous allocations can be hard > + * to come by. The limit of 0x20000, which is 1MiB for each of > + * the PASID and PASID-state tables, is somewhat arbitrary. */ > + if (iommu->pasid_max > 0x20000) > + iommu->pasid_max = 0x20000; > + > + order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max); > pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); > if (!pages) { > pr_warn("IOMMU: %s: Failed to allocate PASID table\n", > @@ -53,6 +61,8 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) > pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order); > > if (ecap_dis(iommu->ecap)) { > + /* Just making it explicit... */ > + BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry)); > pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order); > if (pages) > iommu->pasid_state_table = page_address(pages); > @@ -68,11 +78,7 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu) > > int intel_svm_free_pasid_tables(struct intel_iommu *iommu) > { > - int order; > - > - order = ecap_pss(iommu->ecap) + 7 - PAGE_SHIFT; > - if (order < 0) > - order = 0; > + int order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max); > > if (iommu->pasid_table) { > free_pages((unsigned long)iommu->pasid_table, order); > @@ -371,8 +377,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ > } > svm->iommu = iommu; > > - if (pasid_max > 2 << ecap_pss(iommu->ecap)) > - pasid_max = 2 << ecap_pss(iommu->ecap); > + if (pasid_max > iommu->pasid_max) > + pasid_max = iommu->pasid_max; > > /* Do not use PASID 0 in caching mode (virtualised IOMMU) */ > ret = idr_alloc(&iommu->pasid_idr, svm, > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h > index 2d9b6500..d49e26c 100644 > --- a/include/linux/intel-iommu.h > +++ b/include/linux/intel-iommu.h > @@ -429,6 +429,7 @@ struct intel_iommu { > struct page_req_dsc *prq; > unsigned char prq_name[16]; /* Name for PRQ interrupt */ > struct idr pasid_idr; > + u32 pasid_max; > #endif > struct q_inval *qi; /* Queued invalidation info */ > u32 *iommu_state; /* Store iommu states between suspend and resume.*/ > -- > 2.5.5 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-10-30 12:18 ` David Woodhouse 2016-10-31 11:30 ` Xunlei Pang @ 2016-11-10 10:45 ` Joerg Roedel 2016-11-10 11:02 ` David Woodhouse 1 sibling, 1 reply; 8+ messages in thread From: Joerg Roedel @ 2016-11-10 10:45 UTC (permalink / raw) To: David Woodhouse; +Cc: Xunlei Pang, iommu, linux-kernel, Mika Kuoppala Hi David, On Sun, Oct 30, 2016 at 06:18:22AM -0600, David Woodhouse wrote: > + /* Start at 2 because it's defined as 1^(1+PSS) */ This probably means 2^(1+PSS), right? > + iommu->pasid_max = 2 << ecap_pss(iommu->ecap); Otherwise the patch looks good. Do you want to send it upstream yourself or should I pick it up? Joerg ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix the size calculation of pasid table 2016-11-10 10:45 ` Joerg Roedel @ 2016-11-10 11:02 ` David Woodhouse 0 siblings, 0 replies; 8+ messages in thread From: David Woodhouse @ 2016-11-10 11:02 UTC (permalink / raw) To: Joerg Roedel; +Cc: Xunlei Pang, iommu, linux-kernel, Mika Kuoppala [-- Attachment #1: Type: text/plain, Size: 576 bytes --] On Thu, 2016-11-10 at 11:45 +0100, Joerg Roedel wrote: > Hi David, > > On Sun, Oct 30, 2016 at 06:18:22AM -0600, David Woodhouse wrote: > > + /* Start at 2 because it's defined as 1^(1+PSS) */ > > This probably means 2^(1+PSS), right? Or 1 << (1+PSS). Yeah. > > + iommu->pasid_max = 2 << ecap_pss(iommu->ecap); > > Otherwise the patch looks good. Do you want to send it upstream yourself > or should I pick it up? I'll let it sit in -next for a day or two more once I've fixed the above, then ask Linus to pull it. Thanks. -- dwmw2 [-- Attachment #2: smime.p7s --] [-- Type: application/x-pkcs7-signature, Size: 5760 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-10 11:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-12 2:49 [PATCH] iommu/vt-d: Fix the size calculation of pasid table Xunlei Pang 2016-09-19 12:18 ` Joerg Roedel 2016-10-10 13:00 ` Xunlei Pang 2016-10-12 12:17 ` David Woodhouse 2016-10-30 12:18 ` David Woodhouse 2016-10-31 11:30 ` Xunlei Pang 2016-11-10 10:45 ` Joerg Roedel 2016-11-10 11:02 ` David Woodhouse
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).