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? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
