Re: [PATCH 1/2] iommu/iova: Flush CPU rcache for when a depot fills

[John Garry <john.garry@huawei.com>]

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.garry@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