[PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Victor Erminpour]

Enable the __efistub_global define to place variables in the
.data section for both CONFIG_ARM and CONFIG_ARM64.

This places the EFIstub sys_table variable and other EFIstub
static variables in the .data section for both CONFIG_ARM and
CONFIG_ARM64.

Signed-off-by: Victor Erminpour <victor.erminpour@oracle.com>
---
 drivers/firmware/efi/libstub/efistub.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index c244b16..59932d6 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -25,7 +25,7 @@
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 #define __efistub_global	__section(.data)
 #else
 #define __efistub_global

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Ard Biesheuvel]

On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
<victor.erminpour@oracle.com> wrote:
>
> Enable the __efistub_global define to place variables in the
> .data section for both CONFIG_ARM and CONFIG_ARM64.
>
> This places the EFIstub sys_table variable and other EFIstub
> static variables in the .data section for both CONFIG_ARM and
> CONFIG_ARM64.
>

What does that achieve?

> Signed-off-by: Victor Erminpour <victor.erminpour@oracle.com>
> ---
>  drivers/firmware/efi/libstub/efistub.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index c244b16..59932d6 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -25,7 +25,7 @@
>  #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
>  #endif
>
> -#ifdef CONFIG_ARM
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>  #define __efistub_global       __section(.data)
>  #else
>  #define __efistub_global

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Victor Erminpour]

On 4/10/20 1:09 AM, Ard Biesheuvel wrote:
> On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
> <victor.erminpour@oracle.com> wrote:
>>
>> Enable the __efistub_global define to place variables in the
>> .data section for both CONFIG_ARM and CONFIG_ARM64.
>>
>> This places the EFIstub sys_table variable and other EFIstub
>> static variables in the .data section for both CONFIG_ARM and
>> CONFIG_ARM64.
>>
> 
> What does that achieve?

Hi Ard,

Without placing these global variables in .data, I get the
following errors when booting an ARM64 EFI system:

EFI stub: ERROR: Exit boot services failed.
EFI stub: ERROR: Failed to update FDT and exit boot services


I know that the ARM64 linker script is supposed to put the
.init.bss into the .init.data section, but I don't think this
is happening for all systems.

Having it explicitly enabled for CONFIG_ARM64 worked for me.


Regards,
--Victor


> 
>> Signed-off-by: Victor Erminpour <victor.erminpour@oracle.com>
>> ---
>>   drivers/firmware/efi/libstub/efistub.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> index c244b16..59932d6 100644
>> --- a/drivers/firmware/efi/libstub/efistub.h
>> +++ b/drivers/firmware/efi/libstub/efistub.h
>> @@ -25,7 +25,7 @@
>>   #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
>>   #endif
>>
>> -#ifdef CONFIG_ARM
>> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>>   #define __efistub_global       __section(.data)
>>   #else
>>   #define __efistub_global

-- 
Victor Hugo Erminpour
Principal Member of Technical Staff
Oracle America

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Ard Biesheuvel]

On Sat, 11 Apr 2020 at 00:12, Victor Erminpour
<victor.erminpour@oracle.com> wrote:
>
>
>
> On 4/10/20 1:09 AM, Ard Biesheuvel wrote:
> > On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
> > <victor.erminpour@oracle.com> wrote:
> >>
> >> Enable the __efistub_global define to place variables in the
> >> .data section for both CONFIG_ARM and CONFIG_ARM64.
> >>
> >> This places the EFIstub sys_table variable and other EFIstub
> >> static variables in the .data section for both CONFIG_ARM and
> >> CONFIG_ARM64.
> >>
> >
> > What does that achieve?
>
> Hi Ard,
>
> Without placing these global variables in .data, I get the
> following errors when booting an ARM64 EFI system:
>
> EFI stub: ERROR: Exit boot services failed.
> EFI stub: ERROR: Failed to update FDT and exit boot services
>

Which boot loader are you using? Does this involve shim?

Also, does it help if you add 'efi=no_disable_early_pci_dma'?


>
> I know that the ARM64 linker script is supposed to put the
> .init.bss into the .init.data section, but I don't think this
> is happening for all systems.
>
> Having it explicitly enabled for CONFIG_ARM64 worked for me.
>

OK, thanks for the report. However, we will be removing
__efistub_global entirely during the next cycle, so this is not the
right fix.

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Arvind Sankar]

On Sat, Apr 11, 2020 at 08:39:00AM +0200, Ard Biesheuvel wrote:
> On Sat, 11 Apr 2020 at 00:12, Victor Erminpour
> <victor.erminpour@oracle.com> wrote:
> >
> >
> >
> > On 4/10/20 1:09 AM, Ard Biesheuvel wrote:
> > > On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
> > > <victor.erminpour@oracle.com> wrote:
> > >>
> > >> Enable the __efistub_global define to place variables in the
> > >> .data section for both CONFIG_ARM and CONFIG_ARM64.
> > >>
> > >> This places the EFIstub sys_table variable and other EFIstub
> > >> static variables in the .data section for both CONFIG_ARM and
> > >> CONFIG_ARM64.
> > >>
> > >
> > > What does that achieve?
> >
> > Hi Ard,
> >
> > Without placing these global variables in .data, I get the
> > following errors when booting an ARM64 EFI system:
> >
> > EFI stub: ERROR: Exit boot services failed.
> > EFI stub: ERROR: Failed to update FDT and exit boot services
> >
> 
> Which boot loader are you using? Does this involve shim?
> 
> Also, does it help if you add 'efi=no_disable_early_pci_dma'?
> 
> 
> >
> > I know that the ARM64 linker script is supposed to put the
> > .init.bss into the .init.data section, but I don't think this
> > is happening for all systems.
> >
> > Having it explicitly enabled for CONFIG_ARM64 worked for me.
> >
> 
> OK, thanks for the report. However, we will be removing
> __efistub_global entirely during the next cycle, so this is not the
> right fix.

Victor, can you attach your kernel configuration and the output of
	objdump -t drivers/firmware/efi/libstub/lib.a
on the non-working kernel and the working kernel?

Also, is this failing with the latest git kernel or some other version?

Thanks.

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Victor Erminpour]

On 4/10/20 11:39 PM, Ard Biesheuvel wrote:
> On Sat, 11 Apr 2020 at 00:12, Victor Erminpour
> <victor.erminpour@oracle.com> wrote:
>>
>>
>>
>> On 4/10/20 1:09 AM, Ard Biesheuvel wrote:
>>> On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
>>> <victor.erminpour@oracle.com> wrote:
>>>>
>>>> Enable the __efistub_global define to place variables in the
>>>> .data section for both CONFIG_ARM and CONFIG_ARM64.
>>>>
>>>> This places the EFIstub sys_table variable and other EFIstub
>>>> static variables in the .data section for both CONFIG_ARM and
>>>> CONFIG_ARM64.
>>>>
>>>
>>> What does that achieve?
>>
>> Hi Ard,
>>
>> Without placing these global variables in .data, I get the
>> following errors when booting an ARM64 EFI system:
>>
>> EFI stub: ERROR: Exit boot services failed.
>> EFI stub: ERROR: Failed to update FDT and exit boot services
>>
> 
> Which boot loader are you using? Does this involve shim?
> 

grub2-efi-aa64-2.02-0.80.0.4.el7.aarch64
shim-aa64-15-1.0.4.el7.aarch64

> Also, does it help if you add 'efi=no_disable_early_pci_dma'?
> 

Tried this boot time option, but to no effect.

> 
>>
>> I know that the ARM64 linker script is supposed to put the
>> .init.bss into the .init.data section, but I don't think this
>> is happening for all systems.
>>
>> Having it explicitly enabled for CONFIG_ARM64 worked for me.
>>
> 
> OK, thanks for the report. However, we will be removing
> __efistub_global entirely during the next cycle, so this is not the
> right fix.

Thanks Ard, this sounds promising!

Regards,
--Victor


> 

-- 
Victor Hugo Erminpour
Principal Member of Technical Staff
Oracle America

Re: [PATCH] efi/libstub/arm64: Enable __efistub_global define in .data section

[Ard Biesheuvel]

On Tue, 14 Apr 2020 at 00:57, Victor Erminpour
<victor.erminpour@oracle.com> wrote:
>
>
>
> On 4/10/20 11:39 PM, Ard Biesheuvel wrote:
> > On Sat, 11 Apr 2020 at 00:12, Victor Erminpour
> > <victor.erminpour@oracle.com> wrote:
> >>
> >>
> >>
> >> On 4/10/20 1:09 AM, Ard Biesheuvel wrote:
> >>> On Thu, 9 Apr 2020 at 23:44, Victor Erminpour
> >>> <victor.erminpour@oracle.com> wrote:
> >>>>
> >>>> Enable the __efistub_global define to place variables in the
> >>>> .data section for both CONFIG_ARM and CONFIG_ARM64.
> >>>>
> >>>> This places the EFIstub sys_table variable and other EFIstub
> >>>> static variables in the .data section for both CONFIG_ARM and
> >>>> CONFIG_ARM64.
> >>>>
> >>>
> >>> What does that achieve?
> >>
> >> Hi Ard,
> >>
> >> Without placing these global variables in .data, I get the
> >> following errors when booting an ARM64 EFI system:
> >>
> >> EFI stub: ERROR: Exit boot services failed.
> >> EFI stub: ERROR: Failed to update FDT and exit boot services
> >>
> >
> > Which boot loader are you using? Does this involve shim?
> >
>
> grub2-efi-aa64-2.02-0.80.0.4.el7.aarch64
> shim-aa64-15-1.0.4.el7.aarch64
>

Does your system give access to the UEFI shell? If so, could you
please try booting the [uncompressed] Image file from the command
prompt? Which hardware are you booting?

> > Also, does it help if you add 'efi=no_disable_early_pci_dma'?
> >
>
> Tried this boot time option, but to no effect.
>
> >
> >>
> >> I know that the ARM64 linker script is supposed to put the
> >> .init.bss into the .init.data section, but I don't think this
> >> is happening for all systems.
> >>
> >> Having it explicitly enabled for CONFIG_ARM64 worked for me.
> >>
> >
> > OK, thanks for the report. However, we will be removing
> > __efistub_global entirely during the next cycle, so this is not the
> > right fix.
>
> Thanks Ard, this sounds promising!
>

Quite the opposite, unfortunately. It means the feature you are using
to fix your boot issue is going away.

This really looks like a bootloader issue to me, so let's narrow this
down first. If there is a real kernel issue here that needs
__efistub_global to be fixed, we obviously won't be removing it.