[PATCH v2] lib/utils/fdt: Reserve exactly enough FDT space for fdt_cpu_fixup

[PATCH v2] lib/utils/fdt: Reserve exactly enough FDT space for fdt_cpu_fixup

[Chen Pei]

fdt_cpu_fixup() reserves a hardcoded 32 extra bytes, plus 16 more per
HART inside the "zicntr" loop. Those 32 bytes also have to cover every
"status" property rewritten to "disabled", so once more than a handful
of HARTs are disabled the fixups fail with FDT_ERR_NOSPACE (-3).

Bumping the reservation to a fixed worst case is not safe either:
fdt_open_into() cannot know how much space the caller's buffer really
has, it just trusts the requested size. As OpenSBI does not own the
memory following a device tree passed in by the previous booting stage,
over-declaring the blob risks corrupting whatever follows it.

Walk the HART nodes once without modifying them instead, and reserve
exactly what the fixups need. The read-only decision logic is factored
into fdt_cpu_fixup_needed() so the sizing and fixup passes cannot drift
apart, which also makes the per-HART fdt_open_into() redundant. Check
the return values of fdt_setprop_string() and fdt_appendprop_string()
as well, as running out of space used to fail silently.

Fixes: dd9439fbace2 ("lib: utils: Add a fdt_cpu_fixup() helper")
Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
Changes in v2:
- Scan the HART nodes and reserve only what the fixups need, instead of a
  fixed SBI_HARTMASK_MAX_BITS * 20 bytes: fdt_open_into() just trusts the
  requested size, so over-declaring a device tree owned by the previous
  booting stage may corrupt what follows it.
- Fix the per-HART estimate; a missing "status" counts as enabled, so the
  property may have to be created rather than overwritten.
- Add the read-only helper fdt_cpu_fixup_needed() so the sizing and fixup
  passes cannot drift apart.
- Check the fdt_setprop_string()/fdt_appendprop_string() return values.
- Re-resolve /cpus after fdt_open_into() and skip it when there is nothing
  to fix.
- Drop the dependency on SBI_HARTMASK_MAX_BITS.

 lib/utils/fdt/fdt_fixup.c | 133 +++++++++++++++++++++++++++-----------
 1 file changed, 95 insertions(+), 38 deletions(-)

diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
index b0ed20c8..23899c0e 100644
--- a/lib/utils/fdt/fdt_fixup.c
+++ b/lib/utils/fdt/fdt_fixup.c
@@ -106,14 +106,65 @@ int fdt_add_cpu_idle_states(void *fdt, const struct sbi_cpu_idle_state *state)
 	return 0;
 }
 
-void fdt_cpu_fixup(void *fdt)
+/**
+ * Report which fixups a HART DT node needs. Read-only, so the sizing pass
+ * and the fixup pass below stay in sync by construction.
+ */
+static void fdt_cpu_fixup_needed(const void *fdt, int cpu_offset,
+				 bool emulated_zicntr, bool *disable,
+				 bool *add_zicntr)
 {
-	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
 	struct sbi_domain *dom = sbi_domain_thishart_ptr();
-	int err, cpu_offset, cpus_offset, len;
 	const char *mmu_type, *extensions;
 	u32 hartid, hartindex;
-	bool emulated_zicntr;
+	int len;
+
+	*disable = false;
+	*add_zicntr = false;
+
+	if (fdt_parse_hart_id(fdt, cpu_offset, &hartid))
+		return;
+
+	if (!fdt_node_is_enabled(fdt, cpu_offset))
+		return;
+
+	/*
+	 * Disable a HART DT node if one of the following is true:
+	 * 1. The HART is not assigned to the current domain
+	 * 2. MMU is not available for the HART
+	 */
+
+	hartindex = sbi_hartid_to_hartindex(hartid);
+	mmu_type = fdt_getprop(fdt, cpu_offset, "mmu-type", &len);
+	if (!sbi_domain_is_assigned_hart(dom, hartindex) || !mmu_type || !len)
+		*disable = true;
+
+	if (!emulated_zicntr)
+		return;
+
+	extensions = fdt_getprop(fdt, cpu_offset, "riscv,isa-extensions", &len);
+	/*
+	 * For legacy devicetrees, don't create riscv,isa-extensions
+	 * property if there hasn't been already one.
+	 */
+	if (extensions && !fdt_stringlist_contains(extensions, len, "zicntr"))
+		*add_zicntr = true;
+}
+
+void fdt_cpu_fixup(void *fdt)
+{
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+	bool emulated_zicntr, disable, add_zicntr;
+	int err, cpu_offset, cpus_offset;
+	int reserve = 0;
+	/*
+	 * A new "status" property costs a header, its tag-aligned value and its
+	 * name; appending "zicntr" only grows an existing tag-aligned value.
+	 */
+	const int status_size = sizeof(struct fdt_property) +
+				ROUNDUP(sizeof("disabled"), FDT_TAGSIZE) +
+				sizeof("status");
+	const int zicntr_size = ROUNDUP(sizeof("zicntr"), FDT_TAGSIZE);
 
 	/*
 	 * Claim Zicntr extension in riscv,isa-extensions if
@@ -124,52 +175,58 @@ void fdt_cpu_fixup(void *fdt)
 			  sbi_hart_has_csr(scratch, SBI_HART_CSR_CYCLE) &&
 			  sbi_hart_has_csr(scratch, SBI_HART_CSR_INSTRET);
 
-	err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
-	if (err < 0)
-		return;
-
 	cpus_offset = fdt_path_offset(fdt, "/cpus");
 	if (cpus_offset < 0)
 		return;
 
+	/*
+	 * fdt_open_into() trusts the size it is given, so reserve only what is
+	 * really needed instead of a fixed worst case for every possible HART.
+	 */
 	fdt_for_each_subnode(cpu_offset, fdt, cpus_offset) {
-		err = fdt_parse_hart_id(fdt, cpu_offset, &hartid);
-		if (err)
-			continue;
+		fdt_cpu_fixup_needed(fdt, cpu_offset, emulated_zicntr,
+				     &disable, &add_zicntr);
+		if (disable)
+			reserve += status_size;
+		if (add_zicntr)
+			reserve += zicntr_size;
+	}
 
-		if (!fdt_node_is_enabled(fdt, cpu_offset))
-			continue;
+	if (!reserve)
+		return;
 
-		/*
-		 * Disable a HART DT node if one of the following is true:
-		 * 1. The HART is not assigned to the current domain
-		 * 2. MMU is not available for the HART
-		 */
+	err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + reserve);
+	if (err < 0)
+		return;
 
-		hartindex = sbi_hartid_to_hartindex(hartid);
-		mmu_type = fdt_getprop(fdt, cpu_offset, "mmu-type", &len);
-		if (!sbi_domain_is_assigned_hart(dom, hartindex) ||
-		    !mmu_type || !len)
-			fdt_setprop_string(fdt, cpu_offset, "status",
-					   "disabled");
+	/* fdt_open_into() may have reordered the blocks, so look up again. */
+	cpus_offset = fdt_path_offset(fdt, "/cpus");
+	if (cpus_offset < 0)
+		return;
 
-		if (!emulated_zicntr)
-			continue;
+	fdt_for_each_subnode(cpu_offset, fdt, cpus_offset) {
+		fdt_cpu_fixup_needed(fdt, cpu_offset, emulated_zicntr,
+				     &disable, &add_zicntr);
 
-		extensions = fdt_getprop(fdt, cpu_offset,
-					 "riscv,isa-extensions", &len);
-		/*
-		 * For legacy devicetrees, don't create riscv,isa-extensions
-		 * property if there hasn't been already one.
-		 */
-		if (extensions &&
-		    !fdt_stringlist_contains(extensions, len, "zicntr")) {
-			err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 16);
+		if (disable) {
+			err = fdt_setprop_string(fdt, cpu_offset, "status",
+						 "disabled");
 			if (err)
-				continue;
+				sbi_printf("%s: failed to disable %s (%d)\n",
+					   __func__,
+					   fdt_get_name(fdt, cpu_offset, NULL),
+					   err);
+		}
 
-			fdt_appendprop_string(fdt, cpu_offset,
-					      "riscv,isa-extensions", "zicntr");
+		if (add_zicntr) {
+			err = fdt_appendprop_string(fdt, cpu_offset,
+						    "riscv,isa-extensions",
+						    "zicntr");
+			if (err)
+				sbi_printf("%s: failed to add zicntr to %s (%d)\n",
+					   __func__,
+					   fdt_get_name(fdt, cpu_offset, NULL),
+					   err);
 		}
 	}
 }
-- 
2.50.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi