diff mbox series

[3/3] efistub: fix missed the initialization of gp

Message ID 20240306085622.87248-3-cuiyunhui@bytedance.com
State New
Headers show
Series [1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" | expand

Commit Message

Yunhui Cui March 6, 2024, 8:56 a.m. UTC
Compared with gcc version 12, gcc version 13 uses the gp
register for compilation optimization, but the efistub module
does not initialize gp.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
---
 arch/riscv/kernel/efi-header.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Ard Biesheuvel March 6, 2024, 9:36 a.m. UTC | #1
On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> Compared with gcc version 12, gcc version 13 uses the gp
> register for compilation optimization, but the efistub module
> does not initialize gp.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>

This needs a sign-off, and your signoff needs to come after.

> ---
>  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> index 515b2dfbca75..fa17c08c092a 100644
> --- a/arch/riscv/kernel/efi-header.S
> +++ b/arch/riscv/kernel/efi-header.S
> @@ -40,7 +40,7 @@ optional_header:
>         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
>  #endif
>         .long   0                                       // SizeOfUninitializedData
> -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> +       .long   _efistub_entry - _start         // AddressOfEntryPoint
>         .long   efi_header_end - _start                 // BaseOfCode
>  #ifdef CONFIG_32BIT
>         .long  __pecoff_text_end - _start               // BaseOfData
> @@ -121,4 +121,13 @@ section_table:
>
>         .balign 0x1000
>  efi_header_end:
> +
> +       .global _efistub_entry
> +_efistub_entry:

This should go into .text or .init.text, not the header.

> +       /* Reload the global pointer */
> +       load_global_pointer
> +

What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
stub Makefile removes the SCS CFLAGS, so the stub will be built
without shadow call stack support, which I guess means that it might
use GP as a global pointer as usual?

> +       call __efistub_efi_pe_entry
> +       ret
> +

You are returning to the firmware here, but after modifying the GP
register. Shouldn't you restore it to its old value?
Yunhui Cui March 6, 2024, 12:34 p.m. UTC | #2
Hi Ard,

On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >
> > Compared with gcc version 12, gcc version 13 uses the gp
> > register for compilation optimization, but the efistub module
> > does not initialize gp.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
>
> This needs a sign-off, and your signoff needs to come after.
>
> > ---
> >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > index 515b2dfbca75..fa17c08c092a 100644
> > --- a/arch/riscv/kernel/efi-header.S
> > +++ b/arch/riscv/kernel/efi-header.S
> > @@ -40,7 +40,7 @@ optional_header:
> >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> >  #endif
> >         .long   0                                       // SizeOfUninitializedData
> > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> >         .long   efi_header_end - _start                 // BaseOfCode
> >  #ifdef CONFIG_32BIT
> >         .long  __pecoff_text_end - _start               // BaseOfData
> > @@ -121,4 +121,13 @@ section_table:
> >
> >         .balign 0x1000
> >  efi_header_end:
> > +
> > +       .global _efistub_entry
> > +_efistub_entry:
>
> This should go into .text or .init.text, not the header.
>
> > +       /* Reload the global pointer */
> > +       load_global_pointer
> > +
>
> What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> stub Makefile removes the SCS CFLAGS, so the stub will be built
> without shadow call stack support, which I guess means that it might
> use GP as a global pointer as usual?
>
> > +       call __efistub_efi_pe_entry
> > +       ret
> > +
>
> You are returning to the firmware here, but after modifying the GP
> register. Shouldn't you restore it to its old value?
There is no need to restore the value of the gp register. Where gp is
needed, the gp register must first be initialized. And here is the
entry.

Regarding your first two comments above, I plan to make the following
changes in v2,
efi_header_end:
+
+       __INIT
+       .global _efistub_entry
+_efistub_entry:
+       /* Reload the global pointer */
+.option push
+.option norelax
+       la gp, __global_pointer$
+.option pop
+
+       call __efistub_efi_pe_entry
+       ret
+       __HEAD
+
        .endm

what do you think?

Thanks,
Yunhui
Ard Biesheuvel March 6, 2024, 1:02 p.m. UTC | #3
On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Compared with gcc version 12, gcc version 13 uses the gp
> > > register for compilation optimization, but the efistub module
> > > does not initialize gp.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> >
> > This needs a sign-off, and your signoff needs to come after.
> >
> > > ---
> > >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > index 515b2dfbca75..fa17c08c092a 100644
> > > --- a/arch/riscv/kernel/efi-header.S
> > > +++ b/arch/riscv/kernel/efi-header.S
> > > @@ -40,7 +40,7 @@ optional_header:
> > >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> > >  #endif
> > >         .long   0                                       // SizeOfUninitializedData
> > > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> > >         .long   efi_header_end - _start                 // BaseOfCode
> > >  #ifdef CONFIG_32BIT
> > >         .long  __pecoff_text_end - _start               // BaseOfData
> > > @@ -121,4 +121,13 @@ section_table:
> > >
> > >         .balign 0x1000
> > >  efi_header_end:
> > > +
> > > +       .global _efistub_entry
> > > +_efistub_entry:
> >
> > This should go into .text or .init.text, not the header.
> >
> > > +       /* Reload the global pointer */
> > > +       load_global_pointer
> > > +
> >
> > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > without shadow call stack support, which I guess means that it might
> > use GP as a global pointer as usual?
> >
> > > +       call __efistub_efi_pe_entry
> > > +       ret
> > > +
> >
> > You are returning to the firmware here, but after modifying the GP
> > register. Shouldn't you restore it to its old value?
> There is no need to restore the value of the gp register. Where gp is
> needed, the gp register must first be initialized. And here is the
> entry.
>

But how should the firmware know that GP was corrupted after calling
the kernel's EFI entrypoint? The EFI stub can return to the firmware
if it encounters any errors while still running in the EFI boot
services.


> Regarding your first two comments above, I plan to make the following
> changes in v2,
> efi_header_end:
> +
> +       __INIT
> +       .global _efistub_entry
> +_efistub_entry:
> +       /* Reload the global pointer */
> +.option push
> +.option norelax
> +       la gp, __global_pointer$
> +.option pop
> +
> +       call __efistub_efi_pe_entry
> +       ret
> +       __HEAD
> +
>         .endm
>
> what do you think?
>

This looks ok to me, but I would still like to understand why it is ok
to return to the firmware with a modified GP value.
Ard Biesheuvel March 6, 2024, 1:09 p.m. UTC | #4
On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Ard,
> >
> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >
> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > register for compilation optimization, but the efistub module
> > > > does not initialize gp.
> > > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > >
> > > This needs a sign-off, and your signoff needs to come after.
> > >
> > > > ---
> > > >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > @@ -40,7 +40,7 @@ optional_header:
> > > >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> > > >  #endif
> > > >         .long   0                                       // SizeOfUninitializedData
> > > > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > > > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> > > >         .long   efi_header_end - _start                 // BaseOfCode
> > > >  #ifdef CONFIG_32BIT
> > > >         .long  __pecoff_text_end - _start               // BaseOfData
> > > > @@ -121,4 +121,13 @@ section_table:
> > > >
> > > >         .balign 0x1000
> > > >  efi_header_end:
> > > > +
> > > > +       .global _efistub_entry
> > > > +_efistub_entry:
> > >
> > > This should go into .text or .init.text, not the header.
> > >
> > > > +       /* Reload the global pointer */
> > > > +       load_global_pointer
> > > > +
> > >
> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > without shadow call stack support, which I guess means that it might
> > > use GP as a global pointer as usual?
> > >
> > > > +       call __efistub_efi_pe_entry
> > > > +       ret
> > > > +
> > >
> > > You are returning to the firmware here, but after modifying the GP
> > > register. Shouldn't you restore it to its old value?
> > There is no need to restore the value of the gp register. Where gp is
> > needed, the gp register must first be initialized. And here is the
> > entry.
> >
>
> But how should the firmware know that GP was corrupted after calling
> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> if it encounters any errors while still running in the EFI boot
> services.
>

Actually, I wonder if GP can be modified at all before
ExitBootServices(). The EFI timer interrupt is still live at this
point, and so the firmware is being called behind your back, and might
rely on GP retaining its original value.
Yunhui Cui March 6, 2024, 1:30 p.m. UTC | #5
Hi Ard,

On Wed, Mar 6, 2024 at 9:09 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >
> > > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > > register for compilation optimization, but the efistub module
> > > > > does not initialize gp.
> > > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >
> > > > This needs a sign-off, and your signoff needs to come after.
> > > >
> > > > > ---
> > > > >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > > --- a/arch/riscv/kernel/efi-header.S
> > > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > > @@ -40,7 +40,7 @@ optional_header:
> > > > >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> > > > >  #endif
> > > > >         .long   0                                       // SizeOfUninitializedData
> > > > > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > > > > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> > > > >         .long   efi_header_end - _start                 // BaseOfCode
> > > > >  #ifdef CONFIG_32BIT
> > > > >         .long  __pecoff_text_end - _start               // BaseOfData
> > > > > @@ -121,4 +121,13 @@ section_table:
> > > > >
> > > > >         .balign 0x1000
> > > > >  efi_header_end:
> > > > > +
> > > > > +       .global _efistub_entry
> > > > > +_efistub_entry:
> > > >
> > > > This should go into .text or .init.text, not the header.
> > > >
> > > > > +       /* Reload the global pointer */
> > > > > +       load_global_pointer
> > > > > +
> > > >
> > > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > without shadow call stack support, which I guess means that it might
> > > > use GP as a global pointer as usual?
> > > >
> > > > > +       call __efistub_efi_pe_entry
> > > > > +       ret
> > > > > +
> > > >
> > > > You are returning to the firmware here, but after modifying the GP
> > > > register. Shouldn't you restore it to its old value?
> > > There is no need to restore the value of the gp register. Where gp is
> > > needed, the gp register must first be initialized. And here is the
> > > entry.
> > >
> >
> > But how should the firmware know that GP was corrupted after calling
> > the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > if it encounters any errors while still running in the EFI boot
> > services.
> >
>
> Actually, I wonder if GP can be modified at all before
> ExitBootServices(). The EFI timer interrupt is still live at this
> point, and so the firmware is being called behind your back, and might
> rely on GP retaining its original value.

OK, in v2 I will restore the value of gp as follows:

  efi_header_end:
+
+ __INIT
+ .global_efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ addi sp,sp,-8
+ sd gp,0(sp)
+ la gp, __global_pointer$
+.option pop
+ call __efistub_efi_pe_entry
+ld gp,0(sp)
+addi sp,sp,8
+ret
+ __HEAD
+
         .endm

what do you think?

Thanks,
Yunhui
Ard Biesheuvel March 7, 2024, 4:48 p.m. UTC | #6
On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > >
> > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >>
> > > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > >> >
> > > > >> > Hi Ard,
> > > > >> >
> > > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >> > >
> > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >> > > >
> > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > >> > > > register for compilation optimization, but the efistub module
> > > > >> > > > does not initialize gp.
> > > > >> > > >
> > > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > > >> > >
> > > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > > >> > >
> > > > >> > > > ---
> > > > >> > > >  arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > >> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >> > > >
> > > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > >> > > >         .long   __pecoff_data_virt_end - __pecoff_text_end      // SizeOfInitializedData
> > > > >> > > >  #endif
> > > > >> > > >         .long   0                                       // SizeOfUninitializedData
> > > > >> > > > -       .long   __efistub_efi_pe_entry - _start         // AddressOfEntryPoint
> > > > >> > > > +       .long   _efistub_entry - _start         // AddressOfEntryPoint
> > > > >> > > >         .long   efi_header_end - _start                 // BaseOfCode
> > > > >> > > >  #ifdef CONFIG_32BIT
> > > > >> > > >         .long  __pecoff_text_end - _start               // BaseOfData
> > > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > > >> > > >
> > > > >> > > >         .balign 0x1000
> > > > >> > > >  efi_header_end:
> > > > >> > > > +
> > > > >> > > > +       .global _efistub_entry
> > > > >> > > > +_efistub_entry:
> > > > >> > >
> > > > >> > > This should go into .text or .init.text, not the header.
> > > > >> > >
> > > > >> > > > +       /* Reload the global pointer */
> > > > >> > > > +       load_global_pointer
> > > > >> > > > +
> > > > >> > >
> > > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > >> > > without shadow call stack support, which I guess means that it might
> > > > >> > > use GP as a global pointer as usual?
> > > > >> > >
> > > > >> > > > +       call __efistub_efi_pe_entry
> > > > >> > > > +       ret
> > > > >> > > > +
> > > > >> > >
> > > > >> > > You are returning to the firmware here, but after modifying the GP
> > > > >> > > register. Shouldn't you restore it to its old value?
> > > > >> > There is no need to restore the value of the gp register. Where gp is
> > > > >> > needed, the gp register must first be initialized. And here is the
> > > > >> > entry.
> > > > >> >
> > > > >>
> > > > >> But how should the firmware know that GP was corrupted after calling
> > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > > >> if it encounters any errors while still running in the EFI boot
> > > > >> services.
> > > > >>
> > > > >
> > > > > Actually, I wonder if GP can be modified at all before
> > > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > > point, and so the firmware is being called behind your back, and might
> > > > > rely on GP retaining its original value.
> > > >
> > > > [A few of us are talking on IRC as I'm writing this...]
> > > >
> > > > The UEFI spec says "UEFI firmware must neither trust the
> > > > values of tp and gp nor make an assumption of owning the write access to
> > > > these register in any circumstances".  It's kind of vague what "UEFI
> > > > firmware" means here, but I think it's reasonable to assume that the
> > > > kernel (and thus the EFI stub) is not included there.
> > > >
> > > > So under that interpretation, the kernel (including the EFI stub) would
> > > > be allowed to overwrite GP with whatever it wants.
> > > >
> > >
> > > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > > applications such as the Linux EFI stub should be safe, I'd still like
> > > to understand why this change is necessary. The patches you are
> > > reverting are supposed to ensure that a) the compiler does not
> > > generate references that can be relaxed to GP based ones, and b) no
> > > R_RISCV_RELAX relocations are present in any of the code that runs in
> > > the context of the EFI firmware.
> > >
> > > Are you still seeing GP based symbol references? Is there C code that
> > > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > > (see list below). If any of those are implemented in C, they should
> > > not be used by the EFI stub directly unless they are guaranteed to be
> > > uninstrumented and callable at arbitrary offsets other than the one
> > > they were linked to run at.
> > >
> > >
> > > __efistub_memcmp         = memcmp;
> > > __efistub_memchr         = memchr;
> > > __efistub_memcpy         = memcpy;
> > > __efistub_memmove        = memmove;
> > > __efistub_memset         = memset;
> > > __efistub_strlen         = strlen;
> > > __efistub_strnlen        = strnlen;
> > > __efistub_strcmp         = strcmp;
> > > __efistub_strncmp        = strncmp;
> > > __efistub_strrchr        = strrchr;
> > > __efistub___memcpy       = memcpy;
> > > __efistub___memmove      = memmove;
> > > __efistub___memset       = memset;
> > > __efistub__start         = _start;
> > > __efistub__start_kernel  = _start_kernel;
> > >
> > > (from arch/riscv/kernel/image-vars.h)
> >
> > Uhm never mind - these are all gone now, I was looking at a v6.1
> > kernel source tree.
> >
> > So that means that, as far as I can tell, the only kernel C code that
> > executes in the context of the EFI firmware is built with -mno-relax
> > and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> > to see why these changes are needed.
> >
> > Yunhui, could you please explain the reason for this series?
>
> From the logic of binutils, if "__global_pointer$" exists, it is
> possible to use GP for optimization. For RISC-V, "__global_pointer$"
> was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> a whole, we should keep using GP uniformly.

There is no 'system as a whole' that can use GP 'uniformly'

The EFI stub is a separate executable that runs from a different
mapping of memory, in an execution context managed by the firmware. It
happens to be linked into the same executable as the vmlinux kernel.

> The root cause of this
> problem is that GP is not loaded, rather than "On RISC-V, we also
> avoid GP based relocations..." as commit "d2baf8cc82c17" said.

GP is not loaded because in the EFI firmware context, there is no safe
way to rely on it.

> We need
> to address problems head-on, rather than avoid them.
>

So what solution are you proposing for the potential GP conflicts
between the boot loader, the Linux EFI stub and the firmware?
diff mbox series

Patch

diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
index 515b2dfbca75..fa17c08c092a 100644
--- a/arch/riscv/kernel/efi-header.S
+++ b/arch/riscv/kernel/efi-header.S
@@ -40,7 +40,7 @@  optional_header:
 	.long	__pecoff_data_virt_end - __pecoff_text_end	// SizeOfInitializedData
 #endif
 	.long	0					// SizeOfUninitializedData
-	.long	__efistub_efi_pe_entry - _start		// AddressOfEntryPoint
+	.long	_efistub_entry - _start		// AddressOfEntryPoint
 	.long	efi_header_end - _start			// BaseOfCode
 #ifdef CONFIG_32BIT
 	.long  __pecoff_text_end - _start		// BaseOfData
@@ -121,4 +121,13 @@  section_table:
 
 	.balign	0x1000
 efi_header_end:
+
+	.global	_efistub_entry
+_efistub_entry:
+	/* Reload the global pointer */
+	load_global_pointer
+
+	call __efistub_efi_pe_entry
+	ret
+
 	.endm