| Message ID | 20221017171700.3736890-2-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | aaeb3fc614d65ec5c0b838ed1afb59c3f0f04643 |
| Headers | show |
| Series | efi: Combine stub functionality with zboot decompressor | expand |
On Mon, Oct 17, 2022 at 07:16:40PM +0200, Ard Biesheuvel wrote: > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > index 61a87fa1c305..1c1be004a271 100644 > --- a/arch/arm64/kernel/efi-entry.S > +++ b/arch/arm64/kernel/efi-entry.S > @@ -23,15 +23,6 @@ SYM_CODE_START(efi_enter_kernel) > add x19, x0, x2 // relocated Image entrypoint > mov x20, x1 // DTB address > > - /* > - * Clean the copied Image to the PoC, and ensure it is not shadowed by > - * stale icache entries from before relocation. > - */ > - ldr w1, =kernel_size > - add x1, x0, x1 > - bl dcache_clean_poc > - ic ialluis > - > /* > * Clean the remainder of this routine to the PoC > * so that we can safely disable the MMU and caches. [...] > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c > index 598c76c4bbaa..e767a5ac8c3d 100644 > --- a/drivers/firmware/efi/libstub/arm64-stub.c > +++ b/drivers/firmware/efi/libstub/arm64-stub.c [...] > @@ -174,5 +174,13 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, > *image_addr = *reserve_addr; > memcpy((void *)*image_addr, _text, kernel_size); > > +clean_image_to_poc: > + /* > + * Clean the copied Image to the PoC, and ensure it is not shadowed by > + * stale icache entries from before relocation. > + */ > + dcache_clean_poc(*image_addr, *image_addr + kernel_size); > + asm("ic ialluis"); Does this need some barriers, at least a DSB? The original code had DSB and ISB, though not immediately after the IC instruction.
On Tue, 18 Oct 2022 at 13:28, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Oct 17, 2022 at 07:16:40PM +0200, Ard Biesheuvel wrote: > > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > > index 61a87fa1c305..1c1be004a271 100644 > > --- a/arch/arm64/kernel/efi-entry.S > > +++ b/arch/arm64/kernel/efi-entry.S > > @@ -23,15 +23,6 @@ SYM_CODE_START(efi_enter_kernel) > > add x19, x0, x2 // relocated Image entrypoint > > mov x20, x1 // DTB address > > > > - /* > > - * Clean the copied Image to the PoC, and ensure it is not shadowed by > > - * stale icache entries from before relocation. > > - */ > > - ldr w1, =kernel_size > > - add x1, x0, x1 > > - bl dcache_clean_poc > > - ic ialluis > > - > > /* > > * Clean the remainder of this routine to the PoC > > * so that we can safely disable the MMU and caches. > [...] > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c > > index 598c76c4bbaa..e767a5ac8c3d 100644 > > --- a/drivers/firmware/efi/libstub/arm64-stub.c > > +++ b/drivers/firmware/efi/libstub/arm64-stub.c > [...] > > @@ -174,5 +174,13 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, > > *image_addr = *reserve_addr; > > memcpy((void *)*image_addr, _text, kernel_size); > > > > +clean_image_to_poc: > > + /* > > + * Clean the copied Image to the PoC, and ensure it is not shadowed by > > + * stale icache entries from before relocation. > > + */ > > + dcache_clean_poc(*image_addr, *image_addr + kernel_size); > > + asm("ic ialluis"); > > Does this need some barriers, at least a DSB? The original code had DSB > and ISB, though not immediately after the IC instruction. > We are still relying on the implicit DSB done by the subsequent call to dcache_clean_to_poc() call in efi_enter_kernel(), which executes much later than this code.
On Tue, Oct 18, 2022 at 01:38:57PM +0200, Ard Biesheuvel wrote: > On Tue, 18 Oct 2022 at 13:28, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Oct 17, 2022 at 07:16:40PM +0200, Ard Biesheuvel wrote: > > > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > > > index 61a87fa1c305..1c1be004a271 100644 > > > --- a/arch/arm64/kernel/efi-entry.S > > > +++ b/arch/arm64/kernel/efi-entry.S > > > @@ -23,15 +23,6 @@ SYM_CODE_START(efi_enter_kernel) > > > add x19, x0, x2 // relocated Image entrypoint > > > mov x20, x1 // DTB address > > > > > > - /* > > > - * Clean the copied Image to the PoC, and ensure it is not shadowed by > > > - * stale icache entries from before relocation. > > > - */ > > > - ldr w1, =kernel_size > > > - add x1, x0, x1 > > > - bl dcache_clean_poc > > > - ic ialluis > > > - > > > /* > > > * Clean the remainder of this routine to the PoC > > > * so that we can safely disable the MMU and caches. > > [...] > > > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c > > > index 598c76c4bbaa..e767a5ac8c3d 100644 > > > --- a/drivers/firmware/efi/libstub/arm64-stub.c > > > +++ b/drivers/firmware/efi/libstub/arm64-stub.c > > [...] > > > @@ -174,5 +174,13 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, > > > *image_addr = *reserve_addr; > > > memcpy((void *)*image_addr, _text, kernel_size); > > > > > > +clean_image_to_poc: > > > + /* > > > + * Clean the copied Image to the PoC, and ensure it is not shadowed by > > > + * stale icache entries from before relocation. > > > + */ > > > + dcache_clean_poc(*image_addr, *image_addr + kernel_size); > > > + asm("ic ialluis"); > > > > Does this need some barriers, at least a DSB? The original code had DSB > > and ISB, though not immediately after the IC instruction. > > We are still relying on the implicit DSB done by the subsequent call > to dcache_clean_to_poc() call in efi_enter_kernel(), which executes > much later than this code. Ah, ok. Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 61a87fa1c305..1c1be004a271 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -23,15 +23,6 @@ SYM_CODE_START(efi_enter_kernel) add x19, x0, x2 // relocated Image entrypoint mov x20, x1 // DTB address - /* - * Clean the copied Image to the PoC, and ensure it is not shadowed by - * stale icache entries from before relocation. - */ - ldr w1, =kernel_size - add x1, x0, x1 - bl dcache_clean_poc - ic ialluis - /* * Clean the remainder of this routine to the PoC * so that we can safely disable the MMU and caches. diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h index 8151412653de..74d20835cf91 100644 --- a/arch/arm64/kernel/image-vars.h +++ b/arch/arm64/kernel/image-vars.h @@ -10,7 +10,6 @@ #error This file should only be included in vmlinux.lds.S #endif -PROVIDE(__efistub_kernel_size = _edata - _text); PROVIDE(__efistub_primary_entry_offset = primary_entry - _text); /* diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c index 598c76c4bbaa..e767a5ac8c3d 100644 --- a/drivers/firmware/efi/libstub/arm64-stub.c +++ b/drivers/firmware/efi/libstub/arm64-stub.c @@ -157,7 +157,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, */ *image_addr = (u64)_text; *reserve_size = 0; - return EFI_SUCCESS; + goto clean_image_to_poc; } status = efi_allocate_pages_aligned(*reserve_size, reserve_addr, @@ -174,5 +174,13 @@ efi_status_t handle_kernel_image(unsigned long *image_addr, *image_addr = *reserve_addr; memcpy((void *)*image_addr, _text, kernel_size); +clean_image_to_poc: + /* + * Clean the copied Image to the PoC, and ensure it is not shadowed by + * stale icache entries from before relocation. + */ + dcache_clean_poc(*image_addr, *image_addr + kernel_size); + asm("ic ialluis"); + return EFI_SUCCESS; }
The efi_enter_kernel() routine will be shared between the existing EFI stub and the zboot decompressor, and the version of dcache_clean_to_poc() that the core kernel exports to the stub will not be available in the latter case. So move the handling into the .c file which will remain part of the stub build that integrates directly with the kernel proper. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/efi-entry.S | 9 --------- arch/arm64/kernel/image-vars.h | 1 - drivers/firmware/efi/libstub/arm64-stub.c | 10 +++++++++- 3 files changed, 9 insertions(+), 11 deletions(-)