Attention is currently required from: Arthur Heymans. Michał Żygowski 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:
(9 comments)
File src/northbridge/amd/agesa/family14/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/803722c9_b81c643c PS3, Line 573: sizek = limitk - 768;
I think this was missing the `+ 1`. Which is implicitly fixed by making […]
Most likely
https://review.coreboot.org/c/coreboot/+/52922/comment/b126bad6_69b6d469 PS3, Line 115: [39:24] at [31:16]
BKDG says [35:24] at [27:16]. This would also be what the code masks below.
Changed the comments in the new patch.
https://review.coreboot.org/c/coreboot/+/52922/comment/91af4f57_ca379c94 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 […]
Please see if the new patch is more clear about it
https://review.coreboot.org/c/coreboot/+/52922/comment/338766a3_0ff39eca PS3, Line 120: convert to KiB by shifting 10 bits left
Please use `/ KiB` (commonlib/helpers.h) instead.
Added
https://review.coreboot.org/c/coreboot/+/52922/comment/2484a881_7896ceb0 PS3, Line 127: [39:24] at [31:16]
BKDG mentions [39:36] as 0, and the mask below only covers the other bits.
Added a full 64bit mask based on cpu_phys_address_size which should mask unused bit appropriately
File src/northbridge/amd/agesa/family15tn/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/a497f221_0500f176 PS3, Line 47: mask out DramMask [26:24] too
Any idea why the original code did this? It was basically forcing 128MiB granularity.
I have no idea. This was one of the comments which led me to think something is wrong here.
https://review.coreboot.org/c/coreboot/+/52922/comment/72ca5bda_581b7f0c PS3, Line 710: sizek = limitk - basek;
This seems off by one granularity step (128MiB). […]
It seems so.
https://review.coreboot.org/c/coreboot/+/52922/comment/60deacf2_3415948f PS3, Line 123: = *basek |
Nit, `|=`.
Done in new patch
File src/northbridge/amd/pi/00730F01/northbridge.c:
https://review.coreboot.org/c/coreboot/+/52922/comment/c4481424_30d0ec61 PS3, Line 135: }
This looks familiar. If it's the same function again, please consider to not […]
Moved to common code in new patchset