Attention is currently required from: Arthur Heymans, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Werner Zeh, Patrick Rudolph, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 49:
(3 comments)
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/953e79b4_f04011d8 PS49, Line 204: added to CBFS and loaded by FSP to run PSE.
"A PSE FW binary is required be 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?
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/9131c748_62fe2bc4 PS49, Line 66:
The issue with your approach would be the size checking. […]
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.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/6c08ffa8_8c08f6ab 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).
From gcc's point of view
I'm more concerned about the reviewers and human readers point of view.
, 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.
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.
-
Nico Huber (Code Review)