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

[Alexander Graf]

On 06/14/2018 02:58 PM, Simon Glass wrote:
> 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.

That's my point. I think most cases that check for CONFIG_X86 are just 
plain wrong.


Alex