[coreboot-gerrit] Change in coreboot[master]: binaryPI: Fix UMA calculations

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19346 )

Change subject: binaryPI: Fix UMA calculations
......................................................................


Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/19346/2/src/northbridge/amd/pi/agesawrapper.c
File src/northbridge/amd/pi/agesawrapper.c:

Line 152: 	backup_top_of_ram(PostParams->MemConfig.Sub4GCacheTop);
> It sounds like any solution is specific to the part. Regarding the MTRRs it
> If the UMA memory cannot be physically accessed from the cpu cores it shouldn't matter too much w.r.t. MTRRs as those transactions should be terminated as failing or throw machine checks. If, however, the architecture allows cpu cores to access the region as well as a graphics agents with different cache'ing properties incoherence will occur (or unexpected thrashing through memory subsystem).

The memory is accessible from the x86.  With both coreboot and UEFI, the UMA region is WB and behaves like addressable DRAM.  See mention above regarding cache coherency.

> Is this value documented in any way for each of the various parts?

Unless something's changed, it's probably only documented internally.  There may also be interesting hotchips presentations showing the higher level details.  I'll see if I can find anything.


Line 152: 	backup_top_of_ram(PostParams->MemConfig.Sub4GCacheTop);
> For f16kb, Sub4GCacheTop already had UMA substracted if it was enabled.
> Test for UmaBase!=0, min(UmaBase, Sub4GCacheTop) instead might be required then for Stoney?

That's exactly what I was thinking too.  I suspect that approach would likely work for all devices.

> I am not exactly sure why UMA region would even have to be UC like MullinsPI vendorcode does it. In CarrizoPI source, UmaMemTyping is not called anymore?

I about CZ, same as ST.  I don't have a good answer but I'm a little concerned about the inconsistency.  As late as KV, I see
 MemNSetMTRRUmaRegionUCNb (NBPtr, &UmaAbove4GBase, &TOM2);

I have gone fuzzy on the incremental design changes in the northbridges.  AMD was working toward having the GPU be 100% cache-coherent with the x86 for optimal HSA performance, and I wonder if the change is related to that.  I thought Kaveri is when that was first available though.  (It's possible AGESA didn't configure UMA optimally for KV.)

I'm seeing the frame buffer BAR set in MMIO space, BTW.  I suspect the recommendation would be against WC to ensure predictable behavior of the devices.


-- 
To view, visit https://review.coreboot.org/19346
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c375e5da0dfef6cef0c50272356cd32a87b1ff6
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes