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 13:
(1 comment)
Patchset:
PS10:
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.
I really don't follow you here. Where are these multiple files that need to be involved here? Are you talking about code that is not uploaded as part of this patch series yet?
Yes. Intel team is working on a new feature implementation which might need to store additional VSD data to be able to access between pre-DRAM init stage and post-ram phase.
As far as I can tell, CB:76395 is the only user of this API, and in that patch the entire code to handle this would be kept within cse_lite.c (and amount to literally one line to declare the variable, one line to memcpy() into it, and a trivial CBMEM_CREATION_HOOK() function to copy it from there into CBMEM).
I guess, I'm trying to convey you that, just don't go by *one* variable at this moment. Based on the visibility I have on the in-pipe implementation, there would be atleast 4 such variable which we need to sync between pre-dram and post-dram phase.
All the other stuff from that patch (in ish.c and cse.c) runs in ramstage and would only need to call cbmem_find().
Correct, you might have noticed that I passed the same feedback in the code that cbmem_find() is enough for ramstage stage functions (as sync already initiated)
Or am I misunderstanding something? (Also note that there is plenty of existing precedent for this kind of thing in coreboot already, for example the `mem_chip_info` variable in `src/soc/qualcomm/common/qclib.c`.)
Meanwhile, I think what you're trying to add here is "messy". You're adding something that sounds like a multi-purpose container ("vendor data"), but really there's only space for a single allocation inside it, and there's no way to identify what kind of data it contains other than reading through the code to find out what was put in there on the other side.
Why you think so? reading the VSD data structure will be enough to understand the length and width of this feature (which I can only assume from here it will grow).
What's the point in calling it "vendor data" in the first place, instead of "ISH version", when that's the only thing it can ever really be used for?
As mention earlier, CSE version, ISH version, CSE downgrade region, CSE downgrade is initiated or not, such information are expected to be stored into the VSD. We can't land all changes at once. Because there are team working on those feature. And I need to make sure we have base CL ready for them to land the additional changes.
You're making it sound reusable but the way you're writing it it really isn't.
Wondering how you can say so? I have visibility on things at Intel side about how things will shape in coming month hence, its easy for me to design things in that way. What you need is to have patients and trust on the implementation that we are not here to make the coreboot messy and don't break other platforms.
And why are you allocating a separate CAR area for something that isn't actually getting passed between stages?
We don't want to have multiple static/global variables floating between pre-DRAM and post-DRAM phase hence, the better way to manage things if we relying on the CAR data which would be migrated once we have physical memory. One big data structure to keep required information rather being scattered (like multiple global/static variables).
I'm sorry, but I just think this is a bad design. It is confusingly named for what it does, it is a lot more complex than it needs to be to solve this problem, and it can't be reused for anything else to justify that extra complexity because now it is already "full" with the ISH version.
The size is a kconfig override hence, not big deal to increase the size from soc/mainboard to keep more req information, This is what we do for any pre-ram car variable usage. Don't see anything that different in this implementation.
If you want something that can really be reused for other pre-RAM passing-between-stage needs, then it needs to be able to handle multiple different allocations side by side, and that would essentially be the pre-RAM CBMEM thing that we already agreed was a good idea but takes too long for your time frame right now.
Sure, pre-ram cbmem implementations would have been ideal but we don't have that much time in hand (we missed the timeline of 07/31 already) to pick a new implementation and testing (which might take few quarters) to land and enable this feature.