From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 390EEC531DF for ; Tue, 20 Aug 2024 11:44:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=m4vVRFcvDiiStlYxLqaRl7FWEixRdek6DukpClrB7wU=; b=sQjn0l40VDmMB6Q+xdEUUokxy5 c+UkaUhzbtjj7FM2x84SrIYA7a/VHL4xBVECvLdoqmiX7HlaCdNPyKP2zl8EUxSFSkF2VXLclYyGu /qftTflH5PagcRFyaRxYzLvhtPzPTw9osoO6BDJhJnz2uLvuGGYRP3h0BhodxXmkTvdSgJKyFnpHC GffF3sMDbMudF8TDjCkFwF2VVXJJTM1E0C/heaq/Da7PMc+v+VWHFiCvr7Rn4QD12DD4qQ2BsYFHp lUreTG2IkM4GcC9Ksz9y3VEqE+KcYedkLud7vPUmssKpGug1oUNOzuUTJfSmsQ+CNtbRXKJAmkY97 FNLrDVhQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgNHg-000000053UY-20jb; Tue, 20 Aug 2024 11:44:20 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sgNGk-000000053GV-1ewW for linux-arm-kernel@lists.infradead.org; Tue, 20 Aug 2024 11:43:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DA91F1007; Tue, 20 Aug 2024 04:43:47 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EBAE73F7BE; Tue, 20 Aug 2024 04:43:20 -0700 (PDT) Date: Tue, 20 Aug 2024 12:43:18 +0100 From: Mark Rutland To: Andre Przywara Cc: linux-arm-kernel@lists.infradead.org, akos.denke@arm.com, luca.fancellu@arm.com, maz@kernel.org Subject: Re: [BOOT-WRAPPER v2 06/10] aarch32: Always enter kernel via exception return Message-ID: References: <20240812101555.3558589-1-mark.rutland@arm.com> <20240812101555.3558589-7-mark.rutland@arm.com> <20240819182241.36d15eb1@donnerap.manchester.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240819182241.36d15eb1@donnerap.manchester.arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240820_044322_557961_97D1369E X-CRM114-Status: GOOD ( 38.49 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Aug 19, 2024 at 06:22:41PM +0100, Andre Przywara wrote: > On Mon, 12 Aug 2024 11:15:51 +0100 > Mark Rutland wrote: > > Hi Mark, > > > When the boot-wrapper is entered at Seculre PL1 it will enter the kernel > > Secure > > > via an exception return, ERET, and when entered at Hyp it will branch to FWIW, that "ERET, " here was meant to go too (which I think addresses a later concern below). I had taken the commit message wording from the early AArch64 commit and adjusted it to suit AArch32, but clearly I had done that in a rush and madea number of mistakes. > > the kernel directly. This is an artifact of the way the boot-wrapper was > > originally written in assembly, and it would be preferable to always > > enter the kernel via an exception return so that PSTATE is always > > initialized to a known-good value. > > > > Rework jump_kernel() to always enter the kernel via ERET, matching the > > stype of the AArch64 version of jump_kernel() > > type > > > > > Signed-off-by: Mark Rutland > > Acked-by: Marc Zyngier > > Cc: Akos Denke > > Cc: Andre Przywara > > Cc: Luca Fancellu > > --- > > arch/aarch32/boot.S | 48 +++++++++++++++++++++++---------------------- > > 1 file changed, 25 insertions(+), 23 deletions(-) > > > > diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S > > index f21f89a..78d19a0 100644 > > --- a/arch/aarch32/boot.S > > +++ b/arch/aarch32/boot.S > > @@ -76,10 +76,6 @@ reset_at_hyp: > > > > bl setup_stack > > > > - mov r0, #1 > > - ldr r1, =flag_no_el3 > > - str r0, [r1] > > - > > bl cpu_init_bootwrapper > > > > bl cpu_init_arch > > @@ -96,9 +92,10 @@ err_invalid_id: > > * r1-r3, sp[0]: kernel arguments > > */ > > ASM_FUNC(jump_kernel) > > - sub sp, #4 @ Ignore fourth argument > > Can we maybe keep the comment, to avoid confusion? The comment above > explicitly mentions sp[0], but we never use it. > > > - push {r0 - r3} > > - mov r5, sp > > + mov r4, r0 > > + mov r5, r1 > > + mov r6, r2 > > + mov r7, r3 > > > > ldr r0, =HSCTLR_KERNEL > > mcr p15, 4, r0, c1, c0, 0 @ HSCTLR > > @@ -111,23 +108,28 @@ ASM_FUNC(jump_kernel) > > bl find_logical_id > > bl setup_stack > > > > - ldr lr, [r5], #4 > > - ldm r5, {r0 - r2} > > - > > - ldr r4, =flag_no_el3 > > - ldr r4, [r4] > > - cmp r4, #1 > > - bxeq lr @ no EL3 > > + mov r0, r5 > > + mov r1, r6 > > + mov r2, r7 > > + ldr r3, =SPSR_KERNEL > > > > - ldr r4, =SPSR_KERNEL > > /* Return in thumb2 mode when bit 0 of address is 1 */ > > - tst lr, #1 > > - orrne r4, #PSR_T > > + tst r4, #1 > > + orrne r3, #PSR_T > > + > > + mrs r5, cpsr > > + and r5, #PSR_MODE_MASK > > + cmp r5, #PSR_MON > > + beq eret_at_mon > > + cmp r5, #PSR_HYP > > + beq eret_at_hyp > > + b . > > > > - msr spsr_cxf, r4 > > +eret_at_mon: > > + mov lr, r4 > > + msr spsr_cxf, r3 > > movs pc, lr > > Maybe that's just a wording confusion between "exception return > instruction" and "eret", but both the commit message and the label > promise an eret, and we have a "movs pc" here. Wording-wise, "ERET" was spurious, and the commit message was inteneded to say "via an exception return", with the "movs pc, lr" being the exception return. > Reading "B9.1 General restrictions on system instructions" in the ARMv7 ARM > I don't immediately see why an eret wouldn't be possible here. > > If there is a restriction I missed, I guess either a comment here or in > the commit message would be helpful. We can use ERET here; IIRC that was added in the ARMv7 virtualization extensions, but the boot-wrapper requires that and really it's ARMv8+ anyway. I had opted to stick with "movs pc, lr" because it was a (trivially) smaller change, and kept the cases distinct, but I'm happy to use ERET. However, beware that in AArch32 ERET is a bit odd: in Hyp mode takes the return address from ELR_HYP, while in all other modes it takes it from the LR (as only hyp has an ELR). > > - > > - .section .data > > - .align 2 > > -flag_no_el3: > > - .long 0 > > +eret_at_hyp: > > + msr elr_hyp, r4 > > + msr spsr_cxf, r3 > > Shouldn't that be spsr_hyp? It can be, but doesn't need to be. This is the SPSR_ encoding, which writes to the SPSR for owned by the active mode, though it skips bits<23:16>, which we probably should initialise. If I change that all to: | eret_at_mon: | mov lr, r4 | msr spsr_mon, r3 | eret | eret_at_hyp: | msr elr_hyp, r4 | msr spsr_hyp, r3 | ... do you think that's clear enough, or do you think we need a comment about the "LR" vs "ELR_HYP" distinction? Mark.