[coreboot-gerrit] Change in coreboot[master]: vendorcode/amd/pi/00670F00: Clarify CAR disable

[Marshall Dawson (Code Review)]

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

Change subject: vendorcode/amd/pi/00670F00: Clarify CAR disable
......................................................................


Patch Set 5:

(1 comment)

> Location of this CAR teardown call differs from AGESA specification.

I don't see it in the spec.  If you can point to a specific section, I can comment further...

> In case you don't know it already, as you claimed AMD_DISABLE_STACK macro would do some CAR migration:

I think it may be inaccurate to call it CAR migration.  The contents moved to DRAM from cache, but is in the identical addressable location as before.  I think of migration as meaning the globals have been specifically copied to cbmem and the global pointer getter knows to search cbmem instead of CAR the next time you use it.

> Now, since somebody placed CAR teardown _after_ AmdInitEnv(), they could not no longer use invd in AMD_DISABLE_STACK since memcpy destination buffer would be on cachelines. All of this AMD_DISABLE_STACK comment fixup is nothing but covering up that mistake instead of fixing the code to match the API.
src/vendorcode/amd/pi/00670F00/binaryPI/gcccar.inc
PS4, Line 1299:

I guess I don't get the reasoning behind these questions.  This patch is only intended to describe the current working state of the system.  It was not intended to change an API or modify any existing behavior.

https://review.coreboot.org/#/c/18492/5/src/vendorcode/amd/pi/00670F00/binaryPI/gcccar.inc
File src/vendorcode/amd/pi/00670F00/binaryPI/gcccar.inc:

Line 1298: * AMD_DISABLE_STACK:  Copy CAR data to DRAM and destroy the stack inside the
> Which part of this macro is the "copy car to dram" part?
It's the wbinvd that moves the data sitting in cache into DRAM, and not a traditional copy.  Would you prefer to suggest some different wording for this comment block?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77e53262212e00bce9145b0bc3909ad8651f2328
Gerrit-PatchSet: 5
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.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: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes