Attention is currently required from: Michał Żygowski, Arthur Heymans. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52922 )
Change subject: nb/amd/{agesa,pi}: Avoid overflows during DRAM calculation ......................................................................
Patch Set 4:
(14 comments)
Patchset:
PS4: Thanks for splitting the patch. It's still a bit big, though. Took me almost 3 hours to read through the first two platforms (fam14 and fam15tn). The new functions look similar enough to extract common code. What generally works well to get a break during review is to add common code in one commit, and then update the individual platforms one-by-one in separate commits.
The code changes look sane, alas the comments and the commit message don't. Without kowing further details, it seems possible that the change of the granularity for the newer platforms results in further problems. And this change is not even mentioned in the commit message.
IMHO, a simple comment like
/* The upper half of each register provides the DRAM base/limit in 16MiB steps. */
could replace all the error-prone and redundant (considering that the documentation is public) comments about bit offsets.
I did not see any overflowing shift operations. Though, it looks like the original code treated the limit as exclusive, while it wasn't.
File src/northbridge/amd/agesa/family14/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/4260891c_854d804c PS3, Line 553: limitk = (resource_t) (((u64) d.mask << 8) | 0xFFFFFF); I don't see any overflow here. Can you provide before/after logs what changed here effectively?
https://review.coreboot.org/c/coreboot/+/52922/comment/55dbadbc_98a37bf1 PS3, Line 573: sizek = limitk - 768; I think this was missing the `+ 1`. Which is implicitly fixed by making `limitk` exclusive.
https://review.coreboot.org/c/coreboot/+/52922/comment/90ad0e13_ab396588 PS3, Line 581: limitk > Same off-by-one here, I assume.
https://review.coreboot.org/c/coreboot/+/52922/comment/58c7e7f7_b01727be PS3, Line 115: [39:24] at [31:16] BKDG says [35:24] at [27:16]. This would also be what the code masks below.
https://review.coreboot.org/c/coreboot/+/52922/comment/b66772e9_708f5cc5 PS3, Line 119: {DramBase[35:24], 00_0000h} <= address[35:0] How to calculate an 8 from this? I'm guessing by taking the masked bits into account. But if I have to look at the code to derive it, what's the comment worth?
Actually the comment above ("[39:24] at [31:16]") seems to explain it better. Well, it would, if the code wouldn't conflict that comment.
https://review.coreboot.org/c/coreboot/+/52922/comment/c9645fc7_61012e42 PS3, Line 120: convert to KiB by shifting 10 bits left Please use `/ KiB` (commonlib/helpers.h) instead.
https://review.coreboot.org/c/coreboot/+/52922/comment/ec6da4ba_a2f9f67a PS3, Line 127: [39:24] at [31:16] BKDG mentions [39:36] as 0, and the mask below only covers the other bits.
https://review.coreboot.org/c/coreboot/+/52922/comment/7cec83bb_1253d2a8 PS3, Line 129: byte kilobyte?
https://review.coreboot.org/c/coreboot/+/52922/comment/38767fb8_1238ad07 PS3, Line 129: *limitk += 1; // round up last byte Nit, if this was `+= 16*MiB/KiB`, no or-mask would be needed above.
Mentioning the 16MiB granularity at the start of the function would probably make things easier to follow.
File src/northbridge/amd/agesa/family15tn/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/064b212b_0e1e1176 PS3, Line 47: mask out DramMask [26:24] too Any idea why the original code did this? It was basically forcing 128MiB granularity.
https://review.coreboot.org/c/coreboot/+/52922/comment/4140edf2_d8402fe9 PS3, Line 710: sizek = limitk - basek; This seems off by one granularity step (128MiB). Probably the actual problem?
https://review.coreboot.org/c/coreboot/+/52922/comment/e7882fd9_8ae57016 PS3, Line 123: = *basek | Nit, `|=`.
File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/8b2532f0_3795c977 PS3, Line 135: } This looks familiar. If it's the same function again, please consider to not add more redundant code. `common/` directories are easily added.
-
Nico Huber (Code Review)