Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, Felix Held. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 49:
(4 comments)
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/88e3ebdf_d7004861 PS49, Line 204: added to CBFS and loaded by FSP to run PSE.
Maybe a misunderstanding. We don't explicitly mention that a ramstage binary is required to be added to CBFS, that an smihandler binary is required to be added to CBFS etc. Hence, I assumed this "required" is referring to the manual work necessary to add the binary to coreboot, which is what we should avoid and replace with a coreboot-native solution right?
I got 'required' more like it is needed to have the possible needed IPs working. This can be written more precise here. I can work with Sheng to point it out and you are wellcome to place your suggestions if you want, too.
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/29dea86e_75c07d88 PS49, Line 66:
Well, no better solution from the top of my head, and I'm not paid to find one. The basic requirement is that it will be build tested by Jenkins. This can either be done by having a board port in the tree that enables the option by default or by an explicit test- config placed in the `configs/` dir.
This particular code is responsible for putting the image into cbfs, which we do not have right now. So build-testing it without having the image around is tricky without cheating right now (I know, this is currently discussed heavily on the mailing list). And I am kind of opposed to cheating here (e.g. just taking a dummy-file for build-testing). I hope that we can convince Intel to release at least their current code to public, but this will take time to sort out. Would it be an option for you to have that as a TODO?
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/46f97740_39e05907 PS49, Line 385: FSP_ARRAY_LOAD
I do not see exactly how the macro itself hides the types from memcpy().
It doesn't, I said it hides the implementation (hides that memcpy() is used).
OK, I see.
From gcc's point of view
I'm more concerned about the reviewers and human readers point of view.
The former version of it was even worse readable for the reviewer. So I guess the macro at least increases readability with the burden that the reader needs to know its implementation.
, after the preprocessor has done its job, the code looks like it was without the macro. And in addition we have '-Werror=sizeof-pointer-div' enabled so that using just two pointers as parameters to the macro will be caught.
Not concerned about that.
OK, then I got it wrong.
Could you please elaborate your concerns here a bit more so that it is more clear here?
As a reviewer I couldn't see what is going on without looking into the macro implementation and practically review that too. The macro currently does a size check that seems insufficient to remedy the use of memcpy(). I know FSP makes these things tedious, and I don't know a sweet solution either. From a C developer's point of view (who cares about errors made due to the lack of type checking), the best solution seems to be to write explicit loops, copying the values of the arrays one at a time. This way, nothing could go wrong, type-wise. There's still the issue of comparing the length of the arrays, I would do that in the header file where the arrays are declared to keep the code readable. If one really wants to use memcpy(), I suggest to check that the array lengths are exactly the same and the size and alignment (or effective size of the whole array) of the elements are the same.
Note that calling memcpy() implicitly converts all pointers to `void *`, this way bypassing the little type checking C offers.
Understood.
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/6ee532aa_f7537f27 PS49, Line 127: m_cfg->PchPseEnable = CONFIG(PSE_ENABLE);
Could we check for the file in CBFS here, so we could still boot […]
Do we have a CBFS API that can check for a file without loading it?
-
Werner Zeh (Code Review)