[M] Change in coreboot[main]: device_tree: Add function to get top of memory from a FDT blob

[Alper Nebi Yasak (Code Review)]

Attention is currently required from: Jakub Czapiga, Julius Werner, Maximilian Brune, Nico Huber.

Alper Nebi Yasak has posted comments on this change by Alper Nebi Yasak. ( https://review.coreboot.org/c/coreboot/+/80322?usp=email )

Change subject: device_tree: Add function to get top of memory from a FDT blob
......................................................................

Patch Set 4:

(11 comments)

Patchset:

PS3:
> Thanks for the effort and the elaborate commit message! […]

Updated the commit message, since RISC-V exception handler things were merged in the meantime.

File src/lib/device_tree.c:

https://review.coreboot.org/c/coreboot/+/80322/comment/ca52caef_d49ce5d0?usp... :
PS3, Line 181: 	if (be32_to_cpu(header->magic) != FDT_HEADER_MAGIC)
Sorry for being very late with all this, I've been going through execution dysfunction hell for a long time. I've rewritten this to use functions from CB:81081, and added an intermediary function to get memory regions like `fdt_read_reg_prop()` in that change.
> I assume that you're going to be needing more from that FDT blob in your boot model

I wasn't sure we'd need more flat-version functions, thinking we could unflatten it after initializing the memory. Also, "use device-tree info in runtime instead of static configuration at build time" feels like a paradigm shift in coreboot that should be part of larger discussion.
> [API design details]

I should've but couldn't read or reply to CB:81081, sorry. I don't like the "take offsets and return size" (vs update a given struct) API that was already there before then, so didn't really want to expand it, and was silently hoping we could replace all that coreboot-only code with libfdt. I still don't like the many-argument functions (vs more structs) and fixed-length arrays (vs iterator functions), but that's maybe because I don't have an eye for designing embedded things, I don't know.

https://review.coreboot.org/c/coreboot/+/80322/comment/510e50a3_a5eadfbc?usp... :
PS3, Line 188: 	int addr_cells = 0;
> Please use `unsigned` so we don't have to think about potential overflows.

Done, but instead I've gone with `size_t` for indices to match other functions merged in the meantime.

https://review.coreboot.org/c/coreboot/+/80322/comment/d03e73a6_bca30b77?usp... :
PS3, Line 191: 		if (!strncmp(prop.name, "#address-cells", sizeof("#address-cells")))
> Why the str*n*cmp()? Isn't "#address_cells" the whole string we should match?

It is, I remembered some "`strcmp()` is unsafe" advice (without fully understanding it apparently) and was being defensive. Replaced these parts of the code the new `fdt_find_node_by_path()`.

https://review.coreboot.org/c/coreboot/+/80322/comment/45bded9d_31bfda31?usp... :
PS3, Line 192: 			addr_cells = be32toh(*(uint32_t *)prop.data);
> read_be32() would be easier to use here. <commonlib/endian. […]

Thanks. Embarrassing, but I don't really have a good grasp of the standard library and other common functions. The newly merged fdt functions seem to use `be32dec()` instead.

https://review.coreboot.org/c/coreboot/+/80322/comment/8006212c_3f59047c?usp... :
PS3, Line 203: offset
> Aren't you hardcoding an assumption here that the `memory` node will come after the `#address-cells` […]

The two are properties, and all properties of a node appear before its first child node. But I'm letting `fdt_find_node_by_path()` handle that now.

https://review.coreboot.org/c/coreboot/+/80322/comment/93deebce_9bb0157c?usp... :
PS3, Line 205: 		if (!strncmp(name, "memory", sizeof("memory")) ||
> `strncmp(a, "literal", sizeof("literal"))` is just a more complicated way to write `strcmp(a, "liter […]

Thanks, replaced this code to use the new `fdt_find_node_by_path()`.

https://review.coreboot.org/c/coreboot/+/80322/comment/21610709_e6a1b2cf?usp... :
PS3, Line 220: 	while ((size = fdt_next_property(blob, offset, &prop))) {
> Same for properties, we should probably write an `fdt_find_prop()` helper rather than having to code […]

Done, used the new `fdt_read_reg_prop()`.

https://review.coreboot.org/c/coreboot/+/80322/comment/f9efc7c1_f81ebd31?usp... :
PS3, Line 229: 		return 0;
> Should we also check that `reg_size >= (addr_cells + size_cells) * sizeof(uint32_t)`?

Yes. Removed all this to use the new `fdt_read_reg_prop()` instead, which has `size_t count = prop.size / (4 * addr_cells + 4 * size_cells);` and reads only that much.

https://review.coreboot.org/c/coreboot/+/80322/comment/3147ddfd_5f54bebd?usp... :
PS3, Line 234: 	uintptr_t mem_size = 0;
> I would recommend using uint64_t when dealing with physical addresses and sizes. […]

Done, I updated `fdt_get_memory_top()` to work with `struct device_tree_region` (which has `u64`s) and use/return `uint64_t`.

https://review.coreboot.org/c/coreboot/+/80322/comment/50d72797_a69f4016?usp... :
PS3, Line 253: 	}
> It's not wrong to write it as a loop, but the code would become much clearer […]

There's a `fdt_read_reg_prop()` now, so I used that.