Attention is currently required from: Julius Werner. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56543 )
Change subject: helpers: Add GENMASK and GENMASK_ULL macros ......................................................................
Patch Set 5:
(3 comments)
File src/commonlib/bsd/include/commonlib/bsd/helpers.h:
https://review.coreboot.org/c/coreboot/+/56543/comment/c87cf84f_5bd29f44 PS4, Line 59: * Separate the case of high >= 63 to avoid shift-count-overflow error.
I don't understand the `high` >= 63... […]
I just mean "high == 63". For high < 63,
POWER_OF_2((high) + 1) - POWER_OF_2(low))
has fewer operations than
((~0ULL >> (31 - (high))) & ~((1ULL << (low)) - 1))
so the former is used. However, the former doesn't support high == 63 (due to shift-count-overflow error when POWER_OF_2(64)), so I separate the 2 cases.
Anyway, linux's macro is even better than both versions.
https://review.coreboot.org/c/coreboot/+/56543/comment/34e1dc85_12b3e151 PS4, Line 61: #define BIT_MASK(high, low) \
Don't we just want to call this GENMASK? While I don't think that name is great, Linux has already e […]
Thanks, I didn't know there's a GENMASK in linux.
File tests/commonlib/bsd/helpers-test.c:
https://review.coreboot.org/c/coreboot/+/56543/comment/476dd028_0af81713 PS4, Line 13: 4, 5
For invalid cases like this, I'd also prefer an assert() (unless you have practical examples why thi […]
Adding an assertion seems to make the macro unnecessarily complicated. There's no assertion in linux's macro either.
This "invalid" case is now removed here. What do you think?
-
Yu-Ping Wu (Code Review)