[U-Boot] [PATCH v2 02/13] efi.h: Do not use config options

[Simon Glass]

Hi Alex,

On 13 June 2018 at 04:17, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 13.06.18 03:29, Simon Glass wrote:
>> Hi Bin, Alex,
>>
>> On 12 June 2018 at 09:36, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> From: Alexander Graf <agraf at suse.de>
>>>
>>> Currently efi.h determines a few bits of its environment according to
>>> config options. This falls apart with the efi stub support which may
>>> result in efi.h getting pulled into the stub as well as real U-Boot
>>> code. In that case, one may be 32bit while the other one is 64bit.
>>>
>>> This patch changes the conditionals to use compiler provided defines
>>> instead. That way we always adhere to the build environment we're in
>>> and the definitions adjust automatically.
>>>
>>> Signed-off-by: Alexander Graf <agraf at suse.de>
>>> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
>>> Tested-by: Bin Meng <bmeng.cn at gmail.com>
>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>> ---
>>>
>>> Changes in v2: None
>>>
>>>  include/efi.h    | 17 ++++-------------
>>>  lib/efi/Makefile |  4 ++--
>>>  2 files changed, 6 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/efi.h b/include/efi.h
>>> index 98bddba..5e1e8ac 100644
>>> --- a/include/efi.h
>>> +++ b/include/efi.h
>>> @@ -19,12 +19,12 @@
>>>  #include <linux/string.h>
>>>  #include <linux/types.h>
>>>
>>> -#if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined(__x86_64__))
>>> -/* EFI uses the Microsoft ABI which is not the default for GCC */
>>> +/* EFI on x86_64 uses the Microsoft ABI which is not the default for GCC */
>>> +#ifdef __x86_64__
>>>  #define EFIAPI __attribute__((ms_abi))
>>>  #else
>>>  #define EFIAPI asmlinkage
>>> -#endif
>>> +#endif /* __x86_64__ */
>>
>> I made the same comment in another patch. This is becoming too ad-hoc
>> where making EFI builds work is distributed and hidden in such a way
>> that no one will be able to know whether a change causes problems or
>> not.
>>
>> I feel that build config should be deterministic given the CONFIG
>> options provided by the board. Any checks of compiler predefines
>> should be done in one place (efi.h?) and bugs in that stuff should
>> there all be in one place too, and easier to debug and fix.
>
> I actually think the opposite is true. We should get rid of any #ifdef
> CONFIG_ARCH checks throughout the code base that are not meant to
> actually check for the "target" (sandbox for example), but instead
> really only want to know the architecture the code is building against.
>
> We can easily trust the compiler to emit correct defines for the target
> architecture it's building against. That's what every other piece of C
> code on earth depends on. Why be different?

By this logic we would check for __x86_64__ everywhere instead of
CONFIG_X86. I can't think of a better way to explain this without
repeating myself.

Bin, do you understand what I am getting at? Are my concerns unwarranted?

Regards,
Simon