Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 2:
(3 comments)
What are the final MTRR settings with this? 2G is often a nice setting as it cleanly splits the 4G region in two, requiring only 1 MTRR to cover the bottom or top region.
I've done three Lenovo G505S builds: for 0x40, 0x80 and 0xc0 BottomIo values. From https://github.com/mikebdp2/bottomio_research files we could see that MTRR fixed ranges are the same (00000-9FFFF write-back, A0000-BFFFF uncachable, C0000-FFFFF write-back), while the MTRR variable ranges are:
Thanks for the investigation. At least everything that should be fully cached seems fine. I wonder, however, where the MTRR allo- cation is done with AGESA. It seems, it doesn't take prefetchable MMIO resources into account... let's just hope, Linux can cover the mess up.
Nico told me that a too low BottomIo setting could result in a performance hit:
You should check MTRR allocations before/after the change. If it runs out of registers, that can be bad for performance (depending on whether the CPU supports PAT).
My major concern was about prefetchable resources, but that seems to be ignored by these AGESA hook-ups anyway?
Aside from
mtrr: your CPUs had inconsistent variable MTRR settings mtrr: probably your BIOS does not setup all CPUs. mtrr: corrected configuration.
This is just more trouble in coreboot. I've been looking at the code (just a little) and couldn't find anything that would write the AP's variable MTRRs. Seems weird. But the whole mess reminds me that integrating vendorcode into coreboot, no matter if blob or open source, is just wrong.
So, although I'm fine with changing a BottomIo position from 0x40000000 to 0x80000000, I'd be more happy to do this if there's a good reason: i.e. if still a real danger of performance hit.
With the Kconfig option, we only have to discuss a default. People can choose whatever they want. I would stick to the old default though, because that seems most tested. And let's not forget about people that might just want to run a 32-bit OS (big hole would remap more RAM outside of 32-bit space).
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... PS2, Line 25: default 0x40000000 I would prefer to leave it at the current default and only override it on tested platforms.
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: (UINT16) Why the explicit cast?
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) Should we mask it with `& 0xf8` to enforce the 128MiB alignment?
Also, how about some `MIN(0xe0, MAX(0x28, ...))` to make sure it doesn't brick because of misconfiguration?