diff mbox

core: Add node-style memory reservation to device tree

Message ID 1430990797.81316.574009132670.1.gpush@pablo
State Superseded
Headers show

Commit Message

Jeremy Kerr May 7, 2015, 9:26 a.m. UTC 
Linux supports a newer memory reservation layout in the device-tree,
where each reservation is represented by a subnode under a top-level
"reserved-memory" node.

This change adds these nodes, using the mem_region names as the property
names (minus any cell addresses). The reserved-memory node looks like
this:

/ {
	name = "reserved-memory";
	ranges;
	#address-cells = <0x2>;
	#size-cells = <0x2>;

	ibm,firmware-code@30000000 {
		reg = <0x0 0x30000000 0x0 0x200000>;
	};

	ibm,firmware-data@30e00000 {
		reg = <0x0 0x30e00000 0x0 0xc00000>;
	};

	ibm,firmware-stacks@31a00000 {
		reg = <0x0 0x31a00000 0x0 0x8000000>;
	};

	ibm,firmware-allocs-memory@39a00000 {
		reg = <0x0 0x39a00000 0x0 0x1c0200>;
	};

	ibm,firmware-heap@30200000 {
		reg = <0x0 0x30200000 0x0 0xc00000>;
	};
};

We keep the property-style reservation information (reserved-names and
reserved-ranges) unchanged.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 core/mem_region.c                       |   27 ++++++
 core/test/run-mem_region_reservations.c |  108 +++++++++++++++++-------
 doc/device-tree/reserved-memory.txt     |   26 +++++
 3 files changed, 131 insertions(+), 30 deletions(-)

Comments

Stewart Smith May 13, 2015, 4:12 a.m. UTC | #1 
Jeremy Kerr <jk@ozlabs.org> writes:
> Linux supports a newer memory reservation layout in the device-tree,
> where each reservation is represented by a subnode under a top-level
> "reserved-memory" node.
>
> This change adds these nodes, using the mem_region names as the property
> names (minus any cell addresses). The reserved-memory node looks like
> this:
>
> / {
> 	name = "reserved-memory";
> 	ranges;
> 	#address-cells = <0x2>;
> 	#size-cells = <0x2>;
>
> 	ibm,firmware-code@30000000 {
> 		reg = <0x0 0x30000000 0x0 0x200000>;
> 	};

With this patch I get this with my mambo tests (have zImage.epapr in
~/skiboot and run make check). It seems as though we're getting some
logs about the reserved memory?

14871833: (14871833): [14868998,5] INIT: Loading kernel
14875859: (14875859): [14872142,5] Assuming kernel at 0x20000000
14881057: (14881057): [14876086,5] INIT: Kernel loaded, size: 0 bytes (0 = unkno
wn preload)
14885549: (14885549): [14881346,5] INIT: 32-bit kernel entry at 0x2001015c
14903544: (14903544): [14886769,3] OCC: No HOMER detected, assuming no pstates
14907601: (14907601): [14903797,3] ELOG: Error getting buffer to log error
19603894: (19603894): [19597505,5] INIT: Starting kernel at 0x2001015c, fdt at 0
x30203a78 (size 0x13c8)
382722408: (382722405): [382718055,3] OPAL: Trying a CPU re-init with flags: 0x1
382731228: (382731225): [382723646,3] SLW: Not found on chip 0
382735345: (382735342): [382731481,3] ELOG: Error getting buffer to log error
387826830: (387826825): [    0.000000] OPAL V3 detected !
387871856: (387871851): [    0.000000] Reserved memory: failed to reserve memory
 for node 'ibm,firmware-stacks@31a00000': base 0x0000000031a00000, size 0 MiB
388038641: (388038636): [    0.000000] Reserved memory: failed to reserve memory
 for node 'ibm,firmware-data@30e00000': base 0x0000000030e00000, size 12 MiB
388204211: (388204206): [    0.000000] Reserved memory: failed to reserve memory
 for node 'ibm,firmware-heap@30200000': base 0x0000000030200000, size 12 MiB
388369776: (388369771): [    0.000000] Reserved memory: failed to reserve memory
 for node 'ibm,firmware-code@30000000': base 0x0000000030000000, size 2 MiB
388534158: (388534153): [    0.000000] Reserved memory: failed to reserve memory
 for node 'ibm,firmware-allocs-memory@39a00000': base 0x0000000039a00000, size 1
 MiB

Are ^ the result of trying to reserve it twice?

388711270: (388711265): [    0.000000] Using PowerNV machine description
388778417: (388778412): [    0.000000] bootconsole [udbg0] enabled
388839611: (388839606): [    0.000000] CPU maps initialized for 1 thread per cor
e
388924402: (388924397):  -> smp_release_cpus()
388954297: (388954292): spinning_secondaries = 0
388986268: (388986263):  <- smp_release_cpus()
389019235: (389019230): [    0.000000] Starting Linux PPC64 #2 SMP Fri Jan 23 15
:52:55 AEDT 2015
389113000: (389112995): [    0.000000] -----------------------------------------
------------
389202007: (389202002): [    0.000000] ppc64_pft_size                = 0x0
389269246: (389269241): [    0.000000] physicalMemorySize            = 0x4000000
0
389345236: (389345231): [    0.000000] htab_address                  = 0xc000000
03f000000
389430689: (389430684): [    0.000000] htab_hash_mask                = 0x1ffff
389502540: (389502535): [    0.000000] -----------------------------------------
------------
389588423: (389588418):  <- setup_system()
389617776: (389617771): [    0.000000] Linux version 3.17.2 (stewart@ka1) (gcc v
ersion 4.8.3 (Buildroot 2014.08-git-g1455c21) ) #2 SMP Fri Jan 23 15:52:55 AEDT 
2015
Jeremy Kerr May 14, 2015, 3:15 a.m. UTC | #2 
Hi Stewart,

> 387871856: (387871851): [    0.000000] Reserved memory: failed to reserve memory
>  for node 'ibm,firmware-stacks@31a00000': base 0x0000000031a00000, size 0 MiB
> 388038641: (388038636): [    0.000000] Reserved memory: failed to reserve memory
>  for node 'ibm,firmware-data@30e00000': base 0x0000000030e00000, size 12 MiB
> 388204211: (388204206): [    0.000000] Reserved memory: failed to reserve memory
>  for node 'ibm,firmware-heap@30200000': base 0x0000000030200000, size 12 MiB
> 388369776: (388369771): [    0.000000] Reserved memory: failed to reserve memory
>  for node 'ibm,firmware-code@30000000': base 0x0000000030000000, size 2 MiB
> 388534158: (388534153): [    0.000000] Reserved memory: failed to reserve memory
>  for node 'ibm,firmware-allocs-memory@39a00000': base 0x0000000039a00000, size 1
>  MiB
> 
> Are ^ the result of trying to reserve it twice?

Yes - the change I've posted adds the node-style reservations alongside
the property-style reservations. Since current kernels support both,
we'll get those messages reported.

Ben - if we've supported the node-style reservations in Linux for long
enough, perhaps we should remove the property-style ones in OPAL?

Regards,


Jeremy
Stewart Smith May 14, 2015, 3:39 a.m. UTC | #3 
Jeremy Kerr <jk@ozlabs.org> writes:

> Hi Stewart,
>
>> 387871856: (387871851): [    0.000000] Reserved memory: failed to reserve memory
>>  for node 'ibm,firmware-stacks@31a00000': base 0x0000000031a00000, size 0 MiB
>> 388038641: (388038636): [    0.000000] Reserved memory: failed to reserve memory
>>  for node 'ibm,firmware-data@30e00000': base 0x0000000030e00000, size 12 MiB
>> 388204211: (388204206): [    0.000000] Reserved memory: failed to reserve memory
>>  for node 'ibm,firmware-heap@30200000': base 0x0000000030200000, size 12 MiB
>> 388369776: (388369771): [    0.000000] Reserved memory: failed to reserve memory
>>  for node 'ibm,firmware-code@30000000': base 0x0000000030000000, size 2 MiB
>> 388534158: (388534153): [    0.000000] Reserved memory: failed to reserve memory
>>  for node 'ibm,firmware-allocs-memory@39a00000': base 0x0000000039a00000, size 1
>>  MiB
>> 
>> Are ^ the result of trying to reserve it twice?
>
> Yes - the change I've posted adds the node-style reservations alongside
> the property-style reservations. Since current kernels support both,
> we'll get those messages reported.
>
> Ben - if we've supported the node-style reservations in Linux for long
> enough, perhaps we should remove the property-style ones in OPAL?

We should probably go back and pick a "this is the earliest upstream"
and other such variants (such as PowerKVM 2.1) and assemble that
somewhere.
Jeremy Kerr May 14, 2015, 3:42 a.m. UTC | #4 
Hi all,

> Yes - the change I've posted adds the node-style reservations alongside
> the property-style reservations. Since current kernels support both,
> we'll get those messages reported.

In fact, these are conflicting with the even-older /memreserve/ entries
in the fdt header!

Perhaps we'd be best with a series of checks in Linux, where we detect
(and use) the latest style of reservation (reserved-memory -->
reserved-ranges --> /memreserve/), and ignore the rest.

Regards,


Jeremy
diff mbox

Patch

diff --git a/core/mem_region.c b/core/mem_region.c
index 3cc8ae0..56601e0 100644
--- a/core/mem_region.c
+++ b/core/mem_region.c
@@ -918,11 +918,30 @@  void mem_region_release_unused(void)
 	unlock(&mem_region_lock);
 }
 
+static void mem_region_add_dt_reserved_node(struct dt_node *parent,
+		struct mem_region *region)
+{
+	struct dt_node *node;
+	char *name, *p;
+
+	name = strdup(region->name);
+
+	/* remove any cell addresses in the region name; we have our own cell
+	 * addresses here */
+	p = strchr(name, '@');
+	if (p)
+		*p = '\0';
+
+	node = dt_new_addr(parent, name, region->start);
+	dt_add_property_u64s(node, "reg", region->start, region->len);
+}
+
 void mem_region_add_dt_reserved(void)
 {
 	int names_len, ranges_len, len;
 	struct mem_region *region;
 	void *names, *ranges;
+	struct dt_node *node;
 	uint64_t *range;
 	char *name;
 
@@ -936,6 +955,12 @@  void mem_region_add_dt_reserved(void)
 	mem_regions_finalised = true;
 	unlock(&mem_region_lock);
 
+	/* establish top-level reservation node */
+	node = dt_new(dt_root, "reserved-memory");
+	dt_add_property_cells(node, "#address-cells", 2);
+	dt_add_property_cells(node, "#size-cells", 2);
+	dt_add_property(node, "ranges", NULL, 0);
+
 	/* First pass: calculate length of property data */
 	list_for_each(&regions, region, list) {
 		if (!region_is_reserved(region))
@@ -961,6 +986,8 @@  void mem_region_add_dt_reserved(void)
 		       (long long)(region->start + region->len - 1),
 		       region->name);
 
+		mem_region_add_dt_reserved_node(node, region);
+
 		range[0] = cpu_to_fdt64(region->start);
 		range[1] = cpu_to_fdt64(region->len);
 		range += 2;
diff --git a/core/test/run-mem_region_reservations.c b/core/test/run-mem_region_reservations.c
index b33827f..8be3e67 100644
--- a/core/test/run-mem_region_reservations.c
+++ b/core/test/run-mem_region_reservations.c
@@ -108,13 +108,83 @@  static struct {
 	{ "test.3", 0x4000, false },
 };
 
-int main(void)
+static void check_property_reservations(void)
 {
 	const struct dt_property *names, *ranges;
-	struct mem_region *r;
-	unsigned int i, l, c;
-	uint64_t *rangep;
+	unsigned int i, l;
 	const char *name;
+	uint64_t *rangep;
+
+	/* check dt properties */
+	names = dt_find_property(dt_root, "reserved-names");
+	ranges = dt_find_property(dt_root, "reserved-ranges");
+
+	assert(names && ranges);
+
+	/* walk through names & ranges properies, ensuring that the test
+	 * regions are all present */
+	for (name = names->prop, rangep = (uint64_t *)ranges->prop;
+			name < names->prop + names->len;
+			name += l, rangep += 2) {
+		uint64_t addr;
+
+		addr = dt_get_number(rangep, 2);
+		l = strlen(name) + 1;
+
+		for (i = 0; i < ARRAY_SIZE(test_regions); i++) {
+			if (strcmp(test_regions[i].name, name))
+				continue;
+			assert(test_regions[i].addr == addr);
+			assert(!test_regions[i].found);
+			test_regions[i].found = true;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(test_regions); i++) {
+		assert(test_regions[i].found);
+		test_regions[i].found = false;
+	}
+}
+
+static void check_node_reservations(void)
+{
+	struct dt_node *parent, *node;
+	unsigned int i;
+
+	parent = dt_find_by_name(dt_root, "reserved-memory");
+	assert(parent);
+
+	assert(dt_prop_get_cell(parent, "#address-cells", 0) == 2);
+	assert(dt_prop_get_cell(parent, "#size-cells", 0) == 2);
+	dt_require_property(parent, "ranges", 0);
+
+	dt_for_each_child(parent, node) {
+		uint64_t addr, size;
+
+		addr = dt_get_address(node, 0, &size);
+
+		for (i = 0; i < ARRAY_SIZE(test_regions); i++) {
+			if (strncmp(test_regions[i].name, node->name,
+						strlen(test_regions[i].name)))
+				continue;
+
+			assert(!test_regions[i].found);
+			assert(test_regions[i].addr == addr);
+			assert(size = 0x1000);
+			test_regions[i].found = true;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(test_regions); i++) {
+		assert(test_regions[i].found);
+		test_regions[i].found = false;
+	}
+}
+
+int main(void)
+{
+	struct mem_region *r;
+	unsigned int i;
 	void *buf;
 
 	/* Use malloc for the heap, so valgrind can find issues. */
@@ -146,33 +216,11 @@  int main(void)
 	r = new_region("test.4", 0x5000, 0x1000, NULL, REGION_RESERVED);
 	assert(!add_region(r));
 
-	/* check dt properties */
-	names = dt_find_property(dt_root, "reserved-names");
-	ranges = dt_find_property(dt_root, "reserved-ranges");
-
-	assert(names && ranges);
-
-	/* walk through names & ranges properies, ensuring that the test
-	 * regions are all present */
-	for (name = names->prop, rangep = (uint64_t *)ranges->prop, c = 0;
-			name < names->prop + names->len;
-			name += l, rangep += 2) {
-		uint64_t addr;
-
-		addr = dt_get_number(rangep, 2);
-		l = strlen(name) + 1;
-
-		for (i = 0; i < ARRAY_SIZE(test_regions); i++) {
-			if (strcmp(test_regions[i].name, name))
-				continue;
-			assert(test_regions[i].addr == addr);
-			assert(!test_regions[i].found);
-			test_regions[i].found = true;
-			c++;
-		}
-	}
+	/* check old property-style reservations */
+	check_property_reservations();
 
-	assert(c == ARRAY_SIZE(test_regions));
+	/* and new node-style reservations */
+	check_node_reservations();
 
 	dt_free(dt_root);
 	free((void *)(long)skiboot_heap.start);
diff --git a/doc/device-tree/reserved-memory.txt b/doc/device-tree/reserved-memory.txt
new file mode 100644
index 0000000..48e32d4
--- /dev/null
+++ b/doc/device-tree/reserved-memory.txt
@@ -0,0 +1,26 @@ 
+reserved-memory device tree nodes
+
+OPAL exposes reserved memory through a top-level reserved-memory node,
+containing subnodes that represent each reserved memory region.
+
+This follows the Linux specification for the /reserved-memory node,
+described in the kernel source tree, in:
+
+  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+
+The top-level /reserved-memory node contains:
+
+  #size-cells = <2>
+  #address-cells = <2>
+   - addresses and sizes are all 64-bits
+
+  ranges;
+   - the empty ranges node indicates no translation of physical
+     addresses in the subnodes.
+
+The sub-nodes under the /reserved-memory node contain:
+
+ reg = <address size>
+  - the address and size of the reserved memory region. The address
+    and size values are two cells each, as sig
+