Messages in this thread Patch in this message | | | Date | Mon, 10 May 2021 17:34:07 +1000 | Subject | Re: [PATCH v4 02/11] powerpc/kernel/iommu: Add new iommu_table_in_use() helper | From | Alexey Kardashevskiy <> |
| |
On 5/1/21 02:31, Leonardo Bras wrote: > 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 | 65 ++++++++++++++++---------------- > 2 files changed, 34 insertions(+), 32 deletions(-) > > diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h > index deef7c94d7b6..bf3b84128525 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 ad82dda81640..5e168bd91401 100644 > --- a/arch/powerpc/kernel/iommu.c > +++ b/arch/powerpc/kernel/iommu.c > @@ -691,32 +691,24 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl, > if (tbl->it_offset == 0) > set_bit(0, tbl->it_map); > > - 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; > + if (res_start < tbl->it_offset) > + res_start = tbl->it_offset; > > - for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) > - set_bit(i - tbl->it_offset, tbl->it_map); > -} > + if (res_end > (tbl->it_offset + tbl->it_size)) > + res_end = tbl->it_offset + tbl->it_size; > > -static void iommu_table_release_pages(struct iommu_table *tbl) > -{ > - int i; > + /* Check if res_start..res_end is a valid range in the table */ > + if (res_start >= res_end) { > + tbl->it_reserved_start = tbl->it_offset; > + tbl->it_reserved_end = tbl->it_offset; > + return; > + } > > - /* > - * 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); > + tbl->it_reserved_start = res_start; > + tbl->it_reserved_end = res_end; > > for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i) > - clear_bit(i - tbl->it_offset, tbl->it_map); > + set_bit(i - tbl->it_offset, tbl->it_map);
git produced a messy chunk here. The new logic is:
static void iommu_table_reserve_pages(struct iommu_table *tbl, unsigned long res_start, unsigned long res_end) { int i;
WARN_ON_ONCE(res_end < res_start); /* * Reserve page 0 so it will not be used for any mappings. * This avoids buggy drivers that consider page 0 to be invalid * to crash the machine or even lose data. */ if (tbl->it_offset == 0) set_bit(0, tbl->it_map);
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;
/* Check if res_start..res_end is a valid range in the table */ if (res_start >= res_end) { tbl->it_reserved_start = tbl->it_offset; tbl->it_reserved_end = tbl->it_offset; return; }
It is just hard to read. A code reviewer would assume res_end >= res_start (as there is WARN_ON) but later we allow res_end to be lesser than res_start.
but may be it is just me :) Otherwise looks good.
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> } > > /* > @@ -781,6 +773,22 @@ 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; > @@ -799,10 +807,8 @@ static void iommu_table_free(struct kref *kref) > > iommu_debugfs_del(tbl); > > - 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 */ > @@ -1108,18 +1114,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); >
-- Alexey
| |