Attention is currently required from: Arthur Heymans, Dinesh Gehlot, Julius Werner, Kapil Porwal, Lean Sheng Tan, Werner Zeh.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76393?usp=email )
Change subject: src: Implement framework for PRERAM VSD store ......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS10:
Sorry, I still disagree with this whole "VSD" thing and the extra APIs you add for it. I don't see how they serve a purpose. CBMEM is already an allocator that can give you a data area of your preferred size. You don't need to add another API that's just "give me an area of size X" and all it does is chain that call to cbmem_add().
We agreed that pre-RAM CBMEM sounds like a good eventual long-term goal, and that it would take too long right now so you need a temporary stopgap solution instead. So please, for that temporary solution, just use a global variable. I agree that having everything floating around in global variables may get unwieldy long term, but that's why the solution is temporary. You don't need to design a big new temporary "VSD" API just so we can eventually throw it away again and do pre-RAM CBMEM instead. Don't over-engineer a stopgap, just do the simplest thing.
@Julius, it's okay that you don't agree with this solution of having a separate structure and API to read/write the VSD data. Ideally we can't make everyone happy with a single change.
Fundamentally, I'm okay with the longer term proposal but that doesn't mean we will make the short term code messy (using so many global variables floating from different source files) so that it's difficult for anyone to follow and review.
If it just one file where we are declaring all required global variables and then migrate into the cbmem, I would have easily follow your instructions but the usage model is different and there would be multiple file and files between the stages are involved hence, write function to sync those global variables after CBMEM comes online is a job honestly. Compared to that, calling into cbmem_vsd is much simpler where the caller adds the newer variable into the VSD and it's getting auto synced when cbmem comes online.
As you might have noticed that we are working very actively to land this feature because there is a production requirement as well as so dependency CLs are blocked as we are working towards creating an infrastructure change to accommodate the pre-ram cbmem usage better.
I don't see a point to cross-argue for reasons below
1. This feature is guarded against the Kconfig hence, not impacting any other SOC, mainboard unless selected. 2. This feature is not auto selected 3. The user of this feature is still inside the Intel SoC which this team is mostly focusing on. hence, no way it can impact other arch 4. Not touching existing cbmem infrastructure hence, in future decoupling would be easy 5. Usage of memory is still very limited once enabled. 6. Easy to navigate using cbmem_vsd to know the scope of those variables is at pre-ram or post-ram