Attention is currently required from: Jason Glenesk, Anjaneya "Reddy" Chagam, Raul Rangel, Marshall Dawson, Jonathan Zhang, Johnny Lin, Arthur Heymans, Morgan Jang, Kyösti Mälkki, Patrick Rudolph, Felix Held. Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54301 )
Change subject: [WIP]arch/x86: Tear down CAR in ramstage ......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54301/comment/f94721b2_d9098d0c PS2, Line 15: likely to be slow. Yes, and among many other things. Caching teardown in general will have the same problem as all the old ones -- just this time they are sitting in ramstage.
Just so the background is captured. We used to teardown cache as ram in romstage then return back into C code to load ramstage. Variable juggling is hard there because much of the code is expecting state to be maintained. That's why we added the relocatable car variable stuff; it was complicated but necessary to reuse code from areas that had SRAM that was maintained or targeting ramstage environment.
So we put in postcar to provide a clean boundary between romstage, ramstage, and semantics of dealing w/ cache as ram backing store disappearing. postcar provides smallish, though it seems ROM space is so tight one can't afford it (would be good to see the numbers. it can be compressed as well), environment where it can be loaded in uncached DRAM, clean up cache-as-ram, enable caching for DRAM, and start running all the fancy C code we have.
Moving that to stuff to ramstage inherently means you are loading a bigger footprint into uncacheable space. And one needs to ensure that cache-as-ram teardown is done from assembly (which this CL does) and no fancy anything until caching is set up.
Long story short, I think re-mixing things isn't the best direction because this area is pretty darn complicated to get right. I think we have a pretty good abstraction w/ postcar to where it makes many things straight forward. I would be curious to know numbers on the pressure people are seeing w.r.t. ROM footprints and if there's anything else we can do to help reduce it.
File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/54301/comment/006b227d_1421dc0a PS2, Line 62: #if CONFIG(RAMSTAGE_CAR_TEARDOWN) What's the reasoning for not putting this before the _cbmem_top_ptr memory access? It might be advisable to put this sequence as close to the entry as possible so that it doesn't allow assumptions to break down over time.
File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/54301/comment/ca28b69f_a9b05257 PS2, Line 121: __attribute__((used, __section__(".module_parameters"))) static const volatile uint64_t post_car_mtrrs; static const is making a local object despite the section attribute. What is the toolchain doing to resolve these?
https://review.coreboot.org/c/coreboot/+/54301/comment/e0bf18d9_2c9d6859 PS2, Line 125: post_car_mtrrs Where is this object initialized? It was previously created the by loader of postcar targeting the module parameter space. Now this object memory is within the ramstage address space. I'm not following how this works. Oh. I see there are other CLs. I think the direction of intermixing cache-as-ram w/ c code is ripe for problems -- just like before.
-
Aaron Durbin (Code Review)