Received-SPF: pass (google.com: domain of linux-kernel+bounces-93639-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240306085622.87248-1-cuiyunhui@bytedance.com> <20240306085622.87248-3-cuiyunhui@bytedance.com>
In-Reply-To: <20240306085622.87248-3-cuiyunhui@bytedance.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Wed, 6 Mar 2024 10:36:05 +0100
Message-ID: <CAMj1kXEjjFAeVAVwiDO22RJECSM=L=0q6J=zog7JR38rUZpLGQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] efistub: fix missed the initialization of gp
To: Yunhui Cui <cuiyunhui@bytedance.com>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com, aou@eecs.berkeley.edu, 
	xuzhipeng.1973@bytedance.com, alexghiti@rivosinc.com, samitolvanen@google.com, 
	bp@alien8.de, xiao.w.wang@intel.com, jan.kiszka@siemens.com, 
	kirill.shutemov@linux.intel.com, nathan@kernel.org, 
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, 
	linux-efi@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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?