From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 14 Jun 2018 06:58:37 -0600 Subject: [U-Boot] [PATCH v2 02/13] efi.h: Do not use config options In-Reply-To: <648d9313-bb90-6afb-f83d-d3bb62fe0d1b@suse.de> References: <1528817785-20208-1-git-send-email-bmeng.cn@gmail.com> <1528817785-20208-3-git-send-email-bmeng.cn@gmail.com> <648d9313-bb90-6afb-f83d-d3bb62fe0d1b@suse.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Alex, On 13 June 2018 at 04:17, Alexander Graf wrote: > > > On 13.06.18 03:29, Simon Glass wrote: >> Hi Bin, Alex, >> >> On 12 June 2018 at 09:36, Bin Meng wrote: >>> From: Alexander Graf >>> >>> 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 >>> Reviewed-by: Bin Meng >>> Tested-by: Bin Meng >>> Signed-off-by: Bin Meng >>> --- >>> >>> 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 >>> #include >>> >>> -#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