Hi Bin, On 15 June 2018 at 03:51, Bin Meng <bmeng...@gmail.com> wrote: > Hi Simon, > > On Thu, Jun 14, 2018 at 8:58 PM, Simon Glass <s...@chromium.org> wrote: >> Hi Alex, >> >> On 13 June 2018 at 04:17, Alexander Graf <ag...@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...@gmail.com> wrote: >>>>> From: Alexander Graf <ag...@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 <ag...@suse.de> >>>>> Reviewed-by: Bin Meng <bmeng...@gmail.com> >>>>> Tested-by: Bin Meng <bmeng...@gmail.com> >>>>> Signed-off-by: Bin Meng <bmeng...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
