Attention is currently required from: Arthur Heymans.
Martin L Roth has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/63716?usp=email )
Change subject: [RFC]util/cbfstool: Rewrite trampoline in C ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4: Hey Arthur, this patch was looked at during the bi-weekly group review meeting and the comments came from the entire group.
You marked the patch as RFC, which is why we commented.
I don't think anyone was necessarily against this update, people were just pointing out possible issues - which are the same for every time we refactor working code or port something from pure ASM to inline ASM. There was a concern about the GCC team's choices for inline assembly which is where the comment came from. We rely on it pretty heavily at this point though, so if it's going to break here, it's likely to break elsewhere as well.
Making a 64bit version of this would trivial if it's compiled. Assembly would be pain.
I think this is a good argument for the switch, or at least for using this code for 64-bit booting.
The ASM is well tested and doesn't really change much.
Then the same would apply to a C version.
The point of that comment was that ASM version has been around for over a decade. In a decade, the C version could say the same, but the longevity of the C version has not yet been stress tested.
If this is a concern to people, we could use the C version for x64 until there's confidence and its use could be expanded to the full x86 platform. Additionally, we can further vet the patch by looking at the output from a few generations of GCC to verify that the ASM output by the C version does the same thing as the ASM version.
Since you're not currently working on this, should it be marked WIP until you're ready to pick it up again?
-
Martin L Roth (Code Review)