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

[Marc Jones (Code Review)]

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

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


Patch Set 4:

(3 comments)

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

Line 649:     #--------------------------------------------------------------------------
> See line 419, I'd say conversion is enabled on entry.
INVD_WBINVD is not set on entry. The state is set in STACK_ENABLE, but since we explicitly call the wbinvd, not the invd, it isn't an issue.  See 355 for unset and 674 for the setting.


Line 674:     bts     $INVD_WBINVD, %eax            # Turn on Conversion of INVD to WBINVD
The exit state for INVD_WBINVD is here. This is the runtime condition.


Line 1299: *                     should only be executed on the BSP
> The essence of AMD_DISABLE_STACK is AMD_DISABLE_STACK_HOOK_F15.
I'm not sure why this discussion moved down here....
The 2008 doc is quiet old, but the 55072 Rev 3.00 Jun 8, 2016
BKDG for AMD Family 15h Models 70h-7Fh Processor states:

When BIOS has completed using the cache for general storage the following steps are followed:
1. An INVD instruction is executed on each core that used cache as general storage; an INVD is issued when
all cores on all nodes have completed using the cache for general storage.
2. If DRAM is initialized and there is data in the cache that needs to get moved to main memory, CLFLUSH
or WBINVD may be used instead of INVD, but software must ensure that needed data in main memory is
not overwritten.


We have tried to explain that this intentional and that the S3 resume path for this device will be significantly different. It doesn't seem prudent or expedient to rewrite this to just throw it away when it won't use the path. 

We also have concerns that the invd stack breakdown that you have implemented on the AGESA side will not work with the multiple verstage and romstage interactions.


-- 
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: 4
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