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 4922BC433F5 for ; Fri, 14 Jan 2022 18:14:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jKl4b9BV6CQ8FuhbgBK3L5vDrrqKYsqfGmwcUoB6eFQ=; b=zNpjmLtUl46Dvl XlmBx6Db5cWaQ4l+/sA7QQjt3ar5o0gklopZcJmXCZC/DG8YD2ljot+2NaRQtja3Ab7HsMS2MjoX4 OWO98z4IGxfM5LW461kkWjihHRMOyD7Wv63LtKGyuZwGw4cnzeyO0S6ABpq7dgjh0FjH9CAcPYdpM 8Gy3BS/gzfJKP4lyCZFxEK+1XBudknzoQwW2h6iv7iuQVJhKL58At47NqVC6ErlshBRLLfyEr+PEP 101/HO/veOXtdxjtNB5Si02Xr9fwhcNGSVeGIT45wz/oufQtvQZHeEk0Dy8Fv31CFazC65DOGo227 WoYIPURjUkFtidEUXqrA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8R4K-00A0WB-EB; Fri, 14 Jan 2022 18:12:56 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8R4G-00A0Vr-4K for linux-arm-kernel@lists.infradead.org; Fri, 14 Jan 2022 18:12:53 +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 DB53FED1; Fri, 14 Jan 2022 10:12:50 -0800 (PST) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F3C9B3F766; Fri, 14 Jan 2022 10:12:49 -0800 (PST) Date: Fri, 14 Jan 2022 18:12:47 +0000 From: Andre Przywara To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, Jaxson.Han@arm.com, robin.murphy@arm.com, vladimir.murzin@arm.com, Wei.Chen@arm.com Subject: Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper Message-ID: <20220114181247.7b366f45@donnerap.cambridge.arm.com> In-Reply-To: <20220114105653.3003399-7-mark.rutland@arm.com> References: <20220114105653.3003399-1-mark.rutland@arm.com> <20220114105653.3003399-7-mark.rutland@arm.com> Organization: ARM X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220114_101252_315266_161E2882 X-CRM114-Status: GOOD ( 33.21 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 14 Jan 2022 10:56:46 +0000 Mark Rutland wrote: Hi Mark, > The SCTLR_ELx registers contain fields which are UNKNOWN or > IMPLEMENTATION DEFINED out of reset. This includes SCTLR_ELx.EE, which > defines the endianness of memory accesses (e.g. reads from literal > pools). Due to this, portions of boot-wrapper code are not guaranteed > to work correctly. > > Rework the startup code to explicitly initialize SCTLR_ELx for the > exception level the boot-wrapper was entered at. When entered at EL2 > it's necessary to first initialise HCR_EL2.E2H as this affects the RESx > behaviour of bits in SCTLR_EL2, and also aliases SCTLR_EL1 to SCTLR_EL2, > which would break the initialization performed in jump_kernel. > > As we plan to eventually support the highest implemented EL being any of > EL3/EL2/EL1, code is added to handle all of these exception levels, even > though we do not currently support starting at EL1. > > We'll initialize other registers in subsequent patches. So the idea of initialising each EL and the respective code below looks good to me, however I have some questions about the SCTLR reset values below: > > Signed-off-by: Mark Rutland > --- > arch/aarch64/boot.S | 74 +++++++++++++++++++++++++++------- > arch/aarch64/include/asm/cpu.h | 27 ++++++++++++- > 2 files changed, 85 insertions(+), 16 deletions(-) > > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S > index 900b9f8..45a0367 100644 > --- a/arch/aarch64/boot.S > +++ b/arch/aarch64/boot.S > @@ -26,26 +26,26 @@ > * PSCI is not supported when entered in this exception level. > */ > ASM_FUNC(_start) > - cpuid x0, x1 > - bl find_logical_id > - cmp x0, #MPIDR_INVALID > - beq err_invalid_id > - bl setup_stack > - > - /* > - * EL3 initialisation > - */ > mrs x0, CurrentEL > cmp x0, #CURRENTEL_EL3 > - b.eq 1f > + b.eq reset_at_el3 > + cmp x0, #CURRENTEL_EL2 > + b.eq reset_at_el2 > + cmp x0, #CURRENTEL_EL1 > + b.eq reset_at_el1 > > - mov w0, #1 > - ldr x1, =flag_no_el3 > - str w0, [x1] > + /* Booting at EL0 is not supported */ > + b . > > - b start_no_el3 > + /* > + * EL3 initialisation > + */ > +reset_at_el3: > + mov_64 x0, SCTLR_EL3_RESET > + msr sctlr_el3, x0 > + isb > > -1: mov x0, #0x30 // RES1 > + mov x0, #0x30 // RES1 > orr x0, x0, #(1 << 0) // Non-secure EL1 > orr x0, x0, #(1 << 8) // HVC enable > > @@ -135,10 +135,54 @@ ASM_FUNC(_start) > ldr x0, =COUNTER_FREQ > msr cntfrq_el0, x0 > > + cpuid x0, x1 > + bl find_logical_id > + cmp x0, #MPIDR_INVALID > + b.eq err_invalid_id > + bl setup_stack > + > bl gic_secure_init > > b start_el3 > > + /* > + * EL2 initialization > + */ > +reset_at_el2: > + // Ensure E2H is not in use > + mov_64 x0, HCR_EL2_RESET > + msr hcr_el2, x0 > + isb > + > + mov_64 x0, SCTLR_EL2_RESET > + msr sctlr_el2, x0 > + isb > + > + b reset_no_el3 > + > + /* > + * EL1 initialization > + */ > +reset_at_el1: > + mov_64 x0, SCTLR_EL1_RESET > + msr sctlr_el1, x0 > + isb > + > + b reset_no_el3 > + > +reset_no_el3: > + cpuid x0, x1 > + bl find_logical_id > + cmp x0, #MPIDR_INVALID > + b.eq err_invalid_id > + bl setup_stack > + > + mov w0, #1 > + ldr x1, =flag_no_el3 > + str w0, [x1] > + > + b start_no_el3 > + > err_invalid_id: > b . > > diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h > index 1053414..1e9141a 100644 > --- a/arch/aarch64/include/asm/cpu.h > +++ b/arch/aarch64/include/asm/cpu.h > @@ -14,6 +14,32 @@ > #define MPIDR_ID_BITS 0xff00ffffff > > #define CURRENTEL_EL3 (3 << 2) > +#define CURRENTEL_EL2 (2 << 2) > +#define CURRENTEL_EL1 (1 << 2) > + > +/* > + * RES1 bit definitions definitions as of ARM DDI 0487G.b > + * > + * These includes bits which are RES1 in some configurations. > + */ > +#define SCTLR_EL3_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \ > + BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4)) > + > +#define SCTLR_EL2_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \ > + BIT(18) | BIT(16) | BIT(11) | BIT(5) | BIT(4)) I compared all bits against the ARM ARM and the kernel version for EL2, that looks correct to me. > + > +#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \ > + BIT(11) | BIT(8) | BIT(7) | BIT(4)) - The kernel sets TSCXT(bit[20]), and the ARM ARM says that the value should be RES1 if FEAT_CSV2_* is not implemented. Should we copy this? - The kernel clears ITD(bit[7]), and the ARM ARM says it's *Otherwise* RES1 (no AArch32 in EL0). I feel like we should not disable IT instructions in EL0 needlessly? - I also feel like we should set CP15BEN(bit[5]), for similar reasons. Granted those bits affect only EL0 execution, which we don't care about in the boot-wrapper, but I was wondering if we should change those anyway? At least bit 20? > + > +#define HCR_EL2_RES1 (BIT(1)) Should we set RW(bit[31]), just to be safe? Not sure this is explicitly mentioned somewhere, but is the boot flow when we are entered in EL2 to stay in EL2 and launch the kernel in there as well? > + > +/* > + * Initial register values required for the boot-wrapper to run out-of-reset. > + */ > +#define SCTLR_EL3_RESET SCTLR_EL3_RES1 > +#define SCTLR_EL2_RESET SCTLR_EL2_RES1 > +#define SCTLR_EL1_RESET SCTLR_EL1_RES1 > +#define HCR_EL2_RESET HCR_EL2_RES1 > > #define ID_AA64PFR0_EL1_GIC BITS(27, 24) > > @@ -43,7 +69,6 @@ > #define ZCR_EL3_LEN_MASK 0x1ff > > #define SCTLR_EL1_CP15BEN (1 << 5) > -#define SCTLR_EL1_RES1 (3 << 28 | 3 << 22 | 1 << 11) > > #ifdef KERNEL_32 > /* 32-bit kernel decompressor uses CP15 barriers */ > #define SCTLR_EL1_KERNEL (SCTLR_EL1_RES1 | SCTLR_EL1_CP15BEN) So I wonder if this actually works? The ARMv7 version of SCTLR differs in some bits from both the ARMv8 AArch32 version and more importantly the AArch64 version. I had troubles the other day running the arm32 Linux kernel decompressor with some ARMv8 SCTLR_EL1 reset value. The decompressor code does only read-modify-write of SCTLR (probably to cover multiple architecture revisions), so some bits might stay wrong. In particular I think having bits 28 and 29 set caused problems. By looking at the ARMv7 ARM and with experimentation I came up with 0x00c00878 as a safe and working value. Shall we have a separate reset value for 32bit? Cheers, Andre _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel