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

[Simon Glass]

Hi Bin,

On 15 June 2018 at 03:51, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Jun 14, 2018 at 8:58 PM, Simon Glass <sjg at chromium.org> 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.
>>
>> Bin, do you understand what I am getting at? Are my concerns unwarranted?
>
> I got what you are concerned about. I guess you wanted to say "By this
> logic we would check __x86_64__ everywhere instead of *CONFIG_X86_64*"
> As when CONFIG_X86_64 is defined, the "-m64" flag is passed to
> compiler, and __x86_64__ takes effect. But I think this can only be
> applied in source codes. In makefiles, we still need CONFIG_X86_64.
>
> For the bug we are trying to address here, I believe current patch to
> test __x86_64__ is the simplest way compared to a bunch of config
> options checks. In fact, __x86_64__ contains enough information to fix
> the problem, and the config options checks look superfluous.
>
> How about we add some comments to the changes above to explain some
> more details? Does that look better?

Thanks for looking at this.

Yes comments would really help, thanks.

Regards,
Simon