Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34991 )
Change subject: arch/x86: Include stack_top region into postcar_frame ......................................................................
Patch Set 4:
Patch Set 4: Code-Review-2
I don't understand this commit, more descriptive commit message perhaps required?
First, this is titled arch/x86, althoug it will only make a difference on intel/apollolake where empty weak platform_segment_loaded() has an override.
it will matter for APL because it implements platform_segment_loaded() to flush_l1d_to_l2() hence it might miss a valid range where actually the data been stored (while pushing into stack). Like its been letting platform_segment_loaded() know about stack_top_ptr, it should also info about stack current pointer and size of data been written into.
Second, in the state master branch currently is, while struct postcar_frame within CAR, the created stack is entirely in CBMEM? So intel/apollolake platform_segment_loaded() is a no-op?
why u said platform_segment_loaded() is no-op?
This commit is in my opinion somehow tied to POSTCAR_STAGE=n developments and does not have to be merged until we have decisions about them. Let me know if I misunderstood something. If I see relevant +2 I will change my vote to -1.
No its not, as i have explained above about why this stack region also need to inform, irrespective of its bene postcar_stage=y or n. i guess we are lucky that APL didn't complain but looks like we are ignoring a range where actual data been stored.
@Aaron, do you think this is more specific to APL as you have implemented platform_segment_loaded() in apl/car.c ?