Furquan Shaikh 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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39484/3/src/lib/memrange.c File src/lib/memrange.c:
https://review.coreboot.org/c/coreboot/+/39484/3/src/lib/memrange.c@410 PS3, Line 410: if (!IS_ALIGNED(align, ranges->align))
Why would the requested alignment have to be equal to the memranges alignment? It seems that it coul […]
Dropped this as part of https://review.coreboot.org/c/coreboot/+/39810
https://review.coreboot.org/c/coreboot/+/39484/3/src/lib/memrange.c@422 PS3, Line 422: continue;
Since the ranges are in order wouldn't this be a return NULL case? Same for the limit check.
For the limit - yes. But, I don't think that applies to r->end. We can have multiple ranges which are smaller than what the request is and so cannot be satisfied, but there could be a range later on which could potentially satisfy the request.
Example: Ranges: Begin End 0x100 0x1ff 0x300 0xfff
Request: Size: 0x300 Limit: 0xffffffff Align: 1 Tag: xyz
In this case, for first range entry, base = 0x100, end = 0x3ff. Thus, we need to check the next range entry. base = 0x300 end = 0x5ff and carve out a hole out of this.
But, I agree with limit if end has crossed limit, none of the following range entries can satisfy the request.
https://review.coreboot.org/c/coreboot/+/39484/3/src/lib/memrange.c@433 PS3, Line 433: limit
Is this inclusive or exclusive? It seems like it's inclusive given the '>' check above.
Yes, it is inclusive. I will update the comment.