Re: [PATCH efi-next 2/3] efi/arm64: clean EFI stub exit code from cache instead of avoiding it

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Feb 28, 2020 at 11:14:50AM +0000, Mark Rutland wrote:
> On Fri, Feb 28, 2020 at 11:02:43AM +0100, Ard Biesheuvel wrote:
> > Commit 9f9223778 ("efi/libstub/arm: Make efi_entry() an ordinary PE/COFF
> > entrypoint") modified the handover code written in assembler, and for
> > maintainability, aligned the logic with the logic used in the 32-bit ARM
> > version, which is to avoid cache maintenance on the remaining instructions
> > in the subroutine that will be executed with the MMU and caches off, and
> > instead, branch into the relocated copy of the kernel image.
> > 
> > However, this assumes that this copy is executable, and this means we
> > expect EFI_LOADER_DATA regions to be executable as well, which is not
> > a reasonable assumption to make, even if this is true for most UEFI
> > implementations today.
> > 
> > So change this back, and add a __flush_dcache_area() call to cover the
> > remaining code in the subroutine.
> > 
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/arm64/kernel/efi-entry.S  | 18 +++++++++---------
> >  arch/arm64/kernel/image-vars.h |  2 +-
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> > index 4cfd03c35c49..d5dee064975f 100644
> > --- a/arch/arm64/kernel/efi-entry.S
> > +++ b/arch/arm64/kernel/efi-entry.S
> > @@ -19,7 +19,8 @@ ENTRY(efi_enter_kernel)
> >  	 * point stored in x0. Save those values in registers which are
> >  	 * callee preserved.
> >  	 */
> > -	mov	x19, x0			// relocated Image address
> > +	ldr	w2, =stext_offset
> > +	add	x19, x0, x2		// relocated Image entrypoint
> >  	mov	x20, x1			// DTB address
> >  
> >  	/*
> > @@ -29,15 +30,14 @@ ENTRY(efi_enter_kernel)
> >  	ldr	w1, =kernel_size
> >  	bl	__flush_dcache_area
> >  	ic	ialluis
> > -	dsb	sy
> >  
> >  	/*
> > -	 * Jump across, into the copy of the image that we just cleaned
> > -	 * to the PoC, so that we can safely disable the MMU and caches.
> > +	 * Flush the remainder of this routine to the PoC
> 
> Minor nit, but could we please say 'Clean' rather than 'Flush' here?
> 
> Even better, we now have __clean_dcache_area_poc(), and can use that
> too.

... or if that's better as a subsequent cleanup for consistency, that'd
also be fine, and needn't block this patch.

Thanks,
Mark.