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

(3 comments)

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 406: *   coreboot CAR globals. This should NOT be used with S3 resume IF the
> 1. that corrupts that memory in s3 case.
1) Agree.  That area isn't marked as reserved, of course.  Not sure how S3 will be handled for Stoney.  The PSP is going to have DRAM turned back on for us before allowing x86 to execute.

> 2. But even if you didn't overwrite the entire memory area car_migrated would be corrupting things as well.  Or you are relying on random memory to be non-zero for car_migrated?

If you mean S5-S0 (or reboot), I'm not quite getting the question as it relates to corrupting anything.  The WBINVD will leave artifacts sitting in DRAM at the end of POST, if that's what you mean.

> Or are you doing the backup dance to avoid the corruptions?

I think I see what you're saying.  If not, it's something else we'll need to consider...  If we INVD the region instead of using it for memory backing, it's going to respond with 0 when you read car_migrated, appearing unmigrated.  Or, you corrupt memory if you jam it with ~0.  Looks like Intel systems avoid this by putting CAR at FEF00000 (or something similar).


Line 406: *   coreboot CAR globals. This should NOT be used with S3 resume IF the
> OK... so scrubbed AGESA and maybe also pi/00630f00 have different behaviour
OK.  And I'll re-scrub the file for mentions of destroying the stack, and maybe clarify that the stack remains in place but CAR is torn down.


Line 1298: * AMD_DISABLE_STACK:  Copy CAR data to DRAM and destroy the stack inside the
> This particular comment still says "destroy the stack inside the cache. But
> This particular comment still says "destroy the stack inside the cache. But it does not, as it eventually calls wbinvd.

I'm not at all opposed to modifying the comments.  It seemed better to get agreement first instead of pushing multiple attempts.  You had proposed 'flush cachelines' or 'invalidate with write-back' before.

> I just feel that it becomes very much incorrect to call this macro "AMD_DISABLE_STACK"

I've been having that same feeling, and have been thinking of changing the name until things can be redone.


-- 
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: Zheng Bao <fishbaozi at gmail.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes