Message ID | 20210716082755.428187-3-leobras.c@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | DDW + Indirect Mapping | expand |
Related | show |
On 16/07/2021 10:27, Leonardo Bras wrote: > @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table *tbl) > for (i = 0; i < tbl->nr_pools; i++) > spin_lock_nest_lock(&tbl->pools[i].lock, &tbl->large_pool.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); > + So if the table is not empty, we fail (EBUSY) but we now also completely overwrite the bitmap. It was in an unexpected state, but we're making it worse. Or am I missing something? Fred > for (i = 0; i < tbl->nr_pools; i++) > spin_unlock(&tbl->pools[i].lock); > spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
Hello Fred, thanks for this feedback! Sorry if I miss anything, this snippet was written for v1 over an year ago, and I have not taken a look at it ever since. On Mon, 2021-07-19 at 15:53 +0200, Frederic Barrat wrote: > > > On 16/07/2021 10:27, Leonardo Bras wrote: > > @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table > > *tbl) > > for (i = 0; i < tbl->nr_pools; i++) > > spin_lock_nest_lock(&tbl->pools[i].lock, &tbl- > > >large_pool.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); > > + > > > So if the table is not empty, we fail (EBUSY) but we now also > completely > overwrite the bitmap. It was in an unexpected state, but we're making > it > worse. Or am I missing something? IIRC there was a reason to do that at the time, but TBH I don't really remember it, and by looking at the code right now you seem to be correct about this causing trouble. I will send a v6 fixing it soon. Please review the remaining patches for some issue I may be missing. Alexey, any comments on that? > > Fred > Again, thank you for reviewing Fred! Best regards, Leonardo Bras
On 20/07/2021 15:38, Leonardo Brás wrote: > Hello Fred, thanks for this feedback! > > Sorry if I miss anything, this snippet was written for v1 over an year > ago, and I have not taken a look at it ever since. > > On Mon, 2021-07-19 at 15:53 +0200, Frederic Barrat wrote: >> >> >> On 16/07/2021 10:27, Leonardo Bras wrote: >>> @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table >>> *tbl) >>> for (i = 0; i < tbl->nr_pools; i++) >>> spin_lock_nest_lock(&tbl->pools[i].lock, &tbl- >>>> large_pool.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); >>> + >> >> >> So if the table is not empty, we fail (EBUSY) but we now also >> completely >> overwrite the bitmap. It was in an unexpected state, but we're making >> it >> worse. Or am I missing something? > > IIRC there was a reason to do that at the time, but TBH I don't really > remember it, and by looking at the code right now you seem to be > correct about this causing trouble. > > I will send a v6 fixing it soon. > Please review the remaining patches for some issue I may be missing. > > Alexey, any comments on that? Agree with Fred, this is a bug, EBUSY is not that unexpected :-/ Thanks, > >> >> Fred >> > > Again, thank you for reviewing Fred! > Best regards, > Leonardo Bras > > > > >
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 2af89a5e379f..b10bf58ae467 100644 --- a/arch/powerpc/kernel/iommu.c +++ b/arch/powerpc/kernel/iommu.c @@ -690,32 +690,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); } /* @@ -779,6 +771,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) { struct iommu_table *tbl; @@ -795,10 +803,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__); /* free bitmap */ @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table *tbl) for (i = 0; i < tbl->nr_pools; i++) spin_lock_nest_lock(&tbl->pools[i].lock, &tbl->large_pool.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);