| Message ID | 20200911170738.82818-6-leobras.c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | DDW Indirect Mapping | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (4b552a4cbf286ff9dcdab19153f3c1c7d1680fab) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 104 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On 12/09/2020 03:07, Leonardo Bras wrote: > Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, > > Having a function to check if the iommu table has any allocation helps > deciding if a tbl can be reset for using a new DMA window. > > It should be enough to replace all instances of !bitmap_empty(tbl...). > > iommu_table_in_use() skips reserved memory, so we don't need to worry about > releasing it before testing. This causes iommu_table_release_pages() to > become unnecessary, given it is only used to remove reserved memory for > testing. > > Also, only allow storing reserved memory values in tbl if they are valid > in the table, so there is no need to check it in the new helper. > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> > --- > arch/powerpc/include/asm/iommu.h | 1 + > arch/powerpc/kernel/iommu.c | 61 +++++++++++++++----------------- > 2 files changed, 30 insertions(+), 32 deletions(-) > > diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h > index 5032f1593299..2913e5c8b1f8 100644 > --- a/arch/powerpc/include/asm/iommu.h > +++ b/arch/powerpc/include/asm/iommu.h > @@ -154,6 +154,7 @@ extern int iommu_tce_table_put(struct iommu_table *tbl); > */ > extern struct iommu_table *iommu_init_table(struct iommu_table *tbl, > int nid, unsigned long res_start, unsigned long res_end); > +bool iommu_table_in_use(struct iommu_table *tbl); > > #define IOMMU_TABLE_GROUP_MAX_TABLES 2 > > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c > index ffb2637dc82b..c838da3d8f32 100644 > --- a/arch/powerpc/kernel/iommu.c > +++ b/arch/powerpc/kernel/iommu.c > @@ -655,34 +655,21 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl, > if (tbl->it_offset == 0) > set_bit(0, tbl->it_map); > > + /* Check if res_start..res_end is a valid range in the table */ > + if (res_start >= res_end || res_start < tbl->it_offset || > + res_end > (tbl->it_offset + tbl->it_size)) { > + tbl->it_reserved_start = tbl->it_offset; > + tbl->it_reserved_end = tbl->it_offset; This silently ignores overlapped range of the reserved area and the window which does not seem right. > + return; > + } > + > tbl->it_reserved_start = res_start; > tbl->it_reserved_end = res_end; > > - /* Check if res_start..res_end isn't empty and overlaps the table */ > - if (res_start && res_end && > - (tbl->it_offset + tbl->it_size < res_start || > - res_end < tbl->it_offset)) > - return; > - > for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) > set_bit(i - tbl->it_offset, tbl->it_map); > } > > -static void iommu_table_release_pages(struct iommu_table *tbl) > -{ > - int i; > - > - /* > - * In case we have reserved the first bit, we should not emit > - * the warning below. > - */ > - if (tbl->it_offset == 0) > - clear_bit(0, tbl->it_map); > - > - for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) > - clear_bit(i - tbl->it_offset, tbl->it_map); > -} > - > /* > * Build a iommu_table structure. This contains a bit map which > * is used to manage allocation of the tce space. > @@ -743,6 +730,23 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid, > return tbl; > } > > +bool iommu_table_in_use(struct iommu_table *tbl) > +{ > + unsigned long start = 0, end; > + > + /* ignore reserved bit0 */ > + if (tbl->it_offset == 0) > + start = 1; > + end = tbl->it_reserved_start - tbl->it_offset; > + if (find_next_bit(tbl->it_map, end, start) != end) > + return true; > + > + start = tbl->it_reserved_end - tbl->it_offset; > + end = tbl->it_size; > + return find_next_bit(tbl->it_map, end, start) != end; > + Unnecessary empty line. > +} > + > static void iommu_table_free(struct kref *kref) > { > unsigned long bitmap_sz; > @@ -759,10 +763,8 @@ static void iommu_table_free(struct kref *kref) > return; > } > > - iommu_table_release_pages(tbl); > - > /* verify that table contains no entries */ > - if (!bitmap_empty(tbl->it_map, tbl->it_size)) > + if (iommu_table_in_use(tbl)) > pr_warn("%s: Unexpected TCEs\n", __func__); > > /* calculate bitmap size in bytes */ > @@ -1068,18 +1070,13 @@ int iommu_take_ownership(struct iommu_table *tbl) > for (i = 0; i < tbl->nr_pools; i++) > spin_lock(&tbl->pools[i].lock); > > - iommu_table_release_pages(tbl); > - > - if (!bitmap_empty(tbl->it_map, tbl->it_size)) { > + if (iommu_table_in_use(tbl)) { > pr_err("iommu_tce: it_map is not empty"); > ret = -EBUSY; > - /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */ > - iommu_table_reserve_pages(tbl, tbl->it_reserved_start, > - tbl->it_reserved_end); > - } else { > - memset(tbl->it_map, 0xff, sz); > } > > + memset(tbl->it_map, 0xff, sz); > + > for (i = 0; i < tbl->nr_pools; i++) > spin_unlock(&tbl->pools[i].lock); > spin_unlock_irqrestore(&tbl->large_pool.lock, flags); >
Hello Alexey, thanks for the feedback! On Tue, 2020-09-29 at 13:57 +1000, Alexey Kardashevskiy wrote: > > On 12/09/2020 03:07, Leonardo Bras wrote: > > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c > > index ffb2637dc82b..c838da3d8f32 100644 > > --- a/arch/powerpc/kernel/iommu.c > > +++ b/arch/powerpc/kernel/iommu.c > > @@ -655,34 +655,21 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl, > > if (tbl->it_offset == 0) > > set_bit(0, tbl->it_map); > > > > > > > > > > + /* Check if res_start..res_end is a valid range in the table */ > > + if (res_start >= res_end || res_start < tbl->it_offset || > > + res_end > (tbl->it_offset + tbl->it_size)) { > > + tbl->it_reserved_start = tbl->it_offset; > > + tbl->it_reserved_end = tbl->it_offset; > > > This silently ignores overlapped range of the reserved area and the > window which does not seem right. Humm, that makes sense. Would it be better to do something like this? if (res_start < tbl->it_offset) res_start = tbl->it_offset; if (res_end > (tbl->it_offset + tbl->it_size)) res_end = tbl->it_offset + tbl->it_size; if (res_start >= res_end) { tbl->it_reserved_start = tbl->it_offset; tbl->it_reserved_end = tbl->it_offset; return; } > > + return; > > + } > > + > > tbl->it_reserved_start = res_start; > > tbl->it_reserved_end = res_end; > > - /* Check if res_start..res_end isn't empty and overlaps the table */ > > - if (res_start && res_end && > > - (tbl->it_offset + tbl->it_size < res_start || > > - res_end < tbl->it_offset)) > > - return; > > - > > for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) > > set_bit(i - tbl->it_offset, tbl->it_map); > > } > > +bool iommu_table_in_use(struct iommu_table *tbl) > > +{ > > + unsigned long start = 0, end; > > + > > + /* ignore reserved bit0 */ > > + if (tbl->it_offset == 0) > > + start = 1; > > + end = tbl->it_reserved_start - tbl->it_offset; > > + if (find_next_bit(tbl->it_map, end, start) != end) > > + return true; > > + > > + start = tbl->it_reserved_end - tbl->it_offset; > > + end = tbl->it_size; > > + return find_next_bit(tbl->it_map, end, start) != end; > > + > > Unnecessary empty line. Sure, removing. Thanks!
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h index 5032f1593299..2913e5c8b1f8 100644 --- a/arch/powerpc/include/asm/iommu.h +++ b/arch/powerpc/include/asm/iommu.h @@ -154,6 +154,7 @@ extern int iommu_tce_table_put(struct iommu_table *tbl); */ extern struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid, unsigned long res_start, unsigned long res_end); +bool iommu_table_in_use(struct iommu_table *tbl); #define IOMMU_TABLE_GROUP_MAX_TABLES 2 diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c index ffb2637dc82b..c838da3d8f32 100644 --- a/arch/powerpc/kernel/iommu.c +++ b/arch/powerpc/kernel/iommu.c @@ -655,34 +655,21 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl, if (tbl->it_offset == 0) set_bit(0, tbl->it_map); + /* Check if res_start..res_end is a valid range in the table */ + if (res_start >= res_end || res_start < tbl->it_offset || + res_end > (tbl->it_offset + tbl->it_size)) { + tbl->it_reserved_start = tbl->it_offset; + tbl->it_reserved_end = tbl->it_offset; + return; + } + tbl->it_reserved_start = res_start; tbl->it_reserved_end = res_end; - /* Check if res_start..res_end isn't empty and overlaps the table */ - if (res_start && res_end && - (tbl->it_offset + tbl->it_size < res_start || - res_end < tbl->it_offset)) - return; - for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) set_bit(i - tbl->it_offset, tbl->it_map); } -static void iommu_table_release_pages(struct iommu_table *tbl) -{ - int i; - - /* - * In case we have reserved the first bit, we should not emit - * the warning below. - */ - if (tbl->it_offset == 0) - clear_bit(0, tbl->it_map); - - for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) - clear_bit(i - tbl->it_offset, tbl->it_map); -} - /* * Build a iommu_table structure. This contains a bit map which * is used to manage allocation of the tce space. @@ -743,6 +730,23 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid, return tbl; } +bool iommu_table_in_use(struct iommu_table *tbl) +{ + unsigned long start = 0, end; + + /* ignore reserved bit0 */ + if (tbl->it_offset == 0) + start = 1; + end = tbl->it_reserved_start - tbl->it_offset; + if (find_next_bit(tbl->it_map, end, start) != end) + return true; + + start = tbl->it_reserved_end - tbl->it_offset; + end = tbl->it_size; + return find_next_bit(tbl->it_map, end, start) != end; + +} + static void iommu_table_free(struct kref *kref) { unsigned long bitmap_sz; @@ -759,10 +763,8 @@ static void iommu_table_free(struct kref *kref) return; } - iommu_table_release_pages(tbl); - /* verify that table contains no entries */ - if (!bitmap_empty(tbl->it_map, tbl->it_size)) + if (iommu_table_in_use(tbl)) pr_warn("%s: Unexpected TCEs\n", __func__); /* calculate bitmap size in bytes */ @@ -1068,18 +1070,13 @@ int iommu_take_ownership(struct iommu_table *tbl) for (i = 0; i < tbl->nr_pools; i++) spin_lock(&tbl->pools[i].lock); - iommu_table_release_pages(tbl); - - if (!bitmap_empty(tbl->it_map, tbl->it_size)) { + if (iommu_table_in_use(tbl)) { pr_err("iommu_tce: it_map is not empty"); ret = -EBUSY; - /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */ - iommu_table_reserve_pages(tbl, tbl->it_reserved_start, - tbl->it_reserved_end); - } else { - memset(tbl->it_map, 0xff, sz); } + memset(tbl->it_map, 0xff, sz); + for (i = 0; i < tbl->nr_pools; i++) spin_unlock(&tbl->pools[i].lock); spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Having a function to check if the iommu table has any allocation helps deciding if a tbl can be reset for using a new DMA window. It should be enough to replace all instances of !bitmap_empty(tbl...). iommu_table_in_use() skips reserved memory, so we don't need to worry about releasing it before testing. This causes iommu_table_release_pages() to become unnecessary, given it is only used to remove reserved memory for testing. Also, only allow storing reserved memory values in tbl if they are valid in the table, so there is no need to check it in the new helper. Signed-off-by: Leonardo Bras <leobras.c@gmail.com> --- arch/powerpc/include/asm/iommu.h | 1 + arch/powerpc/kernel/iommu.c | 61 +++++++++++++++----------------- 2 files changed, 30 insertions(+), 32 deletions(-)