Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, Jincheng Li, Jérémy Compostella, Ronak Kanabar, Shuo Liu.
Subrata Banik 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 14:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80574/comment/7e6e79b1_7a4260f1 : 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.
These functions for example: mp_run_on_aps() will execute by coreboot while mp_startup_this_ap() API is called by FSP (as part of the MP PPI service). now mp_run_on_aps() will call a native UEFI function as per `(void *)procedure`. Now what you are actually doing with this CL, you are advancing the call to ensure that UEFI native call follows efi_ap_procedure_caller(). But I'm missing the point where is the problem i.e., have you encounted any issue which i would love to understand first. This code was landed in CNL time (2016) and since then we have been using it. i can't digest the fact that, we were lucky since the last 8 year w/o any failure if something is fundamentally wrong in the calling convention.
if this has to be fixed for 64-bit execution mode then please state that in commit cleanly and don't claim that we are lucky with current code. I know, the EFI calling method has to follow different method between 32-bit vs 64-bit execution mode.