Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39484 )
Change subject: memrange: Add support for stealing required memory from given ranges ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39484/1//COMMIT_MSG@9 PS1, Line 9: This change adds support for nit: just "Add"?
https://review.coreboot.org/c/coreboot/+/39484/1/src/include/memrange.h File src/include/memrange.h:
https://review.coreboot.org/c/coreboot/+/39484/1/src/include/memrange.h@172 PS1, Line 172: Limit beyond which the stolen memory cannot grow so an upper bound for the memory range to reserve?
https://review.coreboot.org/c/coreboot/+/39484/1/src/include/memrange.h@177 PS1, Line 177: If it is not possible to fulfill the request as per given constraints, this function returns : * false. Else, it sets the base address of stolen memory in stolen_base, creates a hole of : * required size at the stolen_base and returns true to indicate success I think it's better to state the successful case first. It could also be clearer that stolen_base is an output argument, never to be read from (this statement can be interpreted to mean that a value put in there might be used to choose the base address).
How about: "If the constraints can be satified, this functin creates a hole in the memrange, writes the base address of that hole to stolen_base and returns true. Otherwise it returns false"?
https://review.coreboot.org/c/coreboot/+/39484/1/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39484/1/src/lib/memrange.c@397 PS1, Line 397: if (size == 0) return NULL; otherwise memranges_steal() with a computed size==0 leads to weird artifacts in stolen_base. While at it, maybe check that align is a power of two (ALIGN_UP does wonky stuff otherwise)?
https://review.coreboot.org/c/coreboot/+/39484/1/src/lib/memrange.c@403 PS1, Line 403: base = ALIGN_UP(r->begin, align); do you intend to use the new memrange align attribute for comparison, too? if (!IS_ALIGNED(align, r->align)) continue; or something like that?