[PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[Ard Biesheuvel]

In some cases, we expose the kernel's struct screen_info to the EFI stub
directly, so it gets populated before even entering the kernel.  This
means the early console is available as soon as the early param parsing
happens, which is nice. It also means we need two different ways to pass
this information, as this trick only works if the EFI stub is baked into
the core kernel image, which is not always the case.

Huacai reports that the preparatory refactoring that was needed to
implement this alternative method for zboot resulted in a non-functional
efifb earlycon for other cases as well, due to the reordering of the
kernel image relocation with the population of the screen_info struct,
and the latter now takes place after copying the image to its new
location, which means we copy the old, uninitialized state.

So let's ensure that alloc_screen_info() produces the correct
screen_info pointer, by keeping its relocated address in a global
variable.

Cc: loongarch@lists.linux.dev
Cc: Xuefeng Li <lixuefeng@loongson.cn>
Cc: Xuerui Wang <kernel@xen0n.name>
Cc: loongson-kernel@lists.loongnix.cn
Reported-by: Huacai Chen <chenhuacai@loongson.cn>
Link: https://lore.kernel.org/linux-efi/20230310021749.921041-1-chenhuacai@loongson.cn/
Fixes: 42c8ea3dca094ab8 ("efi: libstub: Factor out EFI stub entrypoint into separate file")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub-entry.c | 14 ++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c       |  9 ---------
 drivers/firmware/efi/libstub/screen_info.c    |  7 -------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-entry.c b/drivers/firmware/efi/libstub/efi-stub-entry.c
index 5245c4f031c0a70a..883ef2c88f522c4b 100644
--- a/drivers/firmware/efi/libstub/efi-stub-entry.c
+++ b/drivers/firmware/efi/libstub/efi-stub-entry.c
@@ -5,6 +5,17 @@
 
 #include "efistub.h"
 
+static struct screen_info *si;
+
+struct screen_info *alloc_screen_info(void)
+{
+	return si;
+}
+
+void free_screen_info(struct screen_info *si)
+{
+}
+
 /*
  * EFI entry point for the generic EFI stub used by ARM, arm64, RISC-V and
  * LoongArch. This is the entrypoint that is described in the PE/COFF header
@@ -56,6 +67,9 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		return status;
 	}
 
+	/* point si to the relocated copy of struct screen_info */
+	si = (void *)&screen_info + image_addr - (unsigned long)image->image_base;
+
 	status = efi_stub_common(handle, image, image_addr, cmdline_ptr);
 
 	efi_free(image_size, image_addr);
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 2955c1ac6a36ee00..c4b9eccad0f103dc 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -47,15 +47,6 @@
 static u64 virtmap_base = EFI_RT_VIRTUAL_BASE;
 static bool flat_va_mapping = (EFI_RT_VIRTUAL_OFFSET != 0);
 
-struct screen_info * __weak alloc_screen_info(void)
-{
-	return &screen_info;
-}
-
-void __weak free_screen_info(struct screen_info *si)
-{
-}
-
 static struct screen_info *setup_graphics(void)
 {
 	efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
diff --git a/drivers/firmware/efi/libstub/screen_info.c b/drivers/firmware/efi/libstub/screen_info.c
index 8e76a8b384ba142d..b2637420fe53911e 100644
--- a/drivers/firmware/efi/libstub/screen_info.c
+++ b/drivers/firmware/efi/libstub/screen_info.c
@@ -15,14 +15,7 @@
  * early, but it only works if the EFI stub is part of the core kernel image
  * itself. The zboot decompressor can only use the configuration table
  * approach.
- *
- * In order to support both methods from the same build of the EFI stub
- * library, provide this dummy global definition of struct screen_info. If it
- * is required to satisfy a link dependency, it means we need to override the
- * __weak alloc and free methods with the ones below, and those will be pulled
- * in as well.
  */
-struct screen_info screen_info;
 
 static efi_guid_t screen_info_guid = LINUX_EFI_SCREEN_INFO_TABLE_GUID;
 
-- 
2.39.2

Re: [PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[Ard Biesheuvel]

On Mon, 13 Mar 2023 at 09:14, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> In some cases, we expose the kernel's struct screen_info to the EFI stub
> directly, so it gets populated before even entering the kernel.  This
> means the early console is available as soon as the early param parsing
> happens, which is nice. It also means we need two different ways to pass
> this information, as this trick only works if the EFI stub is baked into
> the core kernel image, which is not always the case.
>
> Huacai reports that the preparatory refactoring that was needed to
> implement this alternative method for zboot resulted in a non-functional
> efifb earlycon for other cases as well, due to the reordering of the
> kernel image relocation with the population of the screen_info struct,
> and the latter now takes place after copying the image to its new
> location, which means we copy the old, uninitialized state.
>
> So let's ensure that alloc_screen_info() produces the correct
> screen_info pointer, by keeping its relocated address in a global
> variable.
>
> Cc: loongarch@lists.linux.dev
> Cc: Xuefeng Li <lixuefeng@loongson.cn>
> Cc: Xuerui Wang <kernel@xen0n.name>
> Cc: loongson-kernel@lists.loongnix.cn
> Reported-by: Huacai Chen <chenhuacai@loongson.cn>
> Link: https://lore.kernel.org/linux-efi/20230310021749.921041-1-chenhuacai@loongson.cn/
> Fixes: 42c8ea3dca094ab8 ("efi: libstub: Factor out EFI stub entrypoint into separate file")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

This is still not working :-(

> ---
>  drivers/firmware/efi/libstub/efi-stub-entry.c | 14 ++++++++++++++
>  drivers/firmware/efi/libstub/efi-stub.c       |  9 ---------
>  drivers/firmware/efi/libstub/screen_info.c    |  7 -------
>  3 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-entry.c b/drivers/firmware/efi/libstub/efi-stub-entry.c
> index 5245c4f031c0a70a..883ef2c88f522c4b 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-entry.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-entry.c
> @@ -5,6 +5,17 @@
>
>  #include "efistub.h"
>
> +static struct screen_info *si;
> +
> +struct screen_info *alloc_screen_info(void)
> +{
> +       return si;
> +}
> +
> +void free_screen_info(struct screen_info *si)
> +{
> +}
> +
>  /*
>   * EFI entry point for the generic EFI stub used by ARM, arm64, RISC-V and
>   * LoongArch. This is the entrypoint that is described in the PE/COFF header
> @@ -56,6 +67,9 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>                 return status;
>         }
>
> +       /* point si to the relocated copy of struct screen_info */
> +       si = (void *)&screen_info + image_addr - (unsigned long)image->image_base;
> +
>         status = efi_stub_common(handle, image, image_addr, cmdline_ptr);
>
>         efi_free(image_size, image_addr);
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> index 2955c1ac6a36ee00..c4b9eccad0f103dc 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -47,15 +47,6 @@
>  static u64 virtmap_base = EFI_RT_VIRTUAL_BASE;
>  static bool flat_va_mapping = (EFI_RT_VIRTUAL_OFFSET != 0);
>
> -struct screen_info * __weak alloc_screen_info(void)
> -{
> -       return &screen_info;
> -}
> -
> -void __weak free_screen_info(struct screen_info *si)
> -{
> -}
> -
>  static struct screen_info *setup_graphics(void)
>  {
>         efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
> diff --git a/drivers/firmware/efi/libstub/screen_info.c b/drivers/firmware/efi/libstub/screen_info.c
> index 8e76a8b384ba142d..b2637420fe53911e 100644
> --- a/drivers/firmware/efi/libstub/screen_info.c
> +++ b/drivers/firmware/efi/libstub/screen_info.c
> @@ -15,14 +15,7 @@
>   * early, but it only works if the EFI stub is part of the core kernel image
>   * itself. The zboot decompressor can only use the configuration table
>   * approach.
> - *
> - * In order to support both methods from the same build of the EFI stub
> - * library, provide this dummy global definition of struct screen_info. If it
> - * is required to satisfy a link dependency, it means we need to override the
> - * __weak alloc and free methods with the ones below, and those will be pulled
> - * in as well.
>   */
> -struct screen_info screen_info;
>
>  static efi_guid_t screen_info_guid = LINUX_EFI_SCREEN_INFO_TABLE_GUID;
>
> --
> 2.39.2
>

Re: [PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[Ard Biesheuvel]

On Mon, 13 Mar 2023 at 09:48, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 13 Mar 2023 at 09:14, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > In some cases, we expose the kernel's struct screen_info to the EFI stub
> > directly, so it gets populated before even entering the kernel.  This
> > means the early console is available as soon as the early param parsing
> > happens, which is nice. It also means we need two different ways to pass
> > this information, as this trick only works if the EFI stub is baked into
> > the core kernel image, which is not always the case.
> >
> > Huacai reports that the preparatory refactoring that was needed to
> > implement this alternative method for zboot resulted in a non-functional
> > efifb earlycon for other cases as well, due to the reordering of the
> > kernel image relocation with the population of the screen_info struct,
> > and the latter now takes place after copying the image to its new
> > location, which means we copy the old, uninitialized state.
> >
> > So let's ensure that alloc_screen_info() produces the correct
> > screen_info pointer, by keeping its relocated address in a global
> > variable.
> >
> > Cc: loongarch@lists.linux.dev
> > Cc: Xuefeng Li <lixuefeng@loongson.cn>
> > Cc: Xuerui Wang <kernel@xen0n.name>
> > Cc: loongson-kernel@lists.loongnix.cn
> > Reported-by: Huacai Chen <chenhuacai@loongson.cn>
> > Link: https://lore.kernel.org/linux-efi/20230310021749.921041-1-chenhuacai@loongson.cn/
> > Fixes: 42c8ea3dca094ab8 ("efi: libstub: Factor out EFI stub entrypoint into separate file")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> This is still not working :-(
>

Huacai, could you try this patch please? It is working now - I don't
know what I did wrong before when testing it.

Re: [PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[Huacai Chen]

Hi, Ard,

On Mon, Mar 13, 2023 at 6:00 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 13 Mar 2023 at 09:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 13 Mar 2023 at 09:14, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > In some cases, we expose the kernel's struct screen_info to the EFI stub
> > > directly, so it gets populated before even entering the kernel.  This
> > > means the early console is available as soon as the early param parsing
> > > happens, which is nice. It also means we need two different ways to pass
> > > this information, as this trick only works if the EFI stub is baked into
> > > the core kernel image, which is not always the case.
> > >
> > > Huacai reports that the preparatory refactoring that was needed to
> > > implement this alternative method for zboot resulted in a non-functional
> > > efifb earlycon for other cases as well, due to the reordering of the
> > > kernel image relocation with the population of the screen_info struct,
> > > and the latter now takes place after copying the image to its new
> > > location, which means we copy the old, uninitialized state.
> > >
> > > So let's ensure that alloc_screen_info() produces the correct
> > > screen_info pointer, by keeping its relocated address in a global
> > > variable.
> > >
> > > Cc: loongarch@lists.linux.dev
> > > Cc: Xuefeng Li <lixuefeng@loongson.cn>
> > > Cc: Xuerui Wang <kernel@xen0n.name>
> > > Cc: loongson-kernel@lists.loongnix.cn
> > > Reported-by: Huacai Chen <chenhuacai@loongson.cn>
> > > Link: https://lore.kernel.org/linux-efi/20230310021749.921041-1-chenhuacai@loongson.cn/
> > > Fixes: 42c8ea3dca094ab8 ("efi: libstub: Factor out EFI stub entrypoint into separate file")
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > This is still not working :-(
> >
>
> Huacai, could you try this patch please? It is working now - I don't
> know what I did wrong before when testing it.
I've tested, it works for me.

Tested-by: Huacai Chen <chenhuacai@loongson.cn>

But I think my original patch is also OK, unless screen_info is needed
by zboot (but the earliest zboot doesn't need it).

Huacai
>

Re: [PATCH] efi: libstub: Use relocated version of kernel's struct screen_info

[Ard Biesheuvel]

On Mon, 13 Mar 2023 at 14:04, Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Ard,
>
> On Mon, Mar 13, 2023 at 6:00 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 13 Mar 2023 at 09:48, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 13 Mar 2023 at 09:14, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > In some cases, we expose the kernel's struct screen_info to the EFI stub
> > > > directly, so it gets populated before even entering the kernel.  This
> > > > means the early console is available as soon as the early param parsing
> > > > happens, which is nice. It also means we need two different ways to pass
> > > > this information, as this trick only works if the EFI stub is baked into
> > > > the core kernel image, which is not always the case.
> > > >
> > > > Huacai reports that the preparatory refactoring that was needed to
> > > > implement this alternative method for zboot resulted in a non-functional
> > > > efifb earlycon for other cases as well, due to the reordering of the
> > > > kernel image relocation with the population of the screen_info struct,
> > > > and the latter now takes place after copying the image to its new
> > > > location, which means we copy the old, uninitialized state.
> > > >
> > > > So let's ensure that alloc_screen_info() produces the correct
> > > > screen_info pointer, by keeping its relocated address in a global
> > > > variable.
> > > >
> > > > Cc: loongarch@lists.linux.dev
> > > > Cc: Xuefeng Li <lixuefeng@loongson.cn>
> > > > Cc: Xuerui Wang <kernel@xen0n.name>
> > > > Cc: loongson-kernel@lists.loongnix.cn
> > > > Reported-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > Link: https://lore.kernel.org/linux-efi/20230310021749.921041-1-chenhuacai@loongson.cn/
> > > > Fixes: 42c8ea3dca094ab8 ("efi: libstub: Factor out EFI stub entrypoint into separate file")
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > This is still not working :-(
> > >
> >
> > Huacai, could you try this patch please? It is working now - I don't
> > know what I did wrong before when testing it.
> I've tested, it works for me.
>
> Tested-by: Huacai Chen <chenhuacai@loongson.cn>
>
> But I think my original patch is also OK, unless screen_info is needed
> by zboot (but the earliest zboot doesn't need it).
>

Thanks.

zboot does need it, otherwise the EFI framebuffer does not work at all.