Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, Jincheng Li, Ronak Kanabar, Shuo Liu, Subrata Banik.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80574?usp=email )
Change subject: drivers/intel/fsp2_0/ppi: Fix FSP PPI callback calling convention ......................................................................
Patch Set 8:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80574/comment/a326beb2_d407d7d5 : PS7, Line 9: Multi Processor PEIM-to-PEIM Interface deals with pointer to function : living in FSP space
this is not correct, with a unified stack between FSP and coreboot. This code is getting executed in the context of coreboot rather than FSP.
Coreboot receives a pointer to a function which is a FSP function hence living in the FSP space. I am not talking about the context of execution just where the function is coming from. In this case it lives in the FSP space and therefore obey to FSP C-calling convention rules.
i would like to understand the problem in detail about what you are running into. So far, we are using this PPI for many platforms and don't see any problem.
Yeah and this is mostly out of luck because the C-calling convention between UEFI (Microsoft) and Linux (Unix like - GCC) is the same on 32 bits (`cdecl`). But technically, the C-calling convention for UEFI functions is defined by the `EFIAPI` macro. C components like coreboot when they call into such functions must ensure they follow the right calling convention. For 64-bits the calling convention is clearly different: Microsoft x64 calling convention vs System V AMD64 ABI.
Currently, the coreboot code as receives a `efi_ap_procedure` function (which correctly includes `EFIAPI` in its definition) but then it discards it by casting it as `void *`. While this does not lead to any issue in 32-bits , this is 1- incorrect 2 - lead to crashes in 64-bits. Because it is just incorrect in the first place I made a dedicated patch instead of including it in the 64-bits support patch.