| Message ID | 1528817785-20208-3-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: efi: Fixes and enhancements to application and payload support | expand |
Hi Bin, Alex, On 12 June 2018 at 09:36, Bin Meng <bmeng.cn@gmail.com> wrote: > From: Alexander Graf <agraf@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@suse.de> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > Tested-by: Bin Meng <bmeng.cn@gmail.com> > Signed-off-by: Bin Meng <bmeng.cn@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. Regards, Simon
On 13.06.18 03:29, Simon Glass wrote: > Hi Bin, Alex, > > On 12 June 2018 at 09:36, Bin Meng <bmeng.cn@gmail.com> wrote: >> From: Alexander Graf <agraf@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@suse.de> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> Tested-by: Bin Meng <bmeng.cn@gmail.com> >> Signed-off-by: Bin Meng <bmeng.cn@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? Alex
Hi Alex, On 13 June 2018 at 04:17, Alexander Graf <agraf@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@gmail.com> wrote: >>> From: Alexander Graf <agraf@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@suse.de> >>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>> Tested-by: Bin Meng <bmeng.cn@gmail.com> >>> Signed-off-by: Bin Meng <bmeng.cn@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
On 06/14/2018 02:58 PM, Simon Glass wrote: > Hi Alex, > > On 13 June 2018 at 04:17, Alexander Graf <agraf@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@gmail.com> wrote: >>>> From: Alexander Graf <agraf@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@suse.de> >>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>>> Tested-by: Bin Meng <bmeng.cn@gmail.com> >>>> Signed-off-by: Bin Meng <bmeng.cn@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
Hi Simon, On Thu, Jun 14, 2018 at 8:58 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Alex, > > On 13 June 2018 at 04:17, Alexander Graf <agraf@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@gmail.com> wrote: >>>> From: Alexander Graf <agraf@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@suse.de> >>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>>> Tested-by: Bin Meng <bmeng.cn@gmail.com> >>>> Signed-off-by: Bin Meng <bmeng.cn@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? Regards, Bin
Hi Bin, On 15 June 2018 at 03:51, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Thu, Jun 14, 2018 at 8:58 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Alex, >> >> On 13 June 2018 at 04:17, Alexander Graf <agraf@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@gmail.com> wrote: >>>>> From: Alexander Graf <agraf@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@suse.de> >>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>>>> Tested-by: Bin Meng <bmeng.cn@gmail.com> >>>>> Signed-off-by: Bin Meng <bmeng.cn@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
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__ */ struct efi_device_path; @@ -32,16 +32,7 @@ typedef struct { u8 b[16]; } efi_guid_t; -#define EFI_BITS_PER_LONG BITS_PER_LONG - -/* - * With 64-bit EFI stub, EFI_BITS_PER_LONG has to be 64. EFI_STUB is set - * in lib/efi/Makefile, when building the stub. - */ -#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) -#undef EFI_BITS_PER_LONG -#define EFI_BITS_PER_LONG 64 -#endif +#define EFI_BITS_PER_LONG (sizeof(long) * 8) /* Bit mask for EFI status code with error */ #define EFI_ERROR_MASK (1UL << (EFI_BITS_PER_LONG - 1)) diff --git a/lib/efi/Makefile b/lib/efi/Makefile index f1a3929..a790d2d 100644 --- a/lib/efi/Makefile +++ b/lib/efi/Makefile @@ -7,11 +7,11 @@ obj-$(CONFIG_EFI_STUB) += efi_info.o CFLAGS_REMOVE_efi_stub.o := -mregparm=3 \ $(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32) -CFLAGS_efi_stub.o := -fpic -fshort-wchar -DEFI_STUB \ +CFLAGS_efi_stub.o := -fpic -fshort-wchar \ $(if $(CONFIG_EFI_STUB_64BIT),-m64) CFLAGS_REMOVE_efi.o := -mregparm=3 \ $(if $(CONFIG_EFI_STUB_64BIT),-march=i386 -m32) -CFLAGS_efi.o := -fpic -fshort-wchar -DEFI_STUB \ +CFLAGS_efi.o := -fpic -fshort-wchar \ $(if $(CONFIG_EFI_STUB_64BIT),-m64) extra-$(CONFIG_EFI_STUB) += efi_stub.o efi.o