Change in ...coreboot[master]: soc/intel/common: Implement EFI_MP_SERVICES_PPI structure APIs

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25634 )

Change subject: soc/intel/common: Implement EFI_MP_SERVICES_PPI structure APIs
......................................................................

Patch Set 47:

(1 comment)

Beside the defined to be resolved part, my next concern
is that the current implementation doesn't follow the named
EFI interface accurately. IMHO, we should either never
mention the EFI standard or follow it exactly. No short-
cuts, please.
> @Ron/Philipp/Nico:
>
> Do you have only concern with making use of EFI datatypes ? if so, we can think about something like below https://github.com/u-boot/u-boot/blob/master/include/efi_api.h

I don't think we should use the exact U-Boot definitions
(see inline comment). The names they chose, however, look
much better (one could bikeshed that _t is reserved for
standard C types, but let's not). We have to use the exact
EFI types. If I understood correctly, Ron's concern was
only about their upper-case naming.

Please move the discussion about the types somewhere else.
Looking at this change here is not good for me.

https://review.coreboot.org/#/c/25634/47/src/soc/intel/common/block/cpu/mp_s... 
File src/soc/intel/common/block/cpu/mp_service_ppi.c:

https://review.coreboot.org/#/c/25634/47/src/soc/intel/common/block/cpu/mp_s... 
PS47, Line 27: static EFI_STATUS mp_get_number_of_processors(CONST
> #define efi_uintn_t size_t

We know that this would work with our current toolchain. But I don't see
a reason to make it depend on a vaguely defined type such as `size_t`
(AFAIK, it's definition is `return type of the sizeof operator`).

Please decide first, what problem you want to solve:
a) getting rid of the upper-case names only or
b) getting rid of the UDK header files too.

For a) it would suffice to redefine the existing types, e.g.:

typedef UINTN efi_uintn_t;

For b) please *don't* depend on vague types like `size_t`. Mimic the
respective part of the UDK headers instead, e.g. somewhere in the x86
include path have:

typedef uint32_t efi_uintn_t;

And other architectures (if need be) can define it differently.

If we ever want to interface the existing 32-bit FSP binaries with
x86_64 code, we'll have to use something like that (for all archi-
tectures):

typedef uint32_t efi32_uintn_t;