| Message ID | 1528637118-32739-6-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 |
On 06/10/2018 03:25 PM, Bin Meng wrote: > Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 > 64-bit payload does not work anymore. The call to GetMemoryMap() > in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since > the payload itself is still 32-bit U-Boot Above you say 64-bit payload and now you say 32-bit? Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit Linux EFI stub from an 32-bit EFI implementation in U-Boot? >, efi_uintn_t gets wrongly > interpreted as int, but it should actually be long in a 64-bit EFI > environment. > > Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > include/efi_api.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/efi_api.h b/include/efi_api.h > index 64c27e4..d1158de 100644 > --- a/include/efi_api.h > +++ b/include/efi_api.h > @@ -28,7 +28,11 @@ enum efi_timer_delay { > EFI_TIMER_RELATIVE = 2 > }; > > +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) > +#define efi_uintn_t unsigned long > +#else > #define efi_uintn_t size_t NAK This change will create a lot of build warnings if EFI_STUB and EFI_LOADER are both configured. Could you, please, explain under which compiler settings size_t and unsigned long have a different number of bits? Obviously we have the EFI API exposed by U-Boot which has the bitness of U-Boot. If you want to consume an EFI API of another bitness I suggest that you create separate interface definitions. Best regards Heinrich Schuchardt > +#endif > typedef uint16_t *efi_string_t; > > #define EVT_TIMER 0x80000000 >
Hi Heinrich, On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/10/2018 03:25 PM, Bin Meng wrote: >> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >> 64-bit payload does not work anymore. The call to GetMemoryMap() >> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >> the payload itself is still 32-bit U-Boot > > Above you say 64-bit payload and now you say 32-bit? > > Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit > Linux EFI stub from an 32-bit EFI implementation in U-Boot? > U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as it has to be loaded from the 64-bit EFI BIOS. Note in case you misunderstand: the generated u-boot-payload.efi is 64-bit stub codes (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus 32-bit U-Boot payload. The payload is always 32-bit as of today as U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from README.x86, is far from mature yet. >>, efi_uintn_t gets wrongly >> interpreted as int, but it should actually be long in a 64-bit EFI >> environment. >> >> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> include/efi_api.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/include/efi_api.h b/include/efi_api.h >> index 64c27e4..d1158de 100644 >> --- a/include/efi_api.h >> +++ b/include/efi_api.h >> @@ -28,7 +28,11 @@ enum efi_timer_delay { >> EFI_TIMER_RELATIVE = 2 >> }; >> >> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >> +#define efi_uintn_t unsigned long >> +#else >> #define efi_uintn_t size_t > > NAK > > This change will create a lot of build warnings if EFI_STUB and > EFI_LOADER are both configured. > I don't see any build warnings when building efi-x86_payload32 or efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with these two targets. AFAIK, only x86 supports EFI_STUB currently. I don't know where you see a lot of build warnings. > Could you, please, explain under which compiler settings size_t and > unsigned long have a different number of bits? > As mentioned above, the EFI stub codes (efi_stub.c) are built for 64-bit, which expects 'efi_uintn_t' to be 64-bit. However, it is defined as size_t, which in U-Boot 32, will be 'unsigned int' (see arch/x86/include/asm/posix_types.h) - the bug here! > Obviously we have the EFI API exposed by U-Boot which has the bitness of > U-Boot. > > If you want to consume an EFI API of another bitness I suggest that you > create separate interface definitions. > Regards, Bin
On 06/10/2018 04:30 PM, Bin Meng wrote: > Hi Heinrich, > > On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt > <xypron.glpk@gmx.de> wrote: >> On 06/10/2018 03:25 PM, Bin Meng wrote: >>> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >>> 64-bit payload does not work anymore. The call to GetMemoryMap() >>> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >>> the payload itself is still 32-bit U-Boot >> >> Above you say 64-bit payload and now you say 32-bit? >> >> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit >> Linux EFI stub from an 32-bit EFI implementation in U-Boot? >> > > U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as > it has to be loaded from the 64-bit EFI BIOS. Note in case you > misunderstand: the generated u-boot-payload.efi is 64-bit stub codes > (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus > 32-bit U-Boot payload. The payload is always 32-bit as of today as > U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from > README.x86, is far from mature yet. > >>> , efi_uintn_t gets wrongly >>> interpreted as int, but it should actually be long in a 64-bit EFI >>> environment. >>> >>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> include/efi_api.h | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/include/efi_api.h b/include/efi_api.h >>> index 64c27e4..d1158de 100644 >>> --- a/include/efi_api.h >>> +++ b/include/efi_api.h >>> @@ -28,7 +28,11 @@ enum efi_timer_delay { >>> EFI_TIMER_RELATIVE = 2 >>> }; >>> >>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >>> +#define efi_uintn_t unsigned long >>> +#else >>> #define efi_uintn_t size_t >> >> NAK >> >> This change will create a lot of build warnings if EFI_STUB and >> EFI_LOADER are both configured. >> > > I don't see any build warnings when building efi-x86_payload32 or > efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with > these two targets. AFAIK, only x86 supports EFI_STUB currently. I > don't know where you see a lot of build warnings. Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit stub. See lib/efi_loader/Kconfig. The problem is with the build scripts for the stub using the same CONFIG variables as those used for other binaries. To emulate what would happen with your change once we can build with EFI_LOADER=y and 64bit stub I made the following change: --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,7 +28,8 @@ enum efi_timer_delay { EFI_TIMER_RELATIVE = 2 }; -#define efi_uintn_t size_t +#define efi_uintn_t unsigned long typedef uint16_t *efi_string_t; And then I tried to build: make qemu-x86_defconfig make -j6 It gives me a bunch of errors like below. ^~~~~~~~~ include/efi_loader.h:31:8: warning: format ‘%zu’ expects argument of type ‘size_t’, but argument 13 has type ‘long unsigned int’ [-Wformat=] debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ ^ include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’ #define pr_fmt(fmt) fmt ^~~ include/log.h:141:2: note: in expansion of macro ‘debug_cond’ debug_cond(_DEBUG, fmt, ##args) ^~~~~~~~~~ include/efi_loader.h:31:2: note: in expansion of macro ‘debug’ debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ ^~~~~ lib/efi_loader/efi_gop.c:333:2: note: in expansion of macro ‘EFI_ENTRY’ EFI_ENTRY("%p, %p, %u, %zu, %zu, %zu, %zu, %zu, %zu, %zu", this, ^~~~~~~~~ In file included from include/linux/bug.h:7:0, from include/common.h:25, from lib/efi_loader/efi_disk.c:8: lib/efi_loader/efi_disk.c: In function ‘efi_disk_read_blocks’: include/efi_loader.h:31:8: warning: format ‘%zx’ expects argument of type ‘size_t’, but argument 7 has type ‘long unsigned int’ [-Wformat=] debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ ^ include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’ #define pr_fmt(fmt) fmt ^~~ include/log.h:141:2: note: in expansion of macro ‘debug_cond’ debug_cond(_DEBUG, fmt, ##args) ^~~~~~~~~~ include/efi_loader.h:31:2: note: in expansion of macro ‘debug’ debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ ^~~~~ lib/efi_loader/efi_disk.c:114:2: note: in expansion of macro ‘EFI_ENTRY’ EFI_ENTRY("%p, %x, %" PRIx64 ", %zx, %p", this, media_id, lba, ^~~~~~~~~ lib/efi_loader/efi_disk.c: In function ‘efi_disk_write_blocks’: include/efi_loader.h:31:8: warning: format ‘%zx’ expects argument of type ‘size_t’, but argument 7 has type ‘long unsigned int’ [-Wformat=] debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ ^ include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’ #define pr_fmt(fmt) fmt ^~~ include/log.h:141:2: note: in expansion of macro ‘debug_cond’ debug_cond(_DEBUG, fmt, ##args) ^~~~~~~~~~ include/efi_loader.h:31:2: note: in expansion of macro ‘debug’ debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \ So, please, do not change the type of efi_uintn_t. What is needed is using different types in lib/efi_loader/ and in the calls to the primary firmware if they have different bitness. Best regards Heinrich > >> Could you, please, explain under which compiler settings size_t and >> unsigned long have a different number of bits? >> > > As mentioned above, the EFI stub codes (efi_stub.c) are built for > 64-bit, which expects 'efi_uintn_t' to be 64-bit. However, it is > defined as size_t, which in U-Boot 32, will be 'unsigned int' (see > arch/x86/include/asm/posix_types.h) - the bug here! > >> Obviously we have the EFI API exposed by U-Boot which has the bitness of >> U-Boot. >> >> If you want to consume an EFI API of another bitness I suggest that you >> create separate interface definitions. >> > > Regards, > Bin >
Hi Heinrich, On Mon, Jun 11, 2018 at 2:17 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/10/2018 04:30 PM, Bin Meng wrote: >> Hi Heinrich, >> >> On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt >> <xypron.glpk@gmx.de> wrote: >>> On 06/10/2018 03:25 PM, Bin Meng wrote: >>>> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >>>> 64-bit payload does not work anymore. The call to GetMemoryMap() >>>> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >>>> the payload itself is still 32-bit U-Boot >>> >>> Above you say 64-bit payload and now you say 32-bit? >>> >>> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit >>> Linux EFI stub from an 32-bit EFI implementation in U-Boot? >>> >> >> U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as >> it has to be loaded from the 64-bit EFI BIOS. Note in case you >> misunderstand: the generated u-boot-payload.efi is 64-bit stub codes >> (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus >> 32-bit U-Boot payload. The payload is always 32-bit as of today as >> U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from >> README.x86, is far from mature yet. >> >>>> , efi_uintn_t gets wrongly >>>> interpreted as int, but it should actually be long in a 64-bit EFI >>>> environment. >>>> >>>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>> --- >>>> >>>> include/efi_api.h | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/include/efi_api.h b/include/efi_api.h >>>> index 64c27e4..d1158de 100644 >>>> --- a/include/efi_api.h >>>> +++ b/include/efi_api.h >>>> @@ -28,7 +28,11 @@ enum efi_timer_delay { >>>> EFI_TIMER_RELATIVE = 2 >>>> }; >>>> >>>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >>>> +#define efi_uintn_t unsigned long >>>> +#else >>>> #define efi_uintn_t size_t >>> >>> NAK >>> >>> This change will create a lot of build warnings if EFI_STUB and >>> EFI_LOADER are both configured. >>> >> >> I don't see any build warnings when building efi-x86_payload32 or >> efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with >> these two targets. AFAIK, only x86 supports EFI_STUB currently. I >> don't know where you see a lot of build warnings. > > Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit > stub. See lib/efi_loader/Kconfig. The problem is with the build scripts > for the stub using the same CONFIG variables as those used for other > binaries. > > To emulate what would happen with your change once we can build with > EFI_LOADER=y and 64bit stub I made the following change: > > --- a/include/efi_api.h > +++ b/include/efi_api.h > @@ -28,7 +28,8 @@ enum efi_timer_delay { > EFI_TIMER_RELATIVE = 2 > }; > > -#define efi_uintn_t size_t > +#define efi_uintn_t unsigned long > typedef uint16_t *efi_string_t; > I am not sure why do you unconditionally change efi_uintn_t? My patch has #if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) to guard the change. > And then I tried to build: > make qemu-x86_defconfig > make -j6 > I know. But my patch does not produce any build warnings on efi-x86_payload32_defconfig and efi-x86_payload64_defconfig, on u-boot-x86/efi-working branch. > It gives me a bunch of errors like below. > [snip] Regards, Bin
Hi Alex, On Sun, Jun 10, 2018 at 9:25 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 > 64-bit payload does not work anymore. The call to GetMemoryMap() > in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since > the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly > interpreted as int, but it should actually be long in a 64-bit EFI > environment. > > Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > include/efi_api.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/efi_api.h b/include/efi_api.h > index 64c27e4..d1158de 100644 > --- a/include/efi_api.h > +++ b/include/efi_api.h > @@ -28,7 +28,11 @@ enum efi_timer_delay { > EFI_TIMER_RELATIVE = 2 > }; > > +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) > +#define efi_uintn_t unsigned long > +#else > #define efi_uintn_t size_t > +#endif > typedef uint16_t *efi_string_t; > > #define EVT_TIMER 0x80000000 > -- With your proposed changes to efi.h in [1], do you think we should also fix this by: #ifdef __x86_64__ #define efi_uintn_t unsigned long #else #define efi_uintn_t size_t [1] https://lists.denx.de/pipermail/u-boot/2018-June/331193.html Regards, Bin
On 06/11/2018 01:36 AM, Bin Meng wrote: > Hi Heinrich, > > On Mon, Jun 11, 2018 at 2:17 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> On 06/10/2018 04:30 PM, Bin Meng wrote: >>> Hi Heinrich, >>> >>> On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt >>> <xypron.glpk@gmx.de> wrote: >>>> On 06/10/2018 03:25 PM, Bin Meng wrote: >>>>> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >>>>> 64-bit payload does not work anymore. The call to GetMemoryMap() >>>>> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >>>>> the payload itself is still 32-bit U-Boot >>>> >>>> Above you say 64-bit payload and now you say 32-bit? >>>> >>>> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit >>>> Linux EFI stub from an 32-bit EFI implementation in U-Boot? >>>> >>> >>> U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as >>> it has to be loaded from the 64-bit EFI BIOS. Note in case you >>> misunderstand: the generated u-boot-payload.efi is 64-bit stub codes >>> (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus >>> 32-bit U-Boot payload. The payload is always 32-bit as of today as >>> U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from >>> README.x86, is far from mature yet. >>> >>>>> , efi_uintn_t gets wrongly >>>>> interpreted as int, but it should actually be long in a 64-bit EFI >>>>> environment. >>>>> >>>>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>> --- >>>>> >>>>> include/efi_api.h | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/include/efi_api.h b/include/efi_api.h >>>>> index 64c27e4..d1158de 100644 >>>>> --- a/include/efi_api.h >>>>> +++ b/include/efi_api.h >>>>> @@ -28,7 +28,11 @@ enum efi_timer_delay { >>>>> EFI_TIMER_RELATIVE = 2 >>>>> }; >>>>> >>>>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >>>>> +#define efi_uintn_t unsigned long >>>>> +#else >>>>> #define efi_uintn_t size_t >>>> >>>> NAK >>>> >>>> This change will create a lot of build warnings if EFI_STUB and >>>> EFI_LOADER are both configured. >>>> >>> >>> I don't see any build warnings when building efi-x86_payload32 or >>> efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with >>> these two targets. AFAIK, only x86 supports EFI_STUB currently. I >>> don't know where you see a lot of build warnings. >> >> Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit >> stub. See lib/efi_loader/Kconfig. The problem is with the build scripts >> for the stub using the same CONFIG variables as those used for other >> binaries. >> >> To emulate what would happen with your change once we can build with >> EFI_LOADER=y and 64bit stub I made the following change: >> >> --- a/include/efi_api.h >> +++ b/include/efi_api.h >> @@ -28,7 +28,8 @@ enum efi_timer_delay { >> EFI_TIMER_RELATIVE = 2 >> }; >> >> -#define efi_uintn_t size_t >> +#define efi_uintn_t unsigned long >> typedef uint16_t *efi_string_t; >> > > I am not sure why do you unconditionally change efi_uintn_t? My patch has > > #if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) Currently it is not possible to enable CONFIG_EFI_STUB_64BIT and EFI_LOADER on 32bit U-Boot. Once we have removed this restriction in the build system your patch together with EFI_LOADER=y and CONFIG_EFI_STUB_64BIT=y will provoke all those errors. This is why I NAKed you patch. Best regards Heinrich > > to guard the change. > >> And then I tried to build: >> make qemu-x86_defconfig >> make -j6 >> > > I know. But my patch does not produce any build warnings on > efi-x86_payload32_defconfig and efi-x86_payload64_defconfig, on > u-boot-x86/efi-working branch. > >> It gives me a bunch of errors like below. >> > > [snip] > > Regards, > Bin >
Hi Heinrich, On Mon, Jun 11, 2018 at 11:31 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/11/2018 01:36 AM, Bin Meng wrote: >> Hi Heinrich, >> >> On Mon, Jun 11, 2018 at 2:17 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> On 06/10/2018 04:30 PM, Bin Meng wrote: >>>> Hi Heinrich, >>>> >>>> On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt >>>> <xypron.glpk@gmx.de> wrote: >>>>> On 06/10/2018 03:25 PM, Bin Meng wrote: >>>>>> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >>>>>> 64-bit payload does not work anymore. The call to GetMemoryMap() >>>>>> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >>>>>> the payload itself is still 32-bit U-Boot >>>>> >>>>> Above you say 64-bit payload and now you say 32-bit? >>>>> >>>>> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit >>>>> Linux EFI stub from an 32-bit EFI implementation in U-Boot? >>>>> >>>> >>>> U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as >>>> it has to be loaded from the 64-bit EFI BIOS. Note in case you >>>> misunderstand: the generated u-boot-payload.efi is 64-bit stub codes >>>> (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus >>>> 32-bit U-Boot payload. The payload is always 32-bit as of today as >>>> U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from >>>> README.x86, is far from mature yet. >>>> >>>>>> , efi_uintn_t gets wrongly >>>>>> interpreted as int, but it should actually be long in a 64-bit EFI >>>>>> environment. >>>>>> >>>>>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>> --- >>>>>> >>>>>> include/efi_api.h | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/include/efi_api.h b/include/efi_api.h >>>>>> index 64c27e4..d1158de 100644 >>>>>> --- a/include/efi_api.h >>>>>> +++ b/include/efi_api.h >>>>>> @@ -28,7 +28,11 @@ enum efi_timer_delay { >>>>>> EFI_TIMER_RELATIVE = 2 >>>>>> }; >>>>>> >>>>>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >>>>>> +#define efi_uintn_t unsigned long >>>>>> +#else >>>>>> #define efi_uintn_t size_t >>>>> >>>>> NAK >>>>> >>>>> This change will create a lot of build warnings if EFI_STUB and >>>>> EFI_LOADER are both configured. >>>>> >>>> >>>> I don't see any build warnings when building efi-x86_payload32 or >>>> efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with >>>> these two targets. AFAIK, only x86 supports EFI_STUB currently. I >>>> don't know where you see a lot of build warnings. >>> >>> Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit >>> stub. See lib/efi_loader/Kconfig. The problem is with the build scripts >>> for the stub using the same CONFIG variables as those used for other >>> binaries. >>> >>> To emulate what would happen with your change once we can build with >>> EFI_LOADER=y and 64bit stub I made the following change: >>> >>> --- a/include/efi_api.h >>> +++ b/include/efi_api.h >>> @@ -28,7 +28,8 @@ enum efi_timer_delay { >>> EFI_TIMER_RELATIVE = 2 >>> }; >>> >>> -#define efi_uintn_t size_t >>> +#define efi_uintn_t unsigned long >>> typedef uint16_t *efi_string_t; >>> >> >> I am not sure why do you unconditionally change efi_uintn_t? My patch has >> >> #if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) > > Currently it is not possible to enable CONFIG_EFI_STUB_64BIT and > EFI_LOADER on 32bit U-Boot. Once we have removed this restriction in the > build system your patch together with EFI_LOADER=y and > CONFIG_EFI_STUB_64BIT=y will provoke all those errors. This is why I > NAKed you patch. > I did the following to try to reproduce what you saw: diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index d38780b..29e67b6 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -3,10 +3,6 @@ config EFI_LOADER depends on (ARM || X86) && OF_LIBFDT # We do not support bootefi booting ARMv7 in non-secure mode depends on !ARMV7_NONSEC - # We need EFI_STUB_64BIT to be set on x86_64 with EFI_STUB - depends on !EFI_STUB || !X86_64 || EFI_STUB_64BIT - # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB - depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT default y select LIB_UUID select HAVE_BLOCK_DEVICE Then do a $ make efi-x86_payload32_defconfig $ make It succeeded without any error But doing: $ make efi-x86_payload64_defconfig $ make Failed with a different error (no compiler warning were seen) toolchains/gcc-7.3.0-nolibc/i386-linux/bin/i386-linux-ld.bfd: i386 architecture of input file `lib/efi_loader/helloworld.o' is incompatible with i386:x86-64 output scripts/Makefile.lib:407: recipe for target 'lib/efi_loader/helloworld_efi.so' failed Switching to x86_64-linux-gcc, exposed the same error: toolchains/gcc-7.3.0-nolibc/x86_64-linux/bin/x86_64-linux-ld.bfd: i386 architecture of input file `lib/efi_loader/helloworld.o' is incompatible with i386:x86-64 output Regards, Bin
On 06/11/2018 06:35 PM, Bin Meng wrote: > Hi Heinrich, > > On Mon, Jun 11, 2018 at 11:31 PM, Heinrich Schuchardt > <xypron.glpk@gmx.de> wrote: >> On 06/11/2018 01:36 AM, Bin Meng wrote: >>> Hi Heinrich, >>> >>> On Mon, Jun 11, 2018 at 2:17 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> On 06/10/2018 04:30 PM, Bin Meng wrote: >>>>> Hi Heinrich, >>>>> >>>>> On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt >>>>> <xypron.glpk@gmx.de> wrote: >>>>>> On 06/10/2018 03:25 PM, Bin Meng wrote: >>>>>>> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >>>>>>> 64-bit payload does not work anymore. The call to GetMemoryMap() >>>>>>> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >>>>>>> the payload itself is still 32-bit U-Boot >>>>>> >>>>>> Above you say 64-bit payload and now you say 32-bit? >>>>>> >>>>>> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit >>>>>> Linux EFI stub from an 32-bit EFI implementation in U-Boot? >>>>>> >>>>> >>>>> U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as >>>>> it has to be loaded from the 64-bit EFI BIOS. Note in case you >>>>> misunderstand: the generated u-boot-payload.efi is 64-bit stub codes >>>>> (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus >>>>> 32-bit U-Boot payload. The payload is always 32-bit as of today as >>>>> U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from >>>>> README.x86, is far from mature yet. >>>>> >>>>>>> , efi_uintn_t gets wrongly >>>>>>> interpreted as int, but it should actually be long in a 64-bit EFI >>>>>>> environment. >>>>>>> >>>>>>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>> --- >>>>>>> >>>>>>> include/efi_api.h | 4 ++++ >>>>>>> 1 file changed, 4 insertions(+) >>>>>>> >>>>>>> diff --git a/include/efi_api.h b/include/efi_api.h >>>>>>> index 64c27e4..d1158de 100644 >>>>>>> --- a/include/efi_api.h >>>>>>> +++ b/include/efi_api.h >>>>>>> @@ -28,7 +28,11 @@ enum efi_timer_delay { >>>>>>> EFI_TIMER_RELATIVE = 2 >>>>>>> }; >>>>>>> >>>>>>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >>>>>>> +#define efi_uintn_t unsigned long >>>>>>> +#else >>>>>>> #define efi_uintn_t size_t >>>>>> >>>>>> NAK >>>>>> >>>>>> This change will create a lot of build warnings if EFI_STUB and >>>>>> EFI_LOADER are both configured. >>>>>> >>>>> >>>>> I don't see any build warnings when building efi-x86_payload32 or >>>>> efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with >>>>> these two targets. AFAIK, only x86 supports EFI_STUB currently. I >>>>> don't know where you see a lot of build warnings. >>>> >>>> Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit >>>> stub. See lib/efi_loader/Kconfig. The problem is with the build scripts >>>> for the stub using the same CONFIG variables as those used for other >>>> binaries. >>>> >>>> To emulate what would happen with your change once we can build with >>>> EFI_LOADER=y and 64bit stub I made the following change: >>>> >>>> --- a/include/efi_api.h >>>> +++ b/include/efi_api.h >>>> @@ -28,7 +28,8 @@ enum efi_timer_delay { >>>> EFI_TIMER_RELATIVE = 2 >>>> }; >>>> >>>> -#define efi_uintn_t size_t >>>> +#define efi_uintn_t unsigned long >>>> typedef uint16_t *efi_string_t; >>>> >>> >>> I am not sure why do you unconditionally change efi_uintn_t? My patch has >>> >>> #if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >> >> Currently it is not possible to enable CONFIG_EFI_STUB_64BIT and >> EFI_LOADER on 32bit U-Boot. Once we have removed this restriction in the >> build system your patch together with EFI_LOADER=y and >> CONFIG_EFI_STUB_64BIT=y will provoke all those errors. This is why I >> NAKed you patch. >> My mistake was to confuse EFI_STUB and CONFIG_EFI_STUB. EFI_STUB is only defined in lib/efi/Makefile when compiling lib/efi/efi.c and lib/efi/efi_stub.c These two files are compiled with -m64 when CONFIG_EFI_STUB_64BIT=y. The problem should be fixed in arch/x86/include/asm/posix_types.h That way anybody using size_t in lib/efi/ can be sure that size_t has the same size as a pointer, cf: lib/efi/efi_stub.c:92: void *memcpy(void *dest, const void *src, size_t size) lib/efi/efi_stub.c:104: void *memset(void *inptr, int ch, size_t size) > > I did the following to try to reproduce what you saw: > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index d38780b..29e67b6 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -3,10 +3,6 @@ config EFI_LOADER > depends on (ARM || X86) && OF_LIBFDT > # We do not support bootefi booting ARMv7 in non-secure mode > depends on !ARMV7_NONSEC > - # We need EFI_STUB_64BIT to be set on x86_64 with EFI_STUB > - depends on !EFI_STUB || !X86_64 || EFI_STUB_64BIT > - # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB > - depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT > default y > select LIB_UUID > select HAVE_BLOCK_DEVICE > > Then do a > $ make efi-x86_payload32_defconfig > $ make > > It succeeded without any error > > But doing: > $ make efi-x86_payload64_defconfig > $ make > > Failed with a different error (no compiler warning were seen) > > toolchains/gcc-7.3.0-nolibc/i386-linux/bin/i386-linux-ld.bfd: i386 > architecture of input file `lib/efi_loader/helloworld.o' is > incompatible with i386:x86-64 output > scripts/Makefile.lib:407: recipe for target > 'lib/efi_loader/helloworld_efi.so' failed This is due to using variable EFI_LDS, EFI_CRT0, and EFI_RELOC both when building the stub (64bit) and helloworld (32bit) (see arch/x86/config.mk). To resolve the issue we would have to use different values for the stub and the other EFI payloads (helloworld and efi_selftest_miniapp_*). Best regards Heinrich > > Switching to x86_64-linux-gcc, exposed the same error: > > toolchains/gcc-7.3.0-nolibc/x86_64-linux/bin/x86_64-linux-ld.bfd: i386 > architecture of input file `lib/efi_loader/helloworld.o' is > incompatible with i386:x86-64 output > > Regards, > Bin >
On 11.06.18 11:11, Bin Meng wrote: > Hi Alex, > > On Sun, Jun 10, 2018 at 9:25 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 >> 64-bit payload does not work anymore. The call to GetMemoryMap() >> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since >> the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly >> interpreted as int, but it should actually be long in a 64-bit EFI >> environment. >> >> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> include/efi_api.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/include/efi_api.h b/include/efi_api.h >> index 64c27e4..d1158de 100644 >> --- a/include/efi_api.h >> +++ b/include/efi_api.h >> @@ -28,7 +28,11 @@ enum efi_timer_delay { >> EFI_TIMER_RELATIVE = 2 >> }; >> >> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) >> +#define efi_uintn_t unsigned long >> +#else >> #define efi_uintn_t size_t >> +#endif >> typedef uint16_t *efi_string_t; >> >> #define EVT_TIMER 0x80000000 >> -- > > With your proposed changes to efi.h in [1], do you think we should > also fix this by: > > #ifdef __x86_64__ > #define efi_uintn_t unsigned long > #else > #define efi_uintn_t size_t Can't we just change the definition in arch/x86/include/asm/posix_types.h to adhere to __x86_64__ instead? Then the typedef would just simply always go to size_t. Alex
diff --git a/include/efi_api.h b/include/efi_api.h index 64c27e4..d1158de 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -28,7 +28,11 @@ enum efi_timer_delay { EFI_TIMER_RELATIVE = 2 }; +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB) +#define efi_uintn_t unsigned long +#else #define efi_uintn_t size_t +#endif typedef uint16_t *efi_string_t; #define EVT_TIMER 0x80000000
Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86 64-bit payload does not work anymore. The call to GetMemoryMap() in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly interpreted as int, but it should actually be long in a 64-bit EFI environment. Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t") Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- include/efi_api.h | 4 ++++ 1 file changed, 4 insertions(+)