On 25/09/2020 15:34, John Garry wrote:
Indeed, I think that the mainline code has a bug:
If the initial allocation for the loaded/prev magazines fail (give NULL)
in init_iova_rcaches(), then in __iova_rcache_insert():
if (!iova_magazine_full(cpu_rcache->loaded)) {
can_insert = true;
If cpu_rcache->loaded == NULL, then can_insert is assigned true -> bang,
as I experimented, below. This needs to be fixed...
This looks better:
Subject: [PATCH] iommu/iova: Avoid double-negatives with magazine helpers
Expression !iova_magazine_full(mag) evaluates true when mag == NULL.
This falls over in __iova_rcache_insert() when loaded == NULL:
if (!iova_magazine_full(cpu_rcache->loaded)) {
can_insert = true;
...
if (can_insert)
iova_magazine_push(cpu_rcache->loaded, iova_pfn);
Here, can_insert is evaluated true, which is wrong. Members
loaded/prev can possibly be NULL if the initial allocations fail in
__iova_rcache_insert().
Let's stop using double-negatives, like !iova_magazine_full(), and use
iova_magazine_has_space() instead in this case. And similar for
!iova_magazine_empty().
Signed-off-by: John Garry <john.ga...@huawei.com>
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 5b4ffab7140b..42ca9d0f39b7 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -827,14 +827,18 @@ iova_magazine_free_pfns(struct iova_magazine *mag,
struct iova_domain *iovad)
mag->size = 0;
}
-static bool iova_magazine_full(struct iova_magazine *mag)
+static bool iova_magazine_has_space(struct iova_magazine *mag)
{
- return (mag && mag->size == IOVA_MAG_SIZE);
+ if (!mag)
+ return false;
+ return mag->size < IOVA_MAG_SIZE;
}
-static bool iova_magazine_empty(struct iova_magazine *mag)
+static bool iova_magazine_has_pfns(struct iova_magazine *mag)
{
- return (!mag || mag->size == 0);
+ if (!mag)
+ return false;
+ return mag->size;
}
static unsigned long iova_magazine_pop(struct iova_magazine *mag,
@@ -843,7 +847,7 @@ static unsigned long iova_magazine_pop(struct
iova_magazine *mag,
int i;
unsigned long pfn;
- BUG_ON(iova_magazine_empty(mag));
+ BUG_ON(!iova_magazine_has_pfns(mag));
/* Only fall back to the rbtree if we have no suitable pfns at all */
for (i = mag->size - 1; mag->pfns[i] > limit_pfn; i--)
@@ -859,7 +863,7 @@ static unsigned long iova_magazine_pop(struct
iova_magazine *mag,
static void iova_magazine_push(struct iova_magazine *mag, unsigned
long pfn)
{
- BUG_ON(iova_magazine_full(mag));
+ BUG_ON(!iova_magazine_has_space(mag));
mag->pfns[mag->size++] = pfn;
}
@@ -905,9 +909,9 @@ static bool __iova_rcache_insert(struct iova_domain
*iovad,
cpu_rcache = raw_cpu_ptr(rcache->cpu_rcaches);
spin_lock_irqsave(&cpu_rcache->lock, flags);
- if (!iova_magazine_full(cpu_rcache->loaded)) {
+ if (iova_magazine_has_space(cpu_rcache->loaded)) {
can_insert = true;
- } else if (!iova_magazine_full(cpu_rcache->prev)) {
+ } else if (iova_magazine_has_space(cpu_rcache->prev)) {
swap(cpu_rcache->prev, cpu_rcache->loaded);
can_insert = true;
} else {
@@ -915,7 +919,8 @@ static bool __iova_rcache_insert(struct iova_domain
*iovad,
if (new_mag) {
spin_lock(&rcache->lock);
- if (rcache->depot_size < MAX_GLOBAL_MAGS) {
+ if (rcache->depot_size < MAX_GLOBAL_MAGS &&
+ cpu_rcache->loaded) {
rcache->depot[rcache->depot_size++] =
cpu_rcache->loaded;
} else {
@@ -968,9 +973,9 @@ static unsigned long __iova_rcache_get(struct
iova_rcache *rcache,
cpu_rcache = raw_cpu_ptr(rcache->cpu_rcaches);
spin_lock_irqsave(&cpu_rcache->lock, flags);
- if (!iova_magazine_empty(cpu_rcache->loaded)) {
+ if (iova_magazine_has_pfns(cpu_rcache->loaded)) {
has_pfn = true;
- } else if (!iova_magazine_empty(cpu_rcache->prev)) {
+ } else if (iova_magazine_has_pfns(cpu_rcache->prev)) {
swap(cpu_rcache->prev, cpu_rcache->loaded);
has_pfn = true;
} else {
--
2.26.2
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
