xlat_tables_v2: Revert recent changes to remove recursion

This commit reverts the following commits:

- c54c7fc3 ("xlat_tables_v2: print xlat tables without recursion")
- db8cac2d ("xlat_tables_v2: unmap region without recursion.")
- 0ffe2692

 ("xlat_tables_v2: map region without recursion.")

This was part of PR#1843.

A problem has been detected in one of our test run configurations
involving dynamic mapping of regions and it is blocking the next
release. Until the problem can be solved, it is safer to revert
the changes.

Change-Id: I3d5456e4dbebf291c8b74939c6fb02a912e0903b
Signed-off-by: Antonio Nino Diaz <antonio.ninodiaz@arm.com>

lib/xlat_tables_v2/xlat_tables_core.c

@@ -325,9 +325,8 @@ static action_t xlat_tables_unmap_region_action(const mmap_region_t *mm,
 
 	return action;
 }
-
 /*
- * Function that writes to the translation tables and unmaps the
+ * Recursive function that writes to the translation tables and unmaps the
  * specified region.
  */
 static void xlat_tables_unmap_region(xlat_ctx_t *ctx, mmap_region_t *mm,
@@ -338,137 +337,70 @@ static void xlat_tables_unmap_region(xlat_ctx_t *ctx, mmap_region_t *mm,
 {
 	assert((level >= ctx->base_level) && (level <= XLAT_TABLE_LEVEL_MAX));
 
-	/*
-	 * data structure to track DESC_TABLE entry before iterate into subtable
-	 * of next translation level. it will be used to restore previous level
-	 * after finish subtable iteration.
-	 */
-	struct desc_table_unmap {
-		uint64_t *table_base;
-		uintptr_t table_idx_va;
-		unsigned int idx;
-	} desc_tables[XLAT_TABLE_LEVEL_MAX + 1] = {
-		{NULL, 0U, XLAT_TABLE_ENTRIES}, };
-
-	unsigned int this_level = level;
-	uint64_t *this_base = table_base;
-	unsigned int max_entries = table_entries;
-	size_t level_size = XLAT_BLOCK_SIZE(this_level);
-	unsigned int table_idx;
+	uint64_t *subtable;
+	uint64_t desc;
+
 	uintptr_t table_idx_va;
+	uintptr_t table_idx_end_va; /* End VA of this entry */
 
 	uintptr_t region_end_va = mm->base_va + mm->size - 1U;
 
+	unsigned int table_idx;
+
 	table_idx_va = xlat_tables_find_start_va(mm, table_base_va, level);
 	table_idx = xlat_tables_va_to_index(table_base_va, table_idx_va, level);
 
-	while (this_base != NULL) {
-
-		uint64_t desc;
-		uint64_t desc_type;
-		uintptr_t table_idx_end_va; /* End VA of this entry */
-		action_t action;
-
-		/* finish current xlat level iteration. */
-		if (table_idx >= max_entries) {
-			if (this_level > ctx->base_level) {
-				xlat_table_dec_regions_count(ctx, this_base);
-			}
-
-			if (this_level > level) {
-				uint64_t *subtable;
-
-				/* back from subtable iteration, restore
-				 * previous DESC_TABLE entry.
-				 */
-				this_level--;
-				this_base = desc_tables[this_level].table_base;
-				table_idx = desc_tables[this_level].idx;
-				table_idx_va =
-					desc_tables[this_level].table_idx_va;
-				level_size = XLAT_BLOCK_SIZE(this_level);
-
-				if (this_level == level) {
-					max_entries = table_entries;
-				} else {
-					max_entries = XLAT_TABLE_ENTRIES;
-				}
-
-				desc = this_base[table_idx];
-				subtable =  (uint64_t *)(uintptr_t)(desc & TABLE_ADDR_MASK);
-				/*
-				 * If the subtable is now empty, remove its reference.
-				 */
-				if (xlat_table_is_empty(ctx, subtable)) {
-					this_base[table_idx] = INVALID_DESC;
-					xlat_arch_tlbi_va(table_idx_va,
-							ctx->xlat_regime);
-				}
-				table_idx++;
-				table_idx_va += level_size;
-
-			} else {
-				/* reached end of top level, exit.*/
-				this_base = NULL;
-				break;
-			}
-
-		}
-
-		/* If reached the end of the region, stop iterating entries in
-		 * current xlat level.
-		 */
-		if (region_end_va <= table_idx_va) {
-			table_idx = max_entries;
-			continue;
-		}
+	while (table_idx < table_entries) {
 
+		table_idx_end_va = table_idx_va + XLAT_BLOCK_SIZE(level) - 1U;
 
-		table_idx_end_va = table_idx_va + XLAT_BLOCK_SIZE(this_level) - 1U;
+		desc = table_base[table_idx];
+		uint64_t desc_type = desc & DESC_MASK;
 
-		desc = this_base[table_idx];
-		desc_type = desc & DESC_MASK;
-
-		action = xlat_tables_unmap_region_action(mm, table_idx_va,
-							 table_idx_end_va,
-							 this_level,
-							 desc_type);
+		action_t action = xlat_tables_unmap_region_action(mm,
+				table_idx_va, table_idx_end_va, level,
+				desc_type);
 
 		if (action == ACTION_WRITE_BLOCK_ENTRY) {
-			this_base[table_idx] = INVALID_DESC;
+
+			table_base[table_idx] = INVALID_DESC;
 			xlat_arch_tlbi_va(table_idx_va, ctx->xlat_regime);
 
-			table_idx++;
-			table_idx_va += level_size;
 		} else if (action == ACTION_RECURSE_INTO_TABLE) {
 
-			uint64_t *subtable;
-			uintptr_t base_va;
-
 			subtable = (uint64_t *)(uintptr_t)(desc & TABLE_ADDR_MASK);
 
-			desc_tables[this_level].table_base = this_base;
-			desc_tables[this_level].table_idx_va = table_idx_va;
-			base_va = table_idx_va;
-			desc_tables[this_level].idx = table_idx;
-
-			this_base = subtable;
-			this_level++;
-
-			max_entries = XLAT_TABLE_ENTRIES;
-			level_size = XLAT_BLOCK_SIZE(this_level);
+			/* Recurse to write into subtable */
+			xlat_tables_unmap_region(ctx, mm, table_idx_va,
+						 subtable, XLAT_TABLE_ENTRIES,
+						 level + 1U);
+#if !(HW_ASSISTED_COHERENCY || WARMBOOT_ENABLE_DCACHE_EARLY)
+			xlat_clean_dcache_range((uintptr_t)subtable,
+				XLAT_TABLE_ENTRIES * sizeof(uint64_t));
+#endif
+			/*
+			 * If the subtable is now empty, remove its reference.
+			 */
+			if (xlat_table_is_empty(ctx, subtable)) {
+				table_base[table_idx] = INVALID_DESC;
+				xlat_arch_tlbi_va(table_idx_va,
+						  ctx->xlat_regime);
+			}
 
-			table_idx_va = xlat_tables_find_start_va(mm,
-					base_va, this_level);
-			table_idx = xlat_tables_va_to_index(base_va,
-					table_idx_va, this_level);
 		} else {
 			assert(action == ACTION_NONE);
-
-			table_idx++;
-			table_idx_va += level_size;
 		}
+
+		table_idx++;
+		table_idx_va += XLAT_BLOCK_SIZE(level);
+
+		/* If reached the end of the region, exit */
+		if (region_end_va <= table_idx_va)
+			break;
 	}
+
+	if (level > ctx->base_level)
+		xlat_table_dec_regions_count(ctx, table_base);
 }
 
 #endif /* PLAT_XLAT_TABLES_DYNAMIC */
@@ -605,169 +537,105 @@ static action_t xlat_tables_map_region_action(const mmap_region_t *mm,
 }
 
 /*
- * Function that writes to the translation tables and maps the
+ * Recursive function that writes to the translation tables and maps the
  * specified region. On success, it returns the VA of the last byte that was
  * successfully mapped. On error, it returns the VA of the next entry that
  * should have been mapped.
  */
 static uintptr_t xlat_tables_map_region(xlat_ctx_t *ctx, mmap_region_t *mm,
-				   const uintptr_t table_base_va,
+				   uintptr_t table_base_va,
 				   uint64_t *const table_base,
 				   unsigned int table_entries,
 				   unsigned int level)
 {
-
 	assert((level >= ctx->base_level) && (level <= XLAT_TABLE_LEVEL_MAX));
 
-	/*
-	 * data structure to track DESC_TABLE entry before iterate into subtable
-	 * of next translation level. it will be used to restore previous level
-	 * after finish subtable iteration.
-	 */
-	struct desc_table_map {
-		uint64_t *table_base;
-		uintptr_t table_idx_va;
-		unsigned int idx;
-	} desc_tables[XLAT_TABLE_LEVEL_MAX + 1] = {
-		{NULL, 0U, XLAT_TABLE_ENTRIES}, };
-
-	unsigned int this_level = level;
-	uint64_t *this_base = table_base;
-	unsigned int max_entries = table_entries;
-	size_t level_size = XLAT_BLOCK_SIZE(this_level);
 	uintptr_t mm_end_va = mm->base_va + mm->size - 1U;
 
 	uintptr_t table_idx_va;
+	unsigned long long table_idx_pa;
+
+	uint64_t *subtable;
+	uint64_t desc;
+
 	unsigned int table_idx;
 
 	table_idx_va = xlat_tables_find_start_va(mm, table_base_va, level);
 	table_idx = xlat_tables_va_to_index(table_base_va, table_idx_va, level);
 
-	while (this_base != NULL) {
-
-		uint64_t desc;
-		uint64_t desc_type;
-		unsigned long long table_idx_pa;
-		action_t action;
-
-		/* finish current xlat level iteration. */
-		if (table_idx >= max_entries) {
-			if (this_level <= level) {
-				this_base = NULL;
-				break;
-			} else {
-
-				/* back from subtable iteration, restore
-				 * previous DESC_TABLE entry.
-				 */
-				this_level--;
-				level_size = XLAT_BLOCK_SIZE(this_level);
-				this_base = desc_tables[this_level].table_base;
-				table_idx = desc_tables[this_level].idx;
-				if (this_level == level) {
-					max_entries = table_entries;
-				} else {
-					max_entries = XLAT_TABLE_ENTRIES;
-				}
-#if !(HW_ASSISTED_COHERENCY || WARMBOOT_ENABLE_DCACHE_EARLY)
-				uintptr_t subtable;
-				desc = this_base[table_idx];
-				subtable = (uintptr_t)(desc & TABLE_ADDR_MASK);
-				xlat_clean_dcache_range(subtable,
-					XLAT_TABLE_ENTRIES * sizeof(uint64_t));
+#if PLAT_XLAT_TABLES_DYNAMIC
+	if (level > ctx->base_level)
+		xlat_table_inc_regions_count(ctx, table_base);
 #endif
 
-				table_idx++;
-				table_idx_va =
-					desc_tables[this_level].table_idx_va +
-					level_size;
-			}
-		}
+	while (table_idx < table_entries) {
 
-		desc = this_base[table_idx];
-		desc_type = desc & DESC_MASK;
+		desc = table_base[table_idx];
 
 		table_idx_pa = mm->base_pa + table_idx_va - mm->base_va;
 
-		/* If reached the end of the region, simply exit since we
-		 * already write all BLOCK entries and create all required
-		 * subtables.
-		 */
-		if (mm_end_va <= table_idx_va) {
-			this_base = NULL;
-			break;
-		}
-
-		action = xlat_tables_map_region_action(mm, desc_type,
-				table_idx_pa, table_idx_va, this_level);
+		action_t action = xlat_tables_map_region_action(mm,
+			(uint32_t)(desc & DESC_MASK), table_idx_pa,
+			table_idx_va, level);
 
 		if (action == ACTION_WRITE_BLOCK_ENTRY) {
-			this_base[table_idx] = xlat_desc(ctx, mm->attr,
-					table_idx_pa, this_level);
-			table_idx++;
-			table_idx_va += level_size;
-		} else if (action == ACTION_CREATE_NEW_TABLE) {
 
-			uintptr_t base_va;
+			table_base[table_idx] =
+				xlat_desc(ctx, (uint32_t)mm->attr, table_idx_pa,
+					  level);
+
+		} else if (action == ACTION_CREATE_NEW_TABLE) {
+			uintptr_t end_va;
 
-			uint64_t *subtable = xlat_table_get_empty(ctx);
+			subtable = xlat_table_get_empty(ctx);
 			if (subtable == NULL) {
-				/* Not enough free tables to map this region. */
+				/* Not enough free tables to map this region */
 				return table_idx_va;
 			}
 
 			/* Point to new subtable from this one. */
-			this_base[table_idx] = TABLE_DESC | (unsigned long)subtable;
-
-			desc_tables[this_level].table_base = this_base;
-			desc_tables[this_level].table_idx_va = table_idx_va;
-			desc_tables[this_level].idx = table_idx;
-			base_va = table_idx_va;
-
-			this_level++;
-			this_base = subtable;
-			level_size = XLAT_BLOCK_SIZE(this_level);
-			table_idx_va = xlat_tables_find_start_va(mm, base_va,
-					this_level);
-			table_idx = xlat_tables_va_to_index(base_va,
-					table_idx_va, this_level);
-			max_entries = XLAT_TABLE_ENTRIES;
+			table_base[table_idx] = TABLE_DESC | (unsigned long)subtable;
 
-#if PLAT_XLAT_TABLES_DYNAMIC
-			if (this_level > ctx->base_level) {
-				xlat_table_inc_regions_count(ctx, subtable);
-			}
+			/* Recurse to write into subtable */
+			end_va = xlat_tables_map_region(ctx, mm, table_idx_va,
+					       subtable, XLAT_TABLE_ENTRIES,
+					       level + 1U);
+#if !(HW_ASSISTED_COHERENCY || WARMBOOT_ENABLE_DCACHE_EARLY)
+			xlat_clean_dcache_range((uintptr_t)subtable,
+				XLAT_TABLE_ENTRIES * sizeof(uint64_t));
 #endif
+			if (end_va !=
+				(table_idx_va + XLAT_BLOCK_SIZE(level) - 1U))
+				return end_va;
 
 		} else if (action == ACTION_RECURSE_INTO_TABLE) {
+			uintptr_t end_va;
 
-			uintptr_t base_va;
-			uint64_t *subtable = (uint64_t *)(uintptr_t)(desc & TABLE_ADDR_MASK);
-
-			desc_tables[this_level].table_base = this_base;
-			desc_tables[this_level].table_idx_va = table_idx_va;
-			desc_tables[this_level].idx = table_idx;
-			base_va = table_idx_va;
-
-			this_level++;
-			level_size = XLAT_BLOCK_SIZE(this_level);
-			table_idx_va = xlat_tables_find_start_va(mm, base_va,
-					this_level);
-			table_idx = xlat_tables_va_to_index(base_va,
-					table_idx_va, this_level);
-			this_base = subtable;
-			max_entries = XLAT_TABLE_ENTRIES;
-
-#if PLAT_XLAT_TABLES_DYNAMIC
-			if (this_level > ctx->base_level) {
-				xlat_table_inc_regions_count(ctx, subtable);
-			}
+			subtable = (uint64_t *)(uintptr_t)(desc & TABLE_ADDR_MASK);
+			/* Recurse to write into subtable */
+			end_va = xlat_tables_map_region(ctx, mm, table_idx_va,
+					       subtable, XLAT_TABLE_ENTRIES,
+					       level + 1U);
+#if !(HW_ASSISTED_COHERENCY || WARMBOOT_ENABLE_DCACHE_EARLY)
+			xlat_clean_dcache_range((uintptr_t)subtable,
+				XLAT_TABLE_ENTRIES * sizeof(uint64_t));
 #endif
+			if (end_va !=
+				(table_idx_va + XLAT_BLOCK_SIZE(level) - 1U))
+				return end_va;
+
 		} else {
+
 			assert(action == ACTION_NONE);
-			table_idx++;
-			table_idx_va += level_size;
+
 		}
+
+		table_idx++;
+		table_idx_va += XLAT_BLOCK_SIZE(level);
+
+		/* If reached the end of the region, exit */
+		if (mm_end_va <= table_idx_va)
+			break;
 	}
 
 	return table_idx_va - 1U;

lib/xlat_tables_v2/xlat_tables_utils.c

@@ -109,7 +109,7 @@ static const char *invalid_descriptors_ommited =
 		"%s(%d invalid descriptors omitted)\n";
 
 /*
- * Function that reads the translation tables passed as an argument
+ * Recursive function that reads the translation tables passed as an argument
  * and prints their status.
  */
 static void xlat_tables_print_internal(xlat_ctx_t *ctx, uintptr_t table_base_va,
@@ -118,23 +118,10 @@ static void xlat_tables_print_internal(xlat_ctx_t *ctx, uintptr_t table_base_va,
 {
 	assert(level <= XLAT_TABLE_LEVEL_MAX);
 
-	/*
-	 * data structure to track DESC_TABLE entry before iterate into subtable
-	 * of next translation level. it will be restored after return from
-	 * subtable iteration.
-	 */
-	struct desc_table {
-		const uint64_t *table_base;
-		uintptr_t table_idx_va;
-		unsigned int idx;
-	} desc_tables[XLAT_TABLE_LEVEL_MAX + 1] = {
-		{NULL, 0U, XLAT_TABLE_ENTRIES}, };
-	unsigned int this_level = level;
-	const uint64_t *this_base = table_base;
-	unsigned int max_entries = table_entries;
-	size_t level_size = XLAT_BLOCK_SIZE(this_level);
-	unsigned int table_idx = 0U;
+	uint64_t desc;
 	uintptr_t table_idx_va = table_base_va;
+	unsigned int table_idx = 0U;
+	size_t level_size = XLAT_BLOCK_SIZE(level);
 
 	/*
 	 * Keep track of how many invalid descriptors are counted in a row.
@@ -144,110 +131,67 @@ static void xlat_tables_print_internal(xlat_ctx_t *ctx, uintptr_t table_base_va,
 	 */
 	int invalid_row_count = 0;
 
-	while (this_base != NULL) {
-		/* finish current xlat level */
-		if (table_idx >= max_entries) {
+	while (table_idx < table_entries) {
+
+		desc = table_base[table_idx];
+
+		if ((desc & DESC_MASK) == INVALID_DESC) {
+
+			if (invalid_row_count == 0) {
+				printf("%sVA:0x%lx size:0x%zx\n",
+				       level_spacers[level],
+				       table_idx_va, level_size);
+			}
+			invalid_row_count++;
+
+		} else {
+
 			if (invalid_row_count > 1) {
 				printf(invalid_descriptors_ommited,
-					  level_spacers[this_level],
-					  invalid_row_count - 1);
+				       level_spacers[level],
+				       invalid_row_count - 1);
 			}
 			invalid_row_count = 0;
 
-			/* no parent level to iterate. */
-			if (this_level <= level) {
-				this_base = NULL;
-				table_idx = max_entries + 1;
-			} else {
-				/* retore previous DESC_TABLE entry and start
-				 * to iterate.
-				 */
-				this_level--;
-				level_size = XLAT_BLOCK_SIZE(this_level);
-				this_base = desc_tables[this_level].table_base;
-				table_idx = desc_tables[this_level].idx;
-				table_idx_va =
-					desc_tables[this_level].table_idx_va;
-				if (this_level == level) {
-					max_entries = table_entries;
-				} else {
-					max_entries = XLAT_TABLE_ENTRIES;
-				}
-
-				assert(this_base != NULL);
-			}
-		} else {
-			uint64_t desc = this_base[table_idx];
-
-			if ((desc & DESC_MASK) == INVALID_DESC) {
-				if (invalid_row_count == 0) {
-					printf("%sVA:0x%lx size:0x%zx\n",
-						  level_spacers[this_level],
-						  table_idx_va, level_size);
-				}
-				invalid_row_count++;
-				table_idx++;
-				table_idx_va += level_size;
-			} else {
-				if (invalid_row_count > 1) {
-					printf(invalid_descriptors_ommited,
-						  level_spacers[this_level],
-						  invalid_row_count - 1);
-				}
-				invalid_row_count = 0;
+			/*
+			 * Check if this is a table or a block. Tables are only
+			 * allowed in levels other than 3, but DESC_PAGE has the
+			 * same value as DESC_TABLE, so we need to check.
+			 */
+			if (((desc & DESC_MASK) == TABLE_DESC) &&
+					(level < XLAT_TABLE_LEVEL_MAX)) {
 				/*
-				 * Check if this is a table or a block. Tables
-				 * are only allowed in levels other than 3, but
-				 * DESC_PAGE has the same value as DESC_TABLE,
-				 * so we need to check.
+				 * Do not print any PA for a table descriptor,
+				 * as it doesn't directly map physical memory
+				 * but instead points to the next translation
+				 * table in the translation table walk.
 				 */
+				printf("%sVA:0x%lx size:0x%zx\n",
+				       level_spacers[level],
+				       table_idx_va, level_size);
+
+				uintptr_t addr_inner = desc & TABLE_ADDR_MASK;
 
-				if (((desc & DESC_MASK) == TABLE_DESC) &&
-				    (this_level < XLAT_TABLE_LEVEL_MAX)) {
-					uintptr_t addr_inner;
-
-					/*
-					 * Do not print any PA for a table
-					 * descriptor, as it doesn't directly
-					 * map physical memory but instead
-					 * points to the next translation
-					 * table in the translation table walk.
-					 */
-					printf("%sVA:0x%lx size:0x%zx\n",
-					       level_spacers[this_level],
-					       table_idx_va, level_size);
-
-					addr_inner = desc & TABLE_ADDR_MASK;
-					/* save current xlat level */
-					desc_tables[this_level].table_base =
-						this_base;
-					desc_tables[this_level].idx =
-						table_idx + 1;
-					desc_tables[this_level].table_idx_va =
-						table_idx_va + level_size;
-
-					/* start iterating next level entries */
-					this_base = (uint64_t *)addr_inner;
-					max_entries = XLAT_TABLE_ENTRIES;
-					this_level++;
-					level_size =
-						XLAT_BLOCK_SIZE(this_level);
-					table_idx = 0U;
-				} else {
-					printf("%sVA:0x%lx PA:0x%llx size:0x%zx ",
-					       level_spacers[this_level],
-					       table_idx_va,
-					       (uint64_t)(desc & TABLE_ADDR_MASK),
-					       level_size);
-					xlat_desc_print(ctx, desc);
-					printf("\n");
-
-					table_idx++;
-					table_idx_va += level_size;
-
-				}
+				xlat_tables_print_internal(ctx, table_idx_va,
+					(uint64_t *)addr_inner,
+					XLAT_TABLE_ENTRIES, level + 1U);
+			} else {
+				printf("%sVA:0x%lx PA:0x%llx size:0x%zx ",
+				       level_spacers[level], table_idx_va,
+				       (uint64_t)(desc & TABLE_ADDR_MASK),
+				       level_size);
+				xlat_desc_print(ctx, desc);
+				printf("\n");
 			}
 		}
+
+		table_idx++;
+		table_idx_va += level_size;
+	}
+
+	if (invalid_row_count > 1) {
+		printf(invalid_descriptors_ommited,
+		       level_spacers[level], invalid_row_count - 1);
 	}
 }