* [bootwrapper PATCH v2 01/13] Document entry requirements
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 10:56 ` [bootwrapper PATCH v2 02/13] Add bit-field macros Mark Rutland
` (12 subsequent siblings)
13 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
Currently the boot-wrapper only supports some combinations of exception
levels, with other combinations not being supported.
While we generally expect the boot-wrapper to be entered at the highest
implemented exception level, the AArch32 boot-wrapper has a comment
implying it supports being entered with something else owning EL3. As
this would require such EL3 firmware to always be in sync with the
boot-wrapper's requirements, which change over time, we don't actually
support such a configuration.
Some CPU state (such as CNTFRQ/CNTFRQ_EL0) needs to be initialized at
the highest implemented exception level, but today the boot-wrapper only
does so when entered at EL3 / Secure-PL1. Thus, today the only
completely supported configurations are EL3 / Secure-PL1, and entering
in other configurations is not entirely supported.
The aarch64 `jump_kernel` function always writes to SCTLR_EL2, which is
UNDEFINED at EL1. Hence, the aarch64 boot-wrapper does not support being
entered at EL1.
The aarch32 code assumes that any non-hyp mode is Secure PL1, and
attempt to switch to monitor mode. If entered on a system without the
security extensions, where the highest privileged mode is Non-secure
PL1, this will not work. Hence the aarch32 boot-wrapper does not support
being entered at Non-secure PL1.
Actually supporting all of these configurations requires restructuring
much of the boot-wrapper. For now, document the supported configurations
in each architecture's boot.S, and remove the misleading comment from
arch/aarch32/boot.S. Subsequent patches will improve the support and add
support for additional configurations.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch32/boot.S | 14 ++++++++++++++
arch/aarch32/psci.S | 5 -----
arch/aarch64/boot.S | 13 +++++++++++++
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
index 4add338..00c432d 100644
--- a/arch/aarch32/boot.S
+++ b/arch/aarch32/boot.S
@@ -16,6 +16,20 @@
.arch_extension virt
.section .init
+
+ /*
+ * The boot-wrapper must be entered from the reset vector at the
+ * highest implemented exception level. The boot-wrapper only supports
+ * being entered in the following modes:
+ *
+ * - PL1 / EL3 (Secure) Supervisor mode
+ * Entering in this mode is strongly recommended.
+ * PL2 must be implemented.
+ *
+ * - PL2 / EL2 (Non-secure) Hypervisor mode
+ * Entering in this mode is partially supported.
+ * PSCI is not supported when entered in this mode.
+ */
ASM_FUNC(_start)
/* Stack initialisation */
cpuid r0, r1
diff --git a/arch/aarch32/psci.S b/arch/aarch32/psci.S
index dc7aeb7..e0d2972 100644
--- a/arch/aarch32/psci.S
+++ b/arch/aarch32/psci.S
@@ -44,11 +44,6 @@ ASM_FUNC(start_el3)
/* pass through */
ASM_FUNC(start_no_el3)
- /*
- * For no-el3, we assume that firmware launched the boot-wrapper in
- * non-secure EL2 or EL1. We assume it has its own PSCI implementation
- * sitting at EL3, and that this path is only taken by primary CPU.
- */
cpuid r0, r1
blx find_logical_id
b psci_first_spin
diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
index 27ba449..900b9f8 100644
--- a/arch/aarch64/boot.S
+++ b/arch/aarch64/boot.S
@@ -12,6 +12,19 @@
.section .init
+ /*
+ * The boot-wrapper must be entered from the reset vector at the
+ * highest implemented exception level. The boot-wrapper only supports
+ * being entered at the following exception levels:
+ *
+ * - EL3 (Secure)
+ * Entering at EL3 is strongly recommended.
+ * EL2 must be implemented.
+ *
+ * - EL2 (Non-secure)
+ * Entering at EL2 is partially supported.
+ * PSCI is not supported when entered in this exception level.
+ */
ASM_FUNC(_start)
cpuid x0, x1
bl find_logical_id
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [bootwrapper PATCH v2 02/13] Add bit-field macros
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
2022-01-14 10:56 ` [bootwrapper PATCH v2 01/13] Document entry requirements Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 12:11 ` Steven Price
2022-01-14 10:56 ` [bootwrapper PATCH v2 03/13] aarch64: add system register accessors Mark Rutland
` (11 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
Arm architectural documentation typically defines bit-fields as
`[msb,lsb]` and single-bit fields as `[bit]`. For clarity it would be
helpful if we could define fields in the same way.
Add helpers so that we can do so, along with helper to extract/insert
bit-field values.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
include/bits.h | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
create mode 100644 include/bits.h
diff --git a/include/bits.h b/include/bits.h
new file mode 100644
index 0000000..0bf2c67
--- /dev/null
+++ b/include/bits.h
@@ -0,0 +1,79 @@
+/*
+ * include/bits.h - helpers for bit-field definitions.
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#ifndef __BITS_H
+#define __BITS_H
+
+#ifdef __ASSEMBLY__
+#define UL(x) x
+#define ULL(x) x
+#else
+#define UL(x) x##UL
+#define ULL(x) x##ULL
+#endif
+
+/*
+ * Define a contiguous mask of bits with `msb` as the most significant bit and
+ * `lsb` as the least significant bit. The `msb` value must be greater than or
+ * equal to `lsb`.
+ *
+ * For example:
+ * - BITS(63, 63) is 0x8000000000000000
+ * - BITS(63, 0) is 0xFFFFFFFFFFFFFFFF
+ * - BITS(0, 0) is 0x0000000000000001
+ * - BITS(49, 17) is 0x0001FFFFFFFE0000
+ */
+#define BITS(msb, lsb) \
+ ((~ULL(0) >> (63 - msb)) & (~ULL(0) << lsb))
+
+/*
+ * Define a mask of a single set bit `b`.
+ *
+ * For example:
+ * - BIT(63) is 0x8000000000000000
+ * - BIT(0) is 0x0000000000000001
+ * - BIT(32) is 0x0000000100000000
+ */
+#define BIT(b) BITS(b, b)
+
+/*
+ * Find the least significant set bit in the contiguous set of bits in `mask`.
+ *
+ * For example:
+ * - BITS_LSB(0x0000000000000001) is 0
+ * - BITS_LSB(0x000000000000ff00) is 8
+ * - BITS_LSB(0x8000000000000000) is 63
+ */
+#define BITS_LSB(mask) (__builtin_ffsll(mask) - 1)
+
+/*
+ * Extract a bit-field out of `val` described by the contiguous set of bits in
+ * `mask`.
+ *
+ * For example:
+ * - BITS_EXTRACT(0xABCD, BITS(15, 12)) is 0xA
+ * - BITS_EXTRACT(0xABCD, BITS(11, 8)) is 0xB
+ * - BITS_EXTRACT(0xABCD, BIT(7)) is 0x1
+ */
+#define BITS_EXTRACT(val, mask) \
+ (((val) & (mask)) >> BITS_LSB(mask))
+
+/*
+ * Insert the least significant bits of `val` into the bit-field described by
+ * the contiguous set of bits in `mask`.
+ *
+ * For example:
+ * - BITS_INSERT(BITS(3, 0), 0xA) is 0x000A
+ * - BITS_INSERT(BITS(15, 12), 0xA) is 0xA000
+ * - BITS_INSERT(BIT(15), 0xF) is 0x1000
+ *
+ */
+#define BITS_INSERT(mask, val) \
+ (((val) << BITS_LSB(mask)) & (mask))
+
+#endif
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 02/13] Add bit-field macros
2022-01-14 10:56 ` [bootwrapper PATCH v2 02/13] Add bit-field macros Mark Rutland
@ 2022-01-17 12:11 ` Steven Price
2022-01-17 13:28 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Steven Price @ 2022-01-17 12:11 UTC (permalink / raw)
To: Mark Rutland, linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On 14/01/2022 10:56, Mark Rutland wrote:
> Arm architectural documentation typically defines bit-fields as
> `[msb,lsb]` and single-bit fields as `[bit]`. For clarity it would be
> helpful if we could define fields in the same way.
>
> Add helpers so that we can do so, along with helper to extract/insert
> bit-field values.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> include/bits.h | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
> create mode 100644 include/bits.h
>
> diff --git a/include/bits.h b/include/bits.h
> new file mode 100644
> index 0000000..0bf2c67
> --- /dev/null
> +++ b/include/bits.h
> @@ -0,0 +1,79 @@
> +/*
> + * include/bits.h - helpers for bit-field definitions.
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __BITS_H
> +#define __BITS_H
> +
> +#ifdef __ASSEMBLY__
> +#define UL(x) x
> +#define ULL(x) x
> +#else
> +#define UL(x) x##UL
> +#define ULL(x) x##ULL
> +#endif
> +
> +/*
> + * Define a contiguous mask of bits with `msb` as the most significant bit and
> + * `lsb` as the least significant bit. The `msb` value must be greater than or
> + * equal to `lsb`.
> + *
> + * For example:
> + * - BITS(63, 63) is 0x8000000000000000
> + * - BITS(63, 0) is 0xFFFFFFFFFFFFFFFF
> + * - BITS(0, 0) is 0x0000000000000001
> + * - BITS(49, 17) is 0x0001FFFFFFFE0000
> + */
> +#define BITS(msb, lsb) \
> + ((~ULL(0) >> (63 - msb)) & (~ULL(0) << lsb))
> +
> +/*
> + * Define a mask of a single set bit `b`.
> + *
> + * For example:
> + * - BIT(63) is 0x8000000000000000
> + * - BIT(0) is 0x0000000000000001
> + * - BIT(32) is 0x0000000100000000
> + */
> +#define BIT(b) BITS(b, b)
> +
> +/*
> + * Find the least significant set bit in the contiguous set of bits in `mask`.
> + *
> + * For example:
> + * - BITS_LSB(0x0000000000000001) is 0
> + * - BITS_LSB(0x000000000000ff00) is 8
> + * - BITS_LSB(0x8000000000000000) is 63
> + */
> +#define BITS_LSB(mask) (__builtin_ffsll(mask) - 1)
> +
> +/*
> + * Extract a bit-field out of `val` described by the contiguous set of bits in
> + * `mask`.
> + *
> + * For example:
> + * - BITS_EXTRACT(0xABCD, BITS(15, 12)) is 0xA
> + * - BITS_EXTRACT(0xABCD, BITS(11, 8)) is 0xB
> + * - BITS_EXTRACT(0xABCD, BIT(7)) is 0x1
> + */
> +#define BITS_EXTRACT(val, mask) \
> + (((val) & (mask)) >> BITS_LSB(mask))
> +
> +/*
> + * Insert the least significant bits of `val` into the bit-field described by
> + * the contiguous set of bits in `mask`.
> + *
> + * For example:
> + * - BITS_INSERT(BITS(3, 0), 0xA) is 0x000A
> + * - BITS_INSERT(BITS(15, 12), 0xA) is 0xA000
> + * - BITS_INSERT(BIT(15), 0xF) is 0x1000
* - BITS_INSERT(BIT(35), 0x1) is 0x0
... which might be surprising!
of course BITS_INSERT(BIT(35), ULL(0x1)) works, but I feel either the
description above needs to signpost this gotcha, or a cast is needed below.
AFAICT this macro isn't actually used in your series.
Steve
> + *
> + */
> +#define BITS_INSERT(mask, val) \
> + (((val) << BITS_LSB(mask)) & (mask))
> +
> +#endif
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 02/13] Add bit-field macros
2022-01-17 12:11 ` Steven Price
@ 2022-01-17 13:28 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-17 13:28 UTC (permalink / raw)
To: Steven Price
Cc: linux-arm-kernel, andre.przywara, Jaxson.Han, robin.murphy,
vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 12:11:36PM +0000, Steven Price wrote:
> On 14/01/2022 10:56, Mark Rutland wrote:
> > +/*
> > + * Insert the least significant bits of `val` into the bit-field described by
> > + * the contiguous set of bits in `mask`.
> > + *
> > + * For example:
> > + * - BITS_INSERT(BITS(3, 0), 0xA) is 0x000A
> > + * - BITS_INSERT(BITS(15, 12), 0xA) is 0xA000
> > + * - BITS_INSERT(BIT(15), 0xF) is 0x1000
>
> * - BITS_INSERT(BIT(35), 0x1) is 0x0
>
> ... which might be surprising!
>
> of course BITS_INSERT(BIT(35), ULL(0x1)) works, but I feel either the
> description above needs to signpost this gotcha, or a cast is needed below.
>
> AFAICT this macro isn't actually used in your series.
Good point; I'll either add the cast or drop that for now.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 03/13] aarch64: add system register accessors
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
2022-01-14 10:56 ` [bootwrapper PATCH v2 01/13] Document entry requirements Mark Rutland
2022-01-14 10:56 ` [bootwrapper PATCH v2 02/13] Add bit-field macros Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 15:32 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 04/13] aarch32: add coprocessor accessors Mark Rutland
` (10 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
We open code the use of mrs/msr for specific registers, which is
somewhat tedious. Add macros to do this generically, along with a helper
to extract a specific register field. Existing C usage is converted to
the new helpers, and register definitions moved to a common location.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch64/include/asm/cpu.h | 41 ++++++++++++++++++++++---------
arch/aarch64/include/asm/gic-v3.h | 10 +++-----
2 files changed, 32 insertions(+), 19 deletions(-)
diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
index 63eb1c3..1053414 100644
--- a/arch/aarch64/include/asm/cpu.h
+++ b/arch/aarch64/include/asm/cpu.h
@@ -9,10 +9,14 @@
#ifndef __ASM_AARCH64_CPU_H
#define __ASM_AARCH64_CPU_H
+#include <bits.h>
+
#define MPIDR_ID_BITS 0xff00ffffff
#define CURRENTEL_EL3 (3 << 2)
+#define ID_AA64PFR0_EL1_GIC BITS(27, 24)
+
/*
* RES1 bits, little-endian, caches and MMU off, no alignment checking,
* no WXN.
@@ -29,6 +33,12 @@
#define CPTR_EL3_EZ (1 << 8)
+#define ICC_SRE_EL2 S3_4_C12_C9_5
+#define ICC_SRE_EL3 S3_6_C12_C12_5
+#define ICC_CTLR_EL1 S3_0_C12_C12_4
+#define ICC_CTLR_EL3 S3_6_C12_C12_4
+#define ICC_PMR_EL1 S3_0_C4_C6_0
+
#define ZCR_EL3 s3_6_c1_c2_0
#define ZCR_EL3_LEN_MASK 0x1ff
@@ -50,20 +60,27 @@
#define sevl() asm volatile ("sevl\n" : : : "memory")
-static inline unsigned long read_mpidr(void)
-{
- unsigned long mpidr;
+#define __str(def) #def
- asm volatile ("mrs %0, mpidr_el1\n" : "=r" (mpidr));
- return mpidr & MPIDR_ID_BITS;
-}
+#define mrs(reg) \
+({ \
+ unsigned long __mrs_val; \
+ asm volatile("mrs %0, " __str(reg) : "=r" (__mrs_val)); \
+ __mrs_val; \
+})
-static inline uint64_t read_id_aa64pfr0(void)
-{
- uint64_t val;
+#define msr(reg, val) \
+do { \
+ unsigned long __msr_val = val; \
+ asm volatile("msr " __str(reg) ", %0" : : "r" (__msr_val)); \
+} while (0)
+
+#define mrs_field(reg, field) \
+ BITS_EXTRACT(mrs(reg), (reg##_##field))
- asm volatile ("mrs %0, id_aa64pfr0_el1\n" : "=r" (val));
- return val;
+static inline unsigned long read_mpidr(void)
+{
+ return mrs(mpidr_el1) & MPIDR_ID_BITS;
}
static inline void iciallu(void)
@@ -73,7 +90,7 @@ static inline void iciallu(void)
static inline int has_gicv3_sysreg(void)
{
- return !!((read_id_aa64pfr0() >> 24) & 0xf);
+ return !!mrs_field(ID_AA64PFR0_EL1, GIC);
}
#endif /* !__ASSEMBLY__ */
diff --git a/arch/aarch64/include/asm/gic-v3.h b/arch/aarch64/include/asm/gic-v3.h
index 5b32380..2447480 100644
--- a/arch/aarch64/include/asm/gic-v3.h
+++ b/arch/aarch64/include/asm/gic-v3.h
@@ -9,20 +9,16 @@
#ifndef __ASM_AARCH64_GICV3_H
#define __ASM_AARCH64_GICV3_H
-#define ICC_SRE_EL2 "S3_4_C12_C9_5"
-#define ICC_SRE_EL3 "S3_6_C12_C12_5"
-#define ICC_CTLR_EL1 "S3_0_C12_C12_4"
-#define ICC_CTLR_EL3 "S3_6_C12_C12_4"
-#define ICC_PMR_EL1 "S3_0_C4_C6_0"
+#include <asm/cpu.h>
static inline void gic_write_icc_sre(uint32_t val)
{
- asm volatile ("msr " ICC_SRE_EL3 ", %0" : : "r" (val));
+ msr(ICC_SRE_EL3, val);
}
static inline void gic_write_icc_ctlr(uint32_t val)
{
- asm volatile ("msr " ICC_CTLR_EL3 ", %0" : : "r" (val));
+ msr(ICC_CTLR_EL3, val);
}
#endif
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 03/13] aarch64: add system register accessors
2022-01-14 10:56 ` [bootwrapper PATCH v2 03/13] aarch64: add system register accessors Mark Rutland
@ 2022-01-14 15:32 ` Andre Przywara
0 siblings, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-14 15:32 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:43 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> We open code the use of mrs/msr for specific registers, which is
> somewhat tedious. Add macros to do this generically, along with a helper
> to extract a specific register field. Existing C usage is converted to
> the new helpers, and register definitions moved to a common location.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Stared at and verified the architecture bits used in here:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> arch/aarch64/include/asm/cpu.h | 41 ++++++++++++++++++++++---------
> arch/aarch64/include/asm/gic-v3.h | 10 +++-----
> 2 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> index 63eb1c3..1053414 100644
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -9,10 +9,14 @@
> #ifndef __ASM_AARCH64_CPU_H
> #define __ASM_AARCH64_CPU_H
>
> +#include <bits.h>
> +
> #define MPIDR_ID_BITS 0xff00ffffff
>
> #define CURRENTEL_EL3 (3 << 2)
>
> +#define ID_AA64PFR0_EL1_GIC BITS(27, 24)
> +
> /*
> * RES1 bits, little-endian, caches and MMU off, no alignment checking,
> * no WXN.
> @@ -29,6 +33,12 @@
>
> #define CPTR_EL3_EZ (1 << 8)
>
> +#define ICC_SRE_EL2 S3_4_C12_C9_5
> +#define ICC_SRE_EL3 S3_6_C12_C12_5
> +#define ICC_CTLR_EL1 S3_0_C12_C12_4
> +#define ICC_CTLR_EL3 S3_6_C12_C12_4
> +#define ICC_PMR_EL1 S3_0_C4_C6_0
> +
> #define ZCR_EL3 s3_6_c1_c2_0
> #define ZCR_EL3_LEN_MASK 0x1ff
>
> @@ -50,20 +60,27 @@
>
> #define sevl() asm volatile ("sevl\n" : : : "memory")
>
> -static inline unsigned long read_mpidr(void)
> -{
> - unsigned long mpidr;
> +#define __str(def) #def
>
> - asm volatile ("mrs %0, mpidr_el1\n" : "=r" (mpidr));
> - return mpidr & MPIDR_ID_BITS;
> -}
> +#define mrs(reg) \
> +({ \
> + unsigned long __mrs_val; \
> + asm volatile("mrs %0, " __str(reg) : "=r" (__mrs_val)); \
> + __mrs_val; \
> +})
>
> -static inline uint64_t read_id_aa64pfr0(void)
> -{
> - uint64_t val;
> +#define msr(reg, val) \
> +do { \
> + unsigned long __msr_val = val; \
> + asm volatile("msr " __str(reg) ", %0" : : "r" (__msr_val)); \
> +} while (0)
> +
> +#define mrs_field(reg, field) \
> + BITS_EXTRACT(mrs(reg), (reg##_##field))
>
> - asm volatile ("mrs %0, id_aa64pfr0_el1\n" : "=r" (val));
> - return val;
> +static inline unsigned long read_mpidr(void)
> +{
> + return mrs(mpidr_el1) & MPIDR_ID_BITS;
> }
>
> static inline void iciallu(void)
> @@ -73,7 +90,7 @@ static inline void iciallu(void)
>
> static inline int has_gicv3_sysreg(void)
> {
> - return !!((read_id_aa64pfr0() >> 24) & 0xf);
> + return !!mrs_field(ID_AA64PFR0_EL1, GIC);
> }
>
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/aarch64/include/asm/gic-v3.h b/arch/aarch64/include/asm/gic-v3.h
> index 5b32380..2447480 100644
> --- a/arch/aarch64/include/asm/gic-v3.h
> +++ b/arch/aarch64/include/asm/gic-v3.h
> @@ -9,20 +9,16 @@
> #ifndef __ASM_AARCH64_GICV3_H
> #define __ASM_AARCH64_GICV3_H
>
> -#define ICC_SRE_EL2 "S3_4_C12_C9_5"
> -#define ICC_SRE_EL3 "S3_6_C12_C12_5"
> -#define ICC_CTLR_EL1 "S3_0_C12_C12_4"
> -#define ICC_CTLR_EL3 "S3_6_C12_C12_4"
> -#define ICC_PMR_EL1 "S3_0_C4_C6_0"
> +#include <asm/cpu.h>
>
> static inline void gic_write_icc_sre(uint32_t val)
> {
> - asm volatile ("msr " ICC_SRE_EL3 ", %0" : : "r" (val));
> + msr(ICC_SRE_EL3, val);
> }
>
> static inline void gic_write_icc_ctlr(uint32_t val)
> {
> - asm volatile ("msr " ICC_CTLR_EL3 ", %0" : : "r" (val));
> + msr(ICC_CTLR_EL3, val);
> }
>
> #endif
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 04/13] aarch32: add coprocessor accessors
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (2 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 03/13] aarch64: add system register accessors Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 10:56 ` [bootwrapper PATCH v2 05/13] aarch64: add mov_64 macro Mark Rutland
` (9 subsequent siblings)
13 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
We open code the use of mrc/mcr for specific registers, which is
somewhat tedious. Add macros to do this generically, along with a helper
to extract a specific register field. Existing C usage is converted to
the new helpers, and register definitions moved to a common location.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch32/include/asm/cpu.h | 45 ++++++++++++++++++++-----------
arch/aarch32/include/asm/gic-v3.h | 6 +++--
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
index 105cae5..d691c7b 100644
--- a/arch/aarch32/include/asm/cpu.h
+++ b/arch/aarch32/include/asm/cpu.h
@@ -9,9 +9,13 @@
#ifndef __ASM_AARCH32_CPU_H
#define __ASM_AARCH32_CPU_H
+#include <bits.h>
+
#define MPIDR_ID_BITS 0x00ffffff
#define MPIDR_INVALID (-1)
+#define ID_PFR1_GIC BITS(31, 28)
+
/* Only RES1 bits and CP15 barriers for the kernel */
#define HSCTLR_KERNEL (3 << 28 | 3 << 22 | 1 << 18 | 1 << 16 | 1 << 11 | 3 << 4)
#define SCTLR_KERNEL (3 << 22 | 1 << 11 | 1 << 5 | 3 << 4)
@@ -40,32 +44,43 @@
#define sevl() asm volatile ("sev" : : : "memory")
#endif
-static inline unsigned long read_mpidr(void)
-{
- unsigned long mpidr;
+#define MPIDR "p15, 0, %0, c0, c0, 5"
+#define ID_PFR1 "p15, 0, %0, c0, c1, 1"
+#define ICIALLU "p15, 0, %0, c7, c5, 0"
- asm volatile ("mrc p15, 0, %0, c0, c0, 5\n" : "=r" (mpidr));
- return mpidr & MPIDR_ID_BITS;
-}
+#define ICC_SRE "p15, 6, %0, c12, c12, 5"
+#define ICC_CTLR "p15, 6, %0, c12, c12, 4"
-static inline uint32_t read_id_pfr1(void)
-{
- uint32_t val;
+#define mrc(reg) \
+({ \
+ unsigned long __mrc_val; \
+ asm volatile("mrc " reg : "=r" (__mrc_val)); \
+ __mrc_val; \
+})
- asm volatile ("mrc p15, 0, %0, c0, c1, 1\n" : "=r" (val));
- return val;
+#define mcr(reg, val) \
+do { \
+ unsigned long __mcr_val = val; \
+ asm volatile("mcr " reg : : "r" (__mcr_val)); \
+} while (0)
+
+
+#define mrc_field(reg, field) \
+ BITS_EXTRACT(mrc(reg), (reg##_##field))
+
+static inline unsigned long read_mpidr(void)
+{
+ return mrc(MPIDR) & MPIDR_ID_BITS;
}
static inline void iciallu(void)
{
- uint32_t val = 0;
-
- asm volatile ("mcr p15, 0, %0, c7, c5, 0" : : "r" (val));
+ mcr(ICIALLU, 0);
}
static inline int has_gicv3_sysreg(void)
{
- return !!((read_id_pfr1() >> 28) & 0xf);
+ return !!mrc_field(ID_PFR1, GIC);
}
#endif /* __ASSEMBLY__ */
diff --git a/arch/aarch32/include/asm/gic-v3.h b/arch/aarch32/include/asm/gic-v3.h
index 65f38de..b28136a 100644
--- a/arch/aarch32/include/asm/gic-v3.h
+++ b/arch/aarch32/include/asm/gic-v3.h
@@ -9,14 +9,16 @@
#ifndef __ASM_AARCH32_GICV3_H
#define __ASM_AARCH32_GICV3_H
+#include <asm/cpu.h>
+
static inline void gic_write_icc_sre(uint32_t val)
{
- asm volatile ("mcr p15, 6, %0, c12, c12, 5" : : "r" (val));
+ mcr(ICC_SRE, val);
}
static inline void gic_write_icc_ctlr(uint32_t val)
{
- asm volatile ("mcr p15, 6, %0, c12, c12, 4" : : "r" (val));
+ mcr(ICC_CTLR, val);
}
#endif
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* [bootwrapper PATCH v2 05/13] aarch64: add mov_64 macro
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (3 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 04/13] aarch32: add coprocessor accessors Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 15:50 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper Mark Rutland
` (8 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
In subsequent patches we'll need to load 64-bit values into GPRs before
the CPU is in a known endianness, where we cannot use literal pools.
In preparation for that, this patch adds a new `mov_64` macro to load a
64-bit value into a GPR using a sequence of MOV and MOVKs, which will
function the same regardless of the CPU's endianness.
At the same time, move the `cpuid` macro to use `mov_64` internally.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch64/common.S | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/aarch64/common.S b/arch/aarch64/common.S
index c7171a9..3279fa9 100644
--- a/arch/aarch64/common.S
+++ b/arch/aarch64/common.S
@@ -9,9 +9,17 @@
#include <cpu.h>
+ /* Load a 64-bit value using immediates */
+ .macro mov_64 dest, val
+ mov \dest, #(((\val) >> 0) & 0xffff)
+ movk \dest, #(((\val) >> 16) & 0xffff), lsl #16
+ movk \dest, #(((\val) >> 32) & 0xffff), lsl #32
+ movk \dest, #(((\val) >> 48) & 0xffff), lsl #48
+ .endm
+
/* Put MPIDR into \dest, clobber \tmp and flags */
.macro cpuid dest, tmp
mrs \dest, mpidr_el1
- ldr \tmp, =MPIDR_ID_BITS
+ mov_64 \tmp, MPIDR_ID_BITS
ands \dest, \dest, \tmp
.endm
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 05/13] aarch64: add mov_64 macro
2022-01-14 10:56 ` [bootwrapper PATCH v2 05/13] aarch64: add mov_64 macro Mark Rutland
@ 2022-01-14 15:50 ` Andre Przywara
0 siblings, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-14 15:50 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:45 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> In subsequent patches we'll need to load 64-bit values into GPRs before
> the CPU is in a known endianness, where we cannot use literal pools.
>
> In preparation for that, this patch adds a new `mov_64` macro to load a
> 64-bit value into a GPR using a sequence of MOV and MOVKs, which will
> function the same regardless of the CPU's endianness.
>
> At the same time, move the `cpuid` macro to use `mov_64` internally.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Not sure that's worth it, but there is a simpler version of that TF-A
macro, along the lines of:
.if ((\val) >> 16) & 0xffff
movk \dest, #(((\val) >> 16) & 0xffff), lsl #16
.endif
It's a bit less optimal in a corner case than the TF-A version, but avoids
the few pointless " movk x0, #0x0, lsl #..." I found in the code.
But that's an optimisation detail, so anyway:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> arch/aarch64/common.S | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/aarch64/common.S b/arch/aarch64/common.S
> index c7171a9..3279fa9 100644
> --- a/arch/aarch64/common.S
> +++ b/arch/aarch64/common.S
> @@ -9,9 +9,17 @@
>
> #include <cpu.h>
>
> + /* Load a 64-bit value using immediates */
> + .macro mov_64 dest, val
> + mov \dest, #(((\val) >> 0) & 0xffff)
> + movk \dest, #(((\val) >> 16) & 0xffff), lsl #16
> + movk \dest, #(((\val) >> 32) & 0xffff), lsl #32
> + movk \dest, #(((\val) >> 48) & 0xffff), lsl #48
> + .endm
> +
> /* Put MPIDR into \dest, clobber \tmp and flags */
> .macro cpuid dest, tmp
> mrs \dest, mpidr_el1
> - ldr \tmp, =MPIDR_ID_BITS
> + mov_64 \tmp, MPIDR_ID_BITS
> ands \dest, \dest, \tmp
> .endm
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (4 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 05/13] aarch64: add mov_64 macro Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 18:12 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 07/13] Rework common init C code Mark Rutland
` (7 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
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.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
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))
+
+#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
+ BIT(11) | BIT(8) | BIT(7) | BIT(4))
+
+#define HCR_EL2_RES1 (BIT(1))
+
+/*
+ * 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 */
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-14 10:56 ` [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper Mark Rutland
@ 2022-01-14 18:12 ` Andre Przywara
2022-01-17 12:15 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-14 18:12 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:46 +0000
Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com>
> ---
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-14 18:12 ` Andre Przywara
@ 2022-01-17 12:15 ` Mark Rutland
2022-01-17 13:05 ` Mark Rutland
2022-01-19 12:42 ` Mark Rutland
0 siblings, 2 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-17 12:15 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:46 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
Hi Andre,
> > 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 <mark.rutland@arm.com>
> > ---
> > 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?
Yes, we should. I'll go and fold that in.
> - 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?
Per the ARM ARM the bit resets to an UNKNOWN value, and so per our usual
policy the kernel must initialize that before it can depend upon it, and
IIUC you say the kernel already does so.
So it shouldn't matter what the boot-wrapper does, and for consitency, I
think the boot-wrapper should set this to 0b1.
> - I also feel like we should set CP15BEN(bit[5]), for similar reasons.
I agree.
> 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?
I'll set all three of those bits. as above.
> > +#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?
I don't think we need to currently. HCR_EL2.RW is UNKNOWN out-of-reset,
is RAO/WI rather than RES1, and only affects lower ELs, so per usual
policy we leave this to the kernel to initialize. Of the configurations
this could affect:
* BW @ EL3 + {psci,spin}
Kernel entered at EL2 on all PEs.
* BW @ EL2 + spin-table
Kernel entered at EL2 on all PEs.
* BW @ EL2 + PSCI
Kernel entered at EL2 on boot PE.
BROKEN due to PSCI conduit anyhow.
I'd like to clean up the EL + boot-method combinations, but this
reqiures some more work which I had punted out of this series for now.
For now, I'd prefer to leave this as-is. I would like to fix this as
part of a subsequent boot-method cleanup, configuring HCR_EL2.RW as part
of the kernel handover logic later in the boot flow.
>
> > +
> > +/*
> > + * 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.
Hmm. SCTLR_EL1 is architecturally mapped to SCTLR, and for some reason I
thought that applied field-wise, but IIUC you're saying that applies
bit-wise, right?
The ARM ARM is delightfully unclear about this, which is reather
unfortunate. Per the glossary in ARM DDI 0487G.b:
| Where this manual describes a register as being architecturally mapped
| to another register, this indicates that, in an implementation that
| supports both of the registers, the two registers access the same
| state.
That being the case, we'd need to do likewise for all the other
registers accessible at kernel ELs, right?
> 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.
Do we have any comments/documentation saying what the 32-bit kernel
actually needs/wants here?
> Shall we have a separate reset value for 32bit?
Currently, the 32-bit side of the boot-wrapper uses:
(3 << 22 | 1 << 11 | 1 << 5 | 3 << 4)
... noting that `1 << 5` and `3 << 4` overlap..
Which is:
* bit[22]: RES1
* bit[11]: RES1
* bit[5]: CP15BEN
* bit[4]: LSMAOE
Per the above, it sounds like you think that's wrong too?
Whatever we do, the 32-bit/64-bit boot-wrapper should program something
functionally equivalent when booting a 32-bit kernel.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-17 12:15 ` Mark Rutland
@ 2022-01-17 13:05 ` Mark Rutland
2022-01-18 12:37 ` Andre Przywara
2022-01-19 12:42 ` Mark Rutland
1 sibling, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-17 13:05 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 12:15:57PM +0000, Mark Rutland wrote:
> On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> > On Fri, 14 Jan 2022 10:56:46 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Mark,
>
> Hi Andre,
> > > +
> > > +#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > > + BIT(11) | BIT(8) | BIT(7) | BIT(4))
> > > -#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.
Having re-read all of this, I believe you're right; I'll rework the
AArch32-kernel SCTLR_EL1_KERNEL to not use the AArch64 bit definitions,
and I'll add some commentary explaining that we're writing to the AArch32
format.
> > Shall we have a separate reset value for 32bit?
Assuming you meant to alter the SCTLR_ELx_KERNEL definition, yes.
The idea of splitting the SCTLR_ELx_RESET and SCTLR_ELx_KERNEL
definitions was that the former would be whatever the boot-wrapper
needed to run at ELx, and the latter was whatever we must initialize for
the kernel to run at ELx, so I don't want to put the AArch32 kernel bits
into SCTLR_EL1_RESET.
My only remaining concern is exactly what we must initialize. If there's
any documentation we can refer to, that'd be great, otherwise I'll dig
through your prior suggestion.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-17 13:05 ` Mark Rutland
@ 2022-01-18 12:37 ` Andre Przywara
2022-01-25 13:32 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-18 12:37 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, 17 Jan 2022 13:05:54 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> On Mon, Jan 17, 2022 at 12:15:57PM +0000, Mark Rutland wrote:
> > On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> > > On Fri, 14 Jan 2022 10:56:46 +0000
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > Hi Mark,
> >
> > Hi Andre,
>
> > > > +
> > > > +#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > > > + BIT(11) | BIT(8) | BIT(7) | BIT(4))
>
> > > > -#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.
>
> Having re-read all of this, I believe you're right; I'll rework the
> AArch32-kernel SCTLR_EL1_KERNEL to not use the AArch64 bit definitions,
> and I'll add some commentary explaining that we're writing to the AArch32
> format.
>
> > > Shall we have a separate reset value for 32bit?
>
> Assuming you meant to alter the SCTLR_ELx_KERNEL definition, yes.
>
> The idea of splitting the SCTLR_ELx_RESET and SCTLR_ELx_KERNEL
> definitions was that the former would be whatever the boot-wrapper
> needed to run at ELx, and the latter was whatever we must initialize for
> the kernel to run at ELx, so I don't want to put the AArch32 kernel bits
> into SCTLR_EL1_RESET.
>
> My only remaining concern is exactly what we must initialize. If there's
> any documentation we can refer to, that'd be great, otherwise I'll dig
> through your prior suggestion.
I don't know of any recommendation what to initialise, though the ARMv7
ARM speaks a bit about reset values.
So the SCTLR_KERNEL value we use in arch/aarch32/include/asm/cpu.h seems
to work (although it's not perfect), but we should use the same in the
KERNEL_32 case in arch/aarch64/include/asm/cpu.h.
Going through the SCTLR description in the ARMv8 *and* ARMv7 ARM again,
I think a safe and sane init value would be to set bits
[23,22,18,16,11,5,4,3] to one. This differs slightly from the value I told
you above, which I think was what I observed on an Cortex-A7 when dumping
SCTLR in the decompressor code.
I became aware of the issue when I tried to start an ARM kernel on a
Cortex-A53 board, with U-Boot dropping from AArch64-EL2 to AArch32-EL1.
SCTLR_EL1 there got initialised according to the ARMv8 ARM (bits
[29,28,23,22,20,11] set), but this made the kernel decompressor hang as
soon as the MMU got enabled. I *think* I bisected it down to bits 28 and
29, which are RES1 in AArch64, but enable TEX remap and the Access Flag in
v7, and both reset to 0 there. I haven't tried that on the model with the
boot-wrapper, but I guess we see the same problem there.
In any case it seems to be already broken, so a fix or discussion doesn't
need to block this series.
Cheers,
Andre
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-18 12:37 ` Andre Przywara
@ 2022-01-25 13:32 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-25 13:32 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Tue, Jan 18, 2022 at 12:37:41PM +0000, Andre Przywara wrote:
> On Mon, 17 Jan 2022 13:05:54 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
> > On Mon, Jan 17, 2022 at 12:15:57PM +0000, Mark Rutland wrote:
> > > On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> > > > On Fri, 14 Jan 2022 10:56:46 +0000
> > > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > Hi Mark,
> > >
> > > Hi Andre,
> >
> > > > > +
> > > > > +#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> > > > > + BIT(11) | BIT(8) | BIT(7) | BIT(4))
> >
> > > > > -#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.
> >
> > Having re-read all of this, I believe you're right; I'll rework the
> > AArch32-kernel SCTLR_EL1_KERNEL to not use the AArch64 bit definitions,
> > and I'll add some commentary explaining that we're writing to the AArch32
> > format.
> >
> > > > Shall we have a separate reset value for 32bit?
> >
> > Assuming you meant to alter the SCTLR_ELx_KERNEL definition, yes.
> >
> > The idea of splitting the SCTLR_ELx_RESET and SCTLR_ELx_KERNEL
> > definitions was that the former would be whatever the boot-wrapper
> > needed to run at ELx, and the latter was whatever we must initialize for
> > the kernel to run at ELx, so I don't want to put the AArch32 kernel bits
> > into SCTLR_EL1_RESET.
> >
> > My only remaining concern is exactly what we must initialize. If there's
> > any documentation we can refer to, that'd be great, otherwise I'll dig
> > through your prior suggestion.
>
> I don't know of any recommendation what to initialise, though the ARMv7
> ARM speaks a bit about reset values.
> So the SCTLR_KERNEL value we use in arch/aarch32/include/asm/cpu.h seems
> to work (although it's not perfect), but we should use the same in the
> KERNEL_32 case in arch/aarch64/include/asm/cpu.h.
Ok; for now I'm going to copy the SCTLR_KERNEL definition as-is from the
aarch32 header, and add a comment that we should restructure things to share a
single definition in future.
The rest of the reply is mostly for future reference.
> Going through the SCTLR description in the ARMv8 *and* ARMv7 ARM again,
> I think a safe and sane init value would be to set bits
> [23,22,18,16,11,5,4,3] to one. This differs slightly from the value I told
> you above, which I think was what I observed on an Cortex-A7 when dumping
> SCTLR in the decompressor code.
Having delved through the ARM ARM, those bits look right to me. Below is
expanded detail for each bit to save having to delve into the ARM ARM again.
Looking at the latest ARM ARM (ARM DDI 0487G.b), section G8.2.126 "SCTLR,
System Control Register" starting on page G8-6810:
* Bit 25 / EE
RES0/RES1 in depending on implemented endianness support. Currently the
boot-wrapper doesn't handle missin support for either BE or LE, so I'm going
to ignore this for now and just assume LE is present (so 0 is a legitimate
value).
* Bit 23 / SPAN
RES1 where FEAT_PAN is not implemented.
* Bit 22
RES1 always.
* Bit 18 / nTWE
Not RESx, but warm-resets to 1.
* Bit 16 / nTWI
Not RESx, but warm-resets to 1.
* Bit 11
RES1 always.
* Bit 5 / CP15BEN
Not RESx, but warm-resets to 1.
* Bit 4 / LSMAOE
RES1 where FEAT_LSMAOC is not implemented.
Warm-resets to 1 where FEAT_LSMAOC is implemented.
* Bit 3 / nTLSMD
RES1 where FEAT_LSMAOC is not implemented.
Warm-resets to 1 where FEAT_LSMAOC is implemented.
... with all other bits being UNKNOWN, RES0, or warm-rest to 0.
> I became aware of the issue when I tried to start an ARM kernel on a
> Cortex-A53 board, with U-Boot dropping from AArch64-EL2 to AArch32-EL1.
> SCTLR_EL1 there got initialised according to the ARMv8 ARM (bits
> [29,28,23,22,20,11] set), but this made the kernel decompressor hang as
> soon as the MMU got enabled. I *think* I bisected it down to bits 28 and
> 29, which are RES1 in AArch64, but enable TEX remap and the Access Flag in
> v7, and both reset to 0 there. I haven't tried that on the model with the
> boot-wrapper, but I guess we see the same problem there.
>
> In any case it seems to be already broken, so a fix or discussion doesn't
> need to block this series.
Sure; I think I'll save the AArch32 cleanup for a follow-up and go for a simple
change to jsut avoid making it worse in this patch.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper
2022-01-17 12:15 ` Mark Rutland
2022-01-17 13:05 ` Mark Rutland
@ 2022-01-19 12:42 ` Mark Rutland
1 sibling, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-19 12:42 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 12:15:57PM +0000, Mark Rutland wrote:
> On Fri, Jan 14, 2022 at 06:12:47PM +0000, Andre Przywara wrote:
> > On Fri, 14 Jan 2022 10:56:46 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > +
> > > +#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?
>
> Yes, we should. I'll go and fold that in.
>
> > - 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?
>
> Per the ARM ARM the bit resets to an UNKNOWN value, and so per our usual
> policy the kernel must initialize that before it can depend upon it, and
> IIUC you say the kernel already does so.
>
> So it shouldn't matter what the boot-wrapper does, and for consitency, I
> think the boot-wrapper should set this to 0b1.
>
> > - I also feel like we should set CP15BEN(bit[5]), for similar reasons.
>
> I agree.
I thought you meant it was also RES1 in the same case as ITD. It's actually
RES0 when there's no AArch32 EL0 support, so I'm not going to add it to
SCTLR_EL1_RES1.
I have added TSCXT (bit[20]) and ITD (bit[7]), as above, as those are RES1 in
some contexts.
THanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 07/13] Rework common init C code
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (5 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 06/13] aarch64: initialize SCTLR_ELx for the boot-wrapper Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 16:23 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level Mark Rutland
` (6 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
In init_platform() we initialize a UART and announce the presence of the
bootwrapper to the world. We do this relatively late in the boot-flow,
and prior to this will silently ignore errors (e.g. in gic_secure_init).
To make it possible to provide improved diagnostics, and to allow us to
move more initialization into C, this patch reworks the init code to
call a C function earlier, where we can announce the presence of the
boot-wrapper and perform other initialization.
In subsequent patches this will be expanded with more CPU
initialization.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
Makefile.am | 2 +-
arch/aarch32/boot.S | 5 +++++
arch/aarch64/boot.S | 4 ++++
common/boot.c | 4 ----
common/init.c | 24 ++++++++++++++++++++++++
common/platform.c | 12 +++++++-----
include/platform.h | 17 +++++++++++++++++
7 files changed, 58 insertions(+), 10 deletions(-)
create mode 100644 common/init.c
create mode 100644 include/platform.h
diff --git a/Makefile.am b/Makefile.am
index f941b07..0651c38 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -34,7 +34,7 @@ endif
PSCI_CPU_OFF := 0x84000002
COMMON_SRC := common/
-COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o
+COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o init.o
ARCH_OBJ := boot.o stack.o utils.o
diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
index 00c432d..ee073ea 100644
--- a/arch/aarch32/boot.S
+++ b/arch/aarch32/boot.S
@@ -48,6 +48,9 @@ ASM_FUNC(_start)
mov r0, #1
ldr r1, =flag_no_el3
str r0, [r1]
+
+ bl cpu_init_bootwrapper
+
b start_no_el3
_switch_monitor:
@@ -71,6 +74,8 @@ _monitor:
ldr r0, =COUNTER_FREQ
mcr p15, 0, r0, c14, c0, 0 @ CNTFRQ
+ bl cpu_init_bootwrapper
+
bl gic_secure_init
/* Initialise boot method */
diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
index 45a0367..17b4a75 100644
--- a/arch/aarch64/boot.S
+++ b/arch/aarch64/boot.S
@@ -141,6 +141,8 @@ reset_at_el3:
b.eq err_invalid_id
bl setup_stack
+ bl cpu_init_bootwrapper
+
bl gic_secure_init
b start_el3
@@ -181,6 +183,8 @@ reset_no_el3:
ldr x1, =flag_no_el3
str w0, [x1]
+ bl cpu_init_bootwrapper
+
b start_no_el3
err_invalid_id:
diff --git a/common/boot.c b/common/boot.c
index c74d34c..29d53a4 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -12,8 +12,6 @@
extern unsigned long entrypoint;
extern unsigned long dtb;
-void init_platform(void);
-
void __noreturn jump_kernel(unsigned long address,
unsigned long a0,
unsigned long a1,
@@ -62,8 +60,6 @@ void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
unsigned long invalid)
{
if (cpu == 0) {
- init_platform();
-
*mbox = (unsigned long)&entrypoint;
sevl();
spin(mbox, invalid, 1);
diff --git a/common/init.c b/common/init.c
new file mode 100644
index 0000000..9c471c9
--- /dev/null
+++ b/common/init.c
@@ -0,0 +1,24 @@
+/*
+ * init.c - common boot-wrapper initialization
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#include <cpu.h>
+#include <platform.h>
+
+static void announce_bootwrapper(void)
+{
+ print_string("Boot-wrapper v0.2\r\n\r\n");
+}
+
+void cpu_init_bootwrapper(void)
+{
+ if (this_cpu_logical_id() == 0) {
+ init_uart();
+ announce_bootwrapper();
+ init_platform();
+ }
+}
diff --git a/common/platform.c b/common/platform.c
index d11f568..47bf547 100644
--- a/common/platform.c
+++ b/common/platform.c
@@ -1,5 +1,5 @@
/*
- * platform.c - code to initialise everything required when first booting.
+ * platform.c - Platform initialization and I/O.
*
* Copyright (C) 2015 ARM Limited. All rights reserved.
*
@@ -7,6 +7,7 @@
* found in the LICENSE.txt file.
*/
+#include <cpu.h>
#include <stdint.h>
#include <asm/io.h>
@@ -30,7 +31,7 @@
#define V2M_SYS(reg) ((void *)SYSREGS_BASE + V2M_SYS_##reg)
#endif
-static void print_string(const char *str)
+void print_string(const char *str)
{
uint32_t flags;
@@ -47,7 +48,7 @@ static void print_string(const char *str)
}
}
-void init_platform(void)
+void init_uart(void)
{
/*
* UART initialisation (38400 8N1)
@@ -58,9 +59,10 @@ void init_platform(void)
raw_writel(0x70, PL011(UART_LCR_H));
/* Enable the UART, TXen and RXen */
raw_writel(0x301, PL011(UARTCR));
+}
- print_string("Boot-wrapper v0.2\r\n\r\n");
-
+void init_platform(void)
+{
#ifdef SYSREGS_BASE
/*
* CLCD output site MB
diff --git a/include/platform.h b/include/platform.h
new file mode 100644
index 0000000..e5248e1
--- /dev/null
+++ b/include/platform.h
@@ -0,0 +1,17 @@
+/*
+ * include/platform.h - Platform initialization and I/O.
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#ifndef __PLATFORM_H
+#define __PLATFORM_H
+
+void print_string(const char *str);
+void init_uart(void);
+
+void init_platform(void);
+
+#endif /* __PLATFORM_H */
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 07/13] Rework common init C code
2022-01-14 10:56 ` [bootwrapper PATCH v2 07/13] Rework common init C code Mark Rutland
@ 2022-01-17 16:23 ` Andre Przywara
0 siblings, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 16:23 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:47 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> In init_platform() we initialize a UART and announce the presence of the
> bootwrapper to the world. We do this relatively late in the boot-flow,
> and prior to this will silently ignore errors (e.g. in gic_secure_init).
>
> To make it possible to provide improved diagnostics, and to allow us to
> move more initialization into C, this patch reworks the init code to
> call a C function earlier, where we can announce the presence of the
> boot-wrapper and perform other initialization.
>
> In subsequent patches this will be expanded with more CPU
> initialization.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> Makefile.am | 2 +-
> arch/aarch32/boot.S | 5 +++++
> arch/aarch64/boot.S | 4 ++++
> common/boot.c | 4 ----
> common/init.c | 24 ++++++++++++++++++++++++
> common/platform.c | 12 +++++++-----
> include/platform.h | 17 +++++++++++++++++
> 7 files changed, 58 insertions(+), 10 deletions(-)
> create mode 100644 common/init.c
> create mode 100644 include/platform.h
>
> diff --git a/Makefile.am b/Makefile.am
> index f941b07..0651c38 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -34,7 +34,7 @@ endif
> PSCI_CPU_OFF := 0x84000002
>
> COMMON_SRC := common/
> -COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o
> +COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o init.o
>
> ARCH_OBJ := boot.o stack.o utils.o
>
> diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> index 00c432d..ee073ea 100644
> --- a/arch/aarch32/boot.S
> +++ b/arch/aarch32/boot.S
> @@ -48,6 +48,9 @@ ASM_FUNC(_start)
> mov r0, #1
> ldr r1, =flag_no_el3
> str r0, [r1]
> +
> + bl cpu_init_bootwrapper
> +
> b start_no_el3
>
> _switch_monitor:
> @@ -71,6 +74,8 @@ _monitor:
> ldr r0, =COUNTER_FREQ
> mcr p15, 0, r0, c14, c0, 0 @ CNTFRQ
>
> + bl cpu_init_bootwrapper
> +
> bl gic_secure_init
>
> /* Initialise boot method */
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index 45a0367..17b4a75 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -141,6 +141,8 @@ reset_at_el3:
> b.eq err_invalid_id
> bl setup_stack
>
> + bl cpu_init_bootwrapper
> +
> bl gic_secure_init
>
> b start_el3
> @@ -181,6 +183,8 @@ reset_no_el3:
> ldr x1, =flag_no_el3
> str w0, [x1]
>
> + bl cpu_init_bootwrapper
> +
> b start_no_el3
>
> err_invalid_id:
> diff --git a/common/boot.c b/common/boot.c
> index c74d34c..29d53a4 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -12,8 +12,6 @@
> extern unsigned long entrypoint;
> extern unsigned long dtb;
>
> -void init_platform(void);
> -
> void __noreturn jump_kernel(unsigned long address,
> unsigned long a0,
> unsigned long a1,
> @@ -62,8 +60,6 @@ void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
> unsigned long invalid)
> {
> if (cpu == 0) {
> - init_platform();
> -
> *mbox = (unsigned long)&entrypoint;
> sevl();
> spin(mbox, invalid, 1);
> diff --git a/common/init.c b/common/init.c
> new file mode 100644
> index 0000000..9c471c9
> --- /dev/null
> +++ b/common/init.c
> @@ -0,0 +1,24 @@
> +/*
> + * init.c - common boot-wrapper initialization
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#include <cpu.h>
> +#include <platform.h>
> +
> +static void announce_bootwrapper(void)
> +{
> + print_string("Boot-wrapper v0.2\r\n\r\n");
> +}
> +
> +void cpu_init_bootwrapper(void)
> +{
> + if (this_cpu_logical_id() == 0) {
> + init_uart();
> + announce_bootwrapper();
> + init_platform();
> + }
> +}
> diff --git a/common/platform.c b/common/platform.c
> index d11f568..47bf547 100644
> --- a/common/platform.c
> +++ b/common/platform.c
> @@ -1,5 +1,5 @@
> /*
> - * platform.c - code to initialise everything required when first booting.
> + * platform.c - Platform initialization and I/O.
> *
> * Copyright (C) 2015 ARM Limited. All rights reserved.
> *
> @@ -7,6 +7,7 @@
> * found in the LICENSE.txt file.
> */
>
> +#include <cpu.h>
> #include <stdint.h>
>
> #include <asm/io.h>
> @@ -30,7 +31,7 @@
> #define V2M_SYS(reg) ((void *)SYSREGS_BASE + V2M_SYS_##reg)
> #endif
>
> -static void print_string(const char *str)
> +void print_string(const char *str)
> {
> uint32_t flags;
>
> @@ -47,7 +48,7 @@ static void print_string(const char *str)
> }
> }
>
> -void init_platform(void)
> +void init_uart(void)
> {
> /*
> * UART initialisation (38400 8N1)
> @@ -58,9 +59,10 @@ void init_platform(void)
> raw_writel(0x70, PL011(UART_LCR_H));
> /* Enable the UART, TXen and RXen */
> raw_writel(0x301, PL011(UARTCR));
> +}
>
> - print_string("Boot-wrapper v0.2\r\n\r\n");
> -
> +void init_platform(void)
> +{
> #ifdef SYSREGS_BASE
> /*
> * CLCD output site MB
> diff --git a/include/platform.h b/include/platform.h
> new file mode 100644
> index 0000000..e5248e1
> --- /dev/null
> +++ b/include/platform.h
> @@ -0,0 +1,17 @@
> +/*
> + * include/platform.h - Platform initialization and I/O.
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __PLATFORM_H
> +#define __PLATFORM_H
> +
> +void print_string(const char *str);
> +void init_uart(void);
> +
> +void init_platform(void);
> +
> +#endif /* __PLATFORM_H */
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (6 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 07/13] Rework common init C code Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 14:39 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C Mark Rutland
` (5 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
When something goes wrong within the boot-wrapper, it can be very
helpful to know where we started from. Add an arch_announce() function
to log this early in the boot process. More information can be added
here in future.
This is logged ot the serial console as:
| Boot-wrapper v0.2
| Entered at EL3
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
Makefile.am | 2 +-
arch/aarch32/include/asm/cpu.h | 9 +++++++++
arch/aarch32/init.c | 30 ++++++++++++++++++++++++++++++
arch/aarch64/init.c | 19 +++++++++++++++++++
common/init.c | 6 +++++-
common/platform.c | 24 ++++++++++++++----------
include/platform.h | 1 +
7 files changed, 79 insertions(+), 12 deletions(-)
create mode 100644 arch/aarch32/init.c
create mode 100644 arch/aarch64/init.c
diff --git a/Makefile.am b/Makefile.am
index 0651c38..885a77c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -36,7 +36,7 @@ PSCI_CPU_OFF := 0x84000002
COMMON_SRC := common/
COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o init.o
-ARCH_OBJ := boot.o stack.o utils.o
+ARCH_OBJ := boot.o stack.o utils.o init.o
if BOOTWRAPPER_32
CPPFLAGS += -DBOOTWRAPPER_32
diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
index d691c7b..aa72204 100644
--- a/arch/aarch32/include/asm/cpu.h
+++ b/arch/aarch32/include/asm/cpu.h
@@ -44,6 +44,15 @@
#define sevl() asm volatile ("sev" : : : "memory")
#endif
+static inline unsigned long read_cpsr(void)
+{
+ unsigned long cpsr;
+ asm volatile ("mrs %0, cpsr\n" : "=r" (cpsr));
+ return cpsr;
+}
+
+#define read_cpsr_mode() (read_cpsr() & PSR_MODE_MASK)
+
#define MPIDR "p15, 0, %0, c0, c0, 5"
#define ID_PFR1 "p15, 0, %0, c0, c1, 1"
#define ICIALLU "p15, 0, %0, c7, c5, 0"
diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c
new file mode 100644
index 0000000..b29ebb4
--- /dev/null
+++ b/arch/aarch32/init.c
@@ -0,0 +1,30 @@
+/*
+ * init.c - common boot-wrapper initialization
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#include <platform.h>
+
+#include <asm/cpu.h>
+
+static const char *mode_string(void)
+{
+ switch (read_cpsr_mode()) {
+ case PSR_MON:
+ return "PL1";
+ case PSR_HYP:
+ return "PL2 (Non-secure)";
+ default:
+ return "<UNKNOWN MODE>";
+ }
+}
+
+void announce_arch(void)
+{
+ print_string("Entered at ");
+ print_string(mode_string());
+ print_string("\r\n");
+}
diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
new file mode 100644
index 0000000..82816e7
--- /dev/null
+++ b/arch/aarch64/init.c
@@ -0,0 +1,19 @@
+/*
+ * init.c - common boot-wrapper initialization
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#include <cpu.h>
+#include <platform.h>
+
+void announce_arch(void)
+{
+ unsigned char el = mrs(CurrentEl) >> 2;
+
+ print_string("Entered at EL");
+ print_char('0' + el);
+ print_string("\r\n");
+}
diff --git a/common/init.c b/common/init.c
index 9c471c9..2600f73 100644
--- a/common/init.c
+++ b/common/init.c
@@ -11,14 +11,18 @@
static void announce_bootwrapper(void)
{
- print_string("Boot-wrapper v0.2\r\n\r\n");
+ print_string("Boot-wrapper v0.2\r\n");
}
+void announce_arch(void);
+
void cpu_init_bootwrapper(void)
{
if (this_cpu_logical_id() == 0) {
init_uart();
announce_bootwrapper();
+ announce_arch();
+ print_string("\r\n");
init_platform();
}
}
diff --git a/common/platform.c b/common/platform.c
index 47bf547..80d0562 100644
--- a/common/platform.c
+++ b/common/platform.c
@@ -31,21 +31,25 @@
#define V2M_SYS(reg) ((void *)SYSREGS_BASE + V2M_SYS_##reg)
#endif
-void print_string(const char *str)
+void print_char(char c)
{
uint32_t flags;
- while (*str) {
- do
- flags = raw_readl(PL011(UARTFR));
- while (flags & PL011_UARTFR_FIFO_FULL);
+ do {
+ flags = raw_readl(PL011(UARTFR));
+ } while (flags & PL011_UARTFR_FIFO_FULL);
+
+ raw_writel(c, PL011(UARTDR));
- raw_writel(*str++, PL011(UARTDR));
+ do {
+ flags = raw_readl(PL011(UARTFR));
+ } while (flags & PL011_UARTFR_BUSY);
+}
- do
- flags = raw_readl(PL011(UARTFR));
- while (flags & PL011_UARTFR_BUSY);
- }
+void print_string(const char *str)
+{
+ while (*str)
+ print_char(*str++);
}
void init_uart(void)
diff --git a/include/platform.h b/include/platform.h
index e5248e1..237b481 100644
--- a/include/platform.h
+++ b/include/platform.h
@@ -9,6 +9,7 @@
#ifndef __PLATFORM_H
#define __PLATFORM_H
+void print_char(char c);
void print_string(const char *str);
void init_uart(void);
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level
2022-01-14 10:56 ` [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level Mark Rutland
@ 2022-01-17 14:39 ` Andre Przywara
2022-01-17 15:50 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 14:39 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:48 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> When something goes wrong within the boot-wrapper, it can be very
> helpful to know where we started from. Add an arch_announce() function
> to log this early in the boot process. More information can be added
> here in future.
>
> This is logged ot the serial console as:
>
> | Boot-wrapper v0.2
> | Entered at EL3
I like that one, and apart from the (already existing) UART issue below,
this looks fine:
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> Makefile.am | 2 +-
> arch/aarch32/include/asm/cpu.h | 9 +++++++++
> arch/aarch32/init.c | 30 ++++++++++++++++++++++++++++++
> arch/aarch64/init.c | 19 +++++++++++++++++++
> common/init.c | 6 +++++-
> common/platform.c | 24 ++++++++++++++----------
> include/platform.h | 1 +
> 7 files changed, 79 insertions(+), 12 deletions(-)
> create mode 100644 arch/aarch32/init.c
> create mode 100644 arch/aarch64/init.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 0651c38..885a77c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -36,7 +36,7 @@ PSCI_CPU_OFF := 0x84000002
> COMMON_SRC := common/
> COMMON_OBJ := boot.o bakery_lock.o platform.o lib.o init.o
>
> -ARCH_OBJ := boot.o stack.o utils.o
> +ARCH_OBJ := boot.o stack.o utils.o init.o
>
> if BOOTWRAPPER_32
> CPPFLAGS += -DBOOTWRAPPER_32
> diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
> index d691c7b..aa72204 100644
> --- a/arch/aarch32/include/asm/cpu.h
> +++ b/arch/aarch32/include/asm/cpu.h
> @@ -44,6 +44,15 @@
> #define sevl() asm volatile ("sev" : : : "memory")
> #endif
>
> +static inline unsigned long read_cpsr(void)
> +{
> + unsigned long cpsr;
> + asm volatile ("mrs %0, cpsr\n" : "=r" (cpsr));
> + return cpsr;
> +}
> +
> +#define read_cpsr_mode() (read_cpsr() & PSR_MODE_MASK)
> +
> #define MPIDR "p15, 0, %0, c0, c0, 5"
> #define ID_PFR1 "p15, 0, %0, c0, c1, 1"
> #define ICIALLU "p15, 0, %0, c7, c5, 0"
> diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c
> new file mode 100644
> index 0000000..b29ebb4
> --- /dev/null
> +++ b/arch/aarch32/init.c
> @@ -0,0 +1,30 @@
> +/*
> + * init.c - common boot-wrapper initialization
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#include <platform.h>
> +
> +#include <asm/cpu.h>
> +
> +static const char *mode_string(void)
> +{
> + switch (read_cpsr_mode()) {
> + case PSR_MON:
> + return "PL1";
> + case PSR_HYP:
> + return "PL2 (Non-secure)";
> + default:
> + return "<UNKNOWN MODE>";
> + }
> +}
> +
> +void announce_arch(void)
> +{
> + print_string("Entered at ");
> + print_string(mode_string());
> + print_string("\r\n");
> +}
> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> new file mode 100644
> index 0000000..82816e7
> --- /dev/null
> +++ b/arch/aarch64/init.c
> @@ -0,0 +1,19 @@
> +/*
> + * init.c - common boot-wrapper initialization
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#include <cpu.h>
> +#include <platform.h>
> +
> +void announce_arch(void)
> +{
> + unsigned char el = mrs(CurrentEl) >> 2;
> +
> + print_string("Entered at EL");
> + print_char('0' + el);
> + print_string("\r\n");
> +}
> diff --git a/common/init.c b/common/init.c
> index 9c471c9..2600f73 100644
> --- a/common/init.c
> +++ b/common/init.c
> @@ -11,14 +11,18 @@
>
> static void announce_bootwrapper(void)
> {
> - print_string("Boot-wrapper v0.2\r\n\r\n");
> + print_string("Boot-wrapper v0.2\r\n");
> }
>
> +void announce_arch(void);
> +
> void cpu_init_bootwrapper(void)
> {
> if (this_cpu_logical_id() == 0) {
> init_uart();
> announce_bootwrapper();
> + announce_arch();
> + print_string("\r\n");
> init_platform();
> }
> }
> diff --git a/common/platform.c b/common/platform.c
> index 47bf547..80d0562 100644
> --- a/common/platform.c
> +++ b/common/platform.c
> @@ -31,21 +31,25 @@
> #define V2M_SYS(reg) ((void *)SYSREGS_BASE + V2M_SYS_##reg)
> #endif
>
> -void print_string(const char *str)
> +void print_char(char c)
> {
> uint32_t flags;
>
> - while (*str) {
> - do
> - flags = raw_readl(PL011(UARTFR));
> - while (flags & PL011_UARTFR_FIFO_FULL);
> + do {
> + flags = raw_readl(PL011(UARTFR));
> + } while (flags & PL011_UARTFR_FIFO_FULL);
> +
> + raw_writel(c, PL011(UARTDR));
>
> - raw_writel(*str++, PL011(UARTDR));
> + do {
> + flags = raw_readl(PL011(UARTFR));
> + } while (flags & PL011_UARTFR_BUSY);
Apologies if that appears to be nitpicking over a totally pointless issue
(given the nature of the *emulated* PL011 in the model), but:
I understand that this code has not changed, but this loop basically
renders the FIFOs ineffective. Is this intended? I mean we explicitly
enable the FIFOs in init_uart(), but then poll here until the transmit
FIFO becomes empty, after *every* character pushed. I see that we probably
want the output to be synchronous, for debug reasons, but I wonder if this
should be achieved via an extra uart_flush() routine, once per line
output? So call uart_flush() just in print_string(), but not in
print_char(), for instance?
Cheers,
Andre
> +}
>
> - do
> - flags = raw_readl(PL011(UARTFR));
> - while (flags & PL011_UARTFR_BUSY);
> - }
> +void print_string(const char *str)
> +{
> + while (*str)
> + print_char(*str++);
> }
>
> void init_uart(void)
> diff --git a/include/platform.h b/include/platform.h
> index e5248e1..237b481 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -9,6 +9,7 @@
> #ifndef __PLATFORM_H
> #define __PLATFORM_H
>
> +void print_char(char c);
> void print_string(const char *str);
> void init_uart(void);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level
2022-01-17 14:39 ` Andre Przywara
@ 2022-01-17 15:50 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-17 15:50 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 02:39:13PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:48 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
> > When something goes wrong within the boot-wrapper, it can be very
> > helpful to know where we started from. Add an arch_announce() function
> > to log this early in the boot process. More information can be added
> > here in future.
> >
> > This is logged ot the serial console as:
> >
> > | Boot-wrapper v0.2
> > | Entered at EL3
>
> I like that one, and apart from the (already existing) UART issue below,
> this looks fine:
>
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Thanks!
> > diff --git a/common/platform.c b/common/platform.c
> > index 47bf547..80d0562 100644
> > --- a/common/platform.c
> > +++ b/common/platform.c
> > @@ -31,21 +31,25 @@
> > #define V2M_SYS(reg) ((void *)SYSREGS_BASE + V2M_SYS_##reg)
> > #endif
> >
> > -void print_string(const char *str)
> > +void print_char(char c)
> > {
> > uint32_t flags;
> >
> > - while (*str) {
> > - do
> > - flags = raw_readl(PL011(UARTFR));
> > - while (flags & PL011_UARTFR_FIFO_FULL);
> > + do {
> > + flags = raw_readl(PL011(UARTFR));
> > + } while (flags & PL011_UARTFR_FIFO_FULL);
> > +
> > + raw_writel(c, PL011(UARTDR));
> >
> > - raw_writel(*str++, PL011(UARTDR));
> > + do {
> > + flags = raw_readl(PL011(UARTFR));
> > + } while (flags & PL011_UARTFR_BUSY);
>
> Apologies if that appears to be nitpicking over a totally pointless issue
> (given the nature of the *emulated* PL011 in the model), but:
>
> I understand that this code has not changed, but this loop basically
> renders the FIFOs ineffective. Is this intended?
Hmm... AFAICT, we started to enable the FIFOs in commit:
26a17ad59544f026 ("bootwrapper: improve UART initialisation")
... but back then the boot-wrapper didn't print anything to the UART, so
that was purely about putting the UART into a sane state for the OS.
We only started to write to the UART in commit:
7ff3872adb33b068 ("Rewrite platform initialisation in C")
... where we said nothing about the FIFOs.
Therefore, I think there was no intent either way to use/avoid the
FIFOs within the boot-wrapper.
> I mean we explicitly enable the FIFOs in init_uart(), but then poll
> here until the transmit FIFO becomes empty, after *every* character
> pushed. I see that we probably want the output to be synchronous, for
> debug reasons, but I wonder if this should be achieved via an extra
> uart_flush() routine, once per line output? So call uart_flush() just
> in print_string(), but not in print_char(), for instance?
Hmm... we also use print_char() directly from the aarch64 init code when
logging the exception level, so we'd need an explicit sync there, if we
do want that output before other things can go wrong.
Otherwise, I don't have strong feelings either way, but I would like to
have the output completed before print_*() return. We could add an
internal print_char_nosync() that print_{char,string}() use to handle
that.
I'm happy to take a follow-up patch for that, but for now I'm going to
leave this as-is.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (7 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 08/13] Announce boot-wrapper mode / exception level Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 14:31 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 10/13] aarch32: move the bulk of Secure PL1 " Mark Rutland
` (4 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
The majority of state that we initialize at EL3 is necessary for code at
lower ELs to function, but isnt' necessary for the boot-wrapper itself.
Given that, it would be better to write this in C where it can be
written mode clearly, and where it will be possible to add logging/debug
logic.
This patch migrates the AArch64 EL3 initialization to C.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
Makefile.am | 2 +-
arch/aarch64/boot.S | 92 +---------------------------------
arch/aarch64/include/asm/cpu.h | 36 ++++++++++++-
arch/aarch64/init.c | 69 +++++++++++++++++++++++++
4 files changed, 107 insertions(+), 92 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 885a77c..a598b95 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -153,7 +153,7 @@ $(COMMON_SRC):
%.o: %.S Makefile | $(ARCH_SRC)
$(CC) $(CPPFLAGS) -D__ASSEMBLY__ $(CFLAGS) $(DEFINES) -c -o $@ $<
-%.o: %.c Makefile | $(COMMON_SRC)
+%.o: %.c Makefile | $(ARCH_SRC) $(COMMON_SRC)
$(CC) $(CPPFLAGS) $(CFLAGS) $(DEFINES) -c -o $@ $<
model.lds: $(LD_SCRIPT) Makefile
diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
index 17b4a75..c0ec518 100644
--- a/arch/aarch64/boot.S
+++ b/arch/aarch64/boot.S
@@ -45,96 +45,6 @@ reset_at_el3:
msr sctlr_el3, x0
isb
- mov x0, #0x30 // RES1
- orr x0, x0, #(1 << 0) // Non-secure EL1
- orr x0, x0, #(1 << 8) // HVC enable
-
- /* Enable pointer authentication if present */
- mrs x1, id_aa64isar1_el1
- /* We check for APA+API and GPA+GPI */
- ldr x2, =((0xff << 24) | (0xff << 4))
- and x1, x1, x2
- cbz x1, 1f
-
- orr x0, x0, #(1 << 16) // AP key enable
- orr x0, x0, #(1 << 17) // AP insn enable
-1:
- /* Enable TME if present */
- mrs x1, id_aa64isar0_el1
- ubfx x1, x1, #24, #4
- cbz x1, 1f
-
- orr x0, x0, #(1 << 34) // TME enable
-1:
- /* Enable FGT if present */
- mrs x1, id_aa64mmfr0_el1
- ubfx x1, x1, #56, #4
- cbz x1, 1f
-
- orr x0, x0, #(1 << 27) // FGT enable
-1:
- /* Enable ECV2 if present (allows CNTPOFF_EL2) */
- mrs x1, id_aa64mmfr0_el1
- ubfx x1, x1, #60, #4
- cmp x1, #2
- b.lt 1f
-
- orr x0, x0, #(1 << 28) // ECV enable
-1:
- /* Enable MTE if present */
- mrs x10, id_aa64pfr1_el1
- ubfx x10, x10, #8, #4
- cmp x10, #2
- b.lt 1f
-
- orr x0, x0, #(1 << 26) // ATA enable
-1:
-#ifndef KERNEL_32
- orr x0, x0, #(1 << 10) // 64-bit EL2
-#endif
- msr scr_el3, x0
-
- msr cptr_el3, xzr // Disable copro. traps to EL3
-
- mov x0, xzr
- mrs x1, id_aa64dfr0_el1
- ubfx x1, x1, #32, #4
- cbz x1, 1f
-
- // Enable SPE for the non-secure world.
- orr x0, x0, #(0x3 << 12)
-
- // Do not trap PMSNEVFR_EL1 if present
- cmp x1, #3
- b.lt 1f
- orr x0, x0, #(1 << 36)
-
-1: mrs x1, id_aa64dfr0_el1
- ubfx x1, x1, #44, #4
- cbz x1, 1f
-
- // Enable TRBE for the non-secure world.
- ldr x1, =(0x3 << 24)
- orr x0, x0, x1
-
-1: msr mdcr_el3, x0 // Disable traps to EL3
-
- mrs x0, id_aa64pfr0_el1
- ubfx x0, x0, #32, #4 // SVE present?
- cbz x0, 1f // Skip SVE init if not
-
- mrs x0, cptr_el3
- orr x0, x0, #CPTR_EL3_EZ // enable SVE
- msr cptr_el3, x0
- isb
-
- mov x0, #ZCR_EL3_LEN_MASK // SVE: Enable full vector len
- msr ZCR_EL3, x0 // for EL2.
-
-1:
- ldr x0, =COUNTER_FREQ
- msr cntfrq_el0, x0
-
cpuid x0, x1
bl find_logical_id
cmp x0, #MPIDR_INVALID
@@ -143,6 +53,8 @@ reset_at_el3:
bl cpu_init_bootwrapper
+ bl cpu_init_el3
+
bl gic_secure_init
b start_el3
diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
index 1e9141a..e078ae4 100644
--- a/arch/aarch64/include/asm/cpu.h
+++ b/arch/aarch64/include/asm/cpu.h
@@ -31,7 +31,41 @@
#define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
BIT(11) | BIT(8) | BIT(7) | BIT(4))
-#define HCR_EL2_RES1 (BIT(1))
+#define ZCR_EL3 s3_6_c1_c2_0
+#define ZCR_EL3_LEN BITS(3, 1)
+
+#define MDCR_EL3_NSPB_NS_NOTRAP (UL(3) << 12)
+#define MDCR_EL3_NSTB_NS_NOTRAP (UL(3) << 24)
+#define MDCR_EL3_ENPMSN BIT(36)
+
+#define SCR_EL3_RES1 BITS(5, 4)
+#define SCR_EL3_NS BIT(0)
+#define SCR_EL3_HCE BIT(8)
+#define SCR_EL3_RW BIT(10)
+#define SCR_EL3_APK BIT(16)
+#define SCR_EL3_API BIT(17)
+#define SCR_EL3_ATA BIT(26)
+#define SCR_EL3_FGTEN BIT(27)
+#define SCR_EL3_ECVEN BIT(28)
+#define SCR_EL3_TME BIT(34)
+
+#define HCR_EL2_RES1 BIT(1)
+
+#define ID_AA64DFR0_EL1_PMSVER BITS(35, 32)
+#define ID_AA64DFR0_EL1_TRACEBUFFER BITS(47, 44)
+
+#define ID_AA64ISAR0_EL1_TME BITS(27, 24)
+
+#define ID_AA64ISAR1_EL1_APA BITS(7, 4)
+#define ID_AA64ISAR1_EL1_API BITS(11, 8)
+#define ID_AA64ISAR1_EL1_GPA BITS(27, 24)
+#define ID_AA64ISAR1_EL1_GPI BITS(31, 28)
+
+#define ID_AA64MMFR0_EL1_FGT BITS(59, 56)
+#define ID_AA64MMFR0_EL1_ECV BITS(63, 60)
+
+#define ID_AA64PFR1_EL1_MTE BITS(11, 8)
+#define ID_AA64PFR0_EL1_SVE BITS(35, 32)
/*
* Initial register values required for the boot-wrapper to run out-of-reset.
diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
index 82816e7..ded9871 100644
--- a/arch/aarch64/init.c
+++ b/arch/aarch64/init.c
@@ -8,6 +8,7 @@
*/
#include <cpu.h>
#include <platform.h>
+#include <stdbool.h>
void announce_arch(void)
{
@@ -17,3 +18,71 @@ void announce_arch(void)
print_char('0' + el);
print_string("\r\n");
}
+
+static inline bool kernel_is_32bit(void)
+{
+#ifdef KERNEL_32
+ return true;
+#else
+ return false;
+#endif
+}
+
+static inline bool cpu_has_pauth(void)
+{
+ const unsigned long id_pauth = ID_AA64ISAR1_EL1_APA |
+ ID_AA64ISAR1_EL1_API |
+ ID_AA64ISAR1_EL1_GPA |
+ ID_AA64ISAR1_EL1_GPI;
+
+ return mrs(ID_AA64ISAR1_EL1) & id_pauth;
+}
+
+void cpu_init_el3(void)
+{
+ unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
+ unsigned long mdcr = 0;
+ unsigned long cptr = 0;
+
+ if (cpu_has_pauth())
+ scr |= SCR_EL3_APK | SCR_EL3_API;
+
+ if (mrs_field(ID_AA64ISAR0_EL1, TME))
+ scr |= SCR_EL3_TME;
+
+ if (mrs_field(ID_AA64MMFR0_EL1, FGT))
+ scr |= SCR_EL3_FGTEN;
+
+ if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
+ scr |= SCR_EL3_ECVEN;
+
+ if (mrs_field(ID_AA64PFR1_EL1, MTE))
+ scr |= SCR_EL3_ATA;
+
+ if (!kernel_is_32bit())
+ scr |= SCR_EL3_RW;
+
+ msr(SCR_EL3, scr);
+
+ msr(CPTR_EL3, cptr);
+
+ if (mrs_field(ID_AA64DFR0_EL1, PMSVER))
+ mdcr |= MDCR_EL3_NSPB_NS_NOTRAP;
+
+ if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
+ mdcr |= MDCR_EL3_ENPMSN;
+
+ if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
+ mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
+
+ msr(MDCR_EL3, mdcr);
+
+ if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
+ cptr |= CPTR_EL3_EZ;
+ msr(CPTR_EL3, cptr);
+ isb();
+ msr(ZCR_EL3, ZCR_EL3_LEN);
+ }
+
+ msr(CNTFRQ_EL0, COUNTER_FREQ);
+}
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-14 10:56 ` [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C Mark Rutland
@ 2022-01-17 14:31 ` Andre Przywara
2022-01-17 18:08 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 14:31 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:49 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> The majority of state that we initialize at EL3 is necessary for code at
> lower ELs to function, but isnt' necessary for the boot-wrapper itself.
> Given that, it would be better to write this in C where it can be
> written mode clearly, and where it will be possible to add logging/debug
> logic.
Ah, thanks, that looks much nicer and easier to read now, also is more
robust, as keeping register values alive for more than a few assembly
lines always scares me.
> This patch migrates the AArch64 EL3 initialization to C.
>
> There should be no functional change as a result of this patch.
I compared the removed assembly code against to added C code, and also
checked the register bits against the ARM ARM.
Two (and a half) things stood out, see below:
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> Makefile.am | 2 +-
> arch/aarch64/boot.S | 92 +---------------------------------
> arch/aarch64/include/asm/cpu.h | 36 ++++++++++++-
> arch/aarch64/init.c | 69 +++++++++++++++++++++++++
> 4 files changed, 107 insertions(+), 92 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 885a77c..a598b95 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -153,7 +153,7 @@ $(COMMON_SRC):
> %.o: %.S Makefile | $(ARCH_SRC)
> $(CC) $(CPPFLAGS) -D__ASSEMBLY__ $(CFLAGS) $(DEFINES) -c -o $@ $<
>
> -%.o: %.c Makefile | $(COMMON_SRC)
> +%.o: %.c Makefile | $(ARCH_SRC) $(COMMON_SRC)
> $(CC) $(CPPFLAGS) $(CFLAGS) $(DEFINES) -c -o $@ $<
>
> model.lds: $(LD_SCRIPT) Makefile
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index 17b4a75..c0ec518 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -45,96 +45,6 @@ reset_at_el3:
> msr sctlr_el3, x0
> isb
>
> - mov x0, #0x30 // RES1
> - orr x0, x0, #(1 << 0) // Non-secure EL1
> - orr x0, x0, #(1 << 8) // HVC enable
> -
> - /* Enable pointer authentication if present */
> - mrs x1, id_aa64isar1_el1
> - /* We check for APA+API and GPA+GPI */
> - ldr x2, =((0xff << 24) | (0xff << 4))
> - and x1, x1, x2
> - cbz x1, 1f
> -
> - orr x0, x0, #(1 << 16) // AP key enable
> - orr x0, x0, #(1 << 17) // AP insn enable
> -1:
> - /* Enable TME if present */
> - mrs x1, id_aa64isar0_el1
> - ubfx x1, x1, #24, #4
> - cbz x1, 1f
> -
> - orr x0, x0, #(1 << 34) // TME enable
> -1:
> - /* Enable FGT if present */
> - mrs x1, id_aa64mmfr0_el1
> - ubfx x1, x1, #56, #4
> - cbz x1, 1f
> -
> - orr x0, x0, #(1 << 27) // FGT enable
> -1:
> - /* Enable ECV2 if present (allows CNTPOFF_EL2) */
> - mrs x1, id_aa64mmfr0_el1
> - ubfx x1, x1, #60, #4
> - cmp x1, #2
> - b.lt 1f
> -
> - orr x0, x0, #(1 << 28) // ECV enable
> -1:
> - /* Enable MTE if present */
> - mrs x10, id_aa64pfr1_el1
> - ubfx x10, x10, #8, #4
> - cmp x10, #2
> - b.lt 1f
> -
> - orr x0, x0, #(1 << 26) // ATA enable
> -1:
> -#ifndef KERNEL_32
> - orr x0, x0, #(1 << 10) // 64-bit EL2
> -#endif
> - msr scr_el3, x0
> -
> - msr cptr_el3, xzr // Disable copro. traps to EL3
> -
> - mov x0, xzr
> - mrs x1, id_aa64dfr0_el1
> - ubfx x1, x1, #32, #4
> - cbz x1, 1f
> -
> - // Enable SPE for the non-secure world.
> - orr x0, x0, #(0x3 << 12)
> -
> - // Do not trap PMSNEVFR_EL1 if present
> - cmp x1, #3
> - b.lt 1f
> - orr x0, x0, #(1 << 36)
> -
> -1: mrs x1, id_aa64dfr0_el1
> - ubfx x1, x1, #44, #4
> - cbz x1, 1f
> -
> - // Enable TRBE for the non-secure world.
> - ldr x1, =(0x3 << 24)
> - orr x0, x0, x1
> -
> -1: msr mdcr_el3, x0 // Disable traps to EL3
> -
> - mrs x0, id_aa64pfr0_el1
> - ubfx x0, x0, #32, #4 // SVE present?
> - cbz x0, 1f // Skip SVE init if not
> -
> - mrs x0, cptr_el3
> - orr x0, x0, #CPTR_EL3_EZ // enable SVE
> - msr cptr_el3, x0
> - isb
> -
> - mov x0, #ZCR_EL3_LEN_MASK // SVE: Enable full vector len
> - msr ZCR_EL3, x0 // for EL2.
> -
> -1:
> - ldr x0, =COUNTER_FREQ
> - msr cntfrq_el0, x0
> -
> cpuid x0, x1
> bl find_logical_id
> cmp x0, #MPIDR_INVALID
> @@ -143,6 +53,8 @@ reset_at_el3:
>
> bl cpu_init_bootwrapper
>
> + bl cpu_init_el3
> +
> bl gic_secure_init
>
> b start_el3
> diff --git a/arch/aarch64/include/asm/cpu.h b/arch/aarch64/include/asm/cpu.h
> index 1e9141a..e078ae4 100644
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -31,7 +31,41 @@
> #define SCTLR_EL1_RES1 (BIT(29) | BIT(28) | BIT(23) | BIT(22) | \
> BIT(11) | BIT(8) | BIT(7) | BIT(4))
>
> -#define HCR_EL2_RES1 (BIT(1))
> +#define ZCR_EL3 s3_6_c1_c2_0
> +#define ZCR_EL3_LEN BITS(3, 1)
The (current) actual length field should be BITS(3, 0), no?
> +
> +#define MDCR_EL3_NSPB_NS_NOTRAP (UL(3) << 12)
> +#define MDCR_EL3_NSTB_NS_NOTRAP (UL(3) << 24)
> +#define MDCR_EL3_ENPMSN BIT(36)
> +
> +#define SCR_EL3_RES1 BITS(5, 4)
> +#define SCR_EL3_NS BIT(0)
> +#define SCR_EL3_HCE BIT(8)
> +#define SCR_EL3_RW BIT(10)
> +#define SCR_EL3_APK BIT(16)
> +#define SCR_EL3_API BIT(17)
> +#define SCR_EL3_ATA BIT(26)
> +#define SCR_EL3_FGTEN BIT(27)
> +#define SCR_EL3_ECVEN BIT(28)
> +#define SCR_EL3_TME BIT(34)
> +
> +#define HCR_EL2_RES1 BIT(1)
> +
> +#define ID_AA64DFR0_EL1_PMSVER BITS(35, 32)
> +#define ID_AA64DFR0_EL1_TRACEBUFFER BITS(47, 44)
> +
> +#define ID_AA64ISAR0_EL1_TME BITS(27, 24)
> +
> +#define ID_AA64ISAR1_EL1_APA BITS(7, 4)
> +#define ID_AA64ISAR1_EL1_API BITS(11, 8)
> +#define ID_AA64ISAR1_EL1_GPA BITS(27, 24)
> +#define ID_AA64ISAR1_EL1_GPI BITS(31, 28)
> +
> +#define ID_AA64MMFR0_EL1_FGT BITS(59, 56)
> +#define ID_AA64MMFR0_EL1_ECV BITS(63, 60)
> +
> +#define ID_AA64PFR1_EL1_MTE BITS(11, 8)
> +#define ID_AA64PFR0_EL1_SVE BITS(35, 32)
>
> /*
> * Initial register values required for the boot-wrapper to run out-of-reset.
> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> index 82816e7..ded9871 100644
> --- a/arch/aarch64/init.c
> +++ b/arch/aarch64/init.c
> @@ -8,6 +8,7 @@
> */
> #include <cpu.h>
> #include <platform.h>
> +#include <stdbool.h>
>
> void announce_arch(void)
> {
> @@ -17,3 +18,71 @@ void announce_arch(void)
> print_char('0' + el);
> print_string("\r\n");
> }
> +
> +static inline bool kernel_is_32bit(void)
> +{
> +#ifdef KERNEL_32
> + return true;
> +#else
> + return false;
> +#endif
> +}
> +
> +static inline bool cpu_has_pauth(void)
> +{
> + const unsigned long id_pauth = ID_AA64ISAR1_EL1_APA |
> + ID_AA64ISAR1_EL1_API |
> + ID_AA64ISAR1_EL1_GPA |
> + ID_AA64ISAR1_EL1_GPI;
> +
> + return mrs(ID_AA64ISAR1_EL1) & id_pauth;
> +}
> +
> +void cpu_init_el3(void)
> +{
> + unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
> + unsigned long mdcr = 0;
> + unsigned long cptr = 0;
> +
> + if (cpu_has_pauth())
> + scr |= SCR_EL3_APK | SCR_EL3_API;
> +
> + if (mrs_field(ID_AA64ISAR0_EL1, TME))
> + scr |= SCR_EL3_TME;
> +
> + if (mrs_field(ID_AA64MMFR0_EL1, FGT))
> + scr |= SCR_EL3_FGTEN;
> +
> + if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
> + scr |= SCR_EL3_ECVEN;
> +
> + if (mrs_field(ID_AA64PFR1_EL1, MTE))
The assembly code checked for >=2, which seems correct to me, as
SCR_EL3_ATA is about MTE2?
> + scr |= SCR_EL3_ATA;
> +
> + if (!kernel_is_32bit())
> + scr |= SCR_EL3_RW;
> +
> + msr(SCR_EL3, scr);
> +
> + msr(CPTR_EL3, cptr);
> +
> + if (mrs_field(ID_AA64DFR0_EL1, PMSVER))
> + mdcr |= MDCR_EL3_NSPB_NS_NOTRAP;
> +
> + if (mrs_field(ID_AA64DFR0_EL1, PMSVER) >= 3)
> + mdcr |= MDCR_EL3_ENPMSN;
> +
> + if (mrs_field(ID_AA64DFR0_EL1, TRACEBUFFER))
> + mdcr |= MDCR_EL3_NSTB_NS_NOTRAP;
> +
> + msr(MDCR_EL3, mdcr);
> +
> + if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
> + cptr |= CPTR_EL3_EZ;
> + msr(CPTR_EL3, cptr);
> + isb();
> + msr(ZCR_EL3, ZCR_EL3_LEN);
So when comparing this to the other uses of XXX_EL3_YYY, they typically
describe a mask, but here we seems to abuse this as a value? And apart
from bit 0 missing from it (as noted above), the existing code writes 0x1ff
into that register, presumable to cover future vector length extensions
beyond 2048 bits (which those RAZ/WI fields in bits[8:4] seem to suggest).
So shall we define ZCR_EL3_MAX_VEC_LEN to 0x1ff above, and then use that?
Or ignore the crystal ball, and just stick with 2048 bits, by writing 0xf?
The rest passed the checks.
Cheers,
Andre
> + }
> +
> + msr(CNTFRQ_EL0, COUNTER_FREQ);
> +}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-17 14:31 ` Andre Przywara
@ 2022-01-17 18:08 ` Mark Rutland
2022-01-17 18:31 ` Andre Przywara
0 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-17 18:08 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 02:31:04PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:49 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
> > The majority of state that we initialize at EL3 is necessary for code at
> > lower ELs to function, but isnt' necessary for the boot-wrapper itself.
> > Given that, it would be better to write this in C where it can be
> > written mode clearly, and where it will be possible to add logging/debug
> > logic.
>
> Ah, thanks, that looks much nicer and easier to read now, also is more
> robust, as keeping register values alive for more than a few assembly
> lines always scares me.
>
> > This patch migrates the AArch64 EL3 initialization to C.
> >
> > There should be no functional change as a result of this patch.
>
> I compared the removed assembly code against to added C code, and also
> checked the register bits against the ARM ARM.
> Two (and a half) things stood out, see below:
Thanks for this! I've fixed those as noted below.
[...]
> > -#define HCR_EL2_RES1 (BIT(1))
> > +#define ZCR_EL3 s3_6_c1_c2_0
> > +#define ZCR_EL3_LEN BITS(3, 1)
>
> The (current) actual length field should be BITS(3, 0), no?
Yes, it should. I've corrected that to BITS(3, 0) now.
[...]
> > +void cpu_init_el3(void)
> > +{
> > + unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
> > + unsigned long mdcr = 0;
> > + unsigned long cptr = 0;
> > +
> > + if (cpu_has_pauth())
> > + scr |= SCR_EL3_APK | SCR_EL3_API;
> > +
> > + if (mrs_field(ID_AA64ISAR0_EL1, TME))
> > + scr |= SCR_EL3_TME;
> > +
> > + if (mrs_field(ID_AA64MMFR0_EL1, FGT))
> > + scr |= SCR_EL3_FGTEN;
> > +
> > + if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
> > + scr |= SCR_EL3_ECVEN;
> > +
> > + if (mrs_field(ID_AA64PFR1_EL1, MTE))
>
> The assembly code checked for >=2, which seems correct to me, as
> SCR_EL3_ATA is about MTE2?
Yes; I botched that when converting to C. SCR_EL3.ATA is RES0 in the
absence of MTE2. I've added `>= 2` to the condition to match that.
> > + scr |= SCR_EL3_ATA;
[...]
> > + if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
> > + cptr |= CPTR_EL3_EZ;
> > + msr(CPTR_EL3, cptr);
> > + isb();
> > + msr(ZCR_EL3, ZCR_EL3_LEN);
>
> So when comparing this to the other uses of XXX_EL3_YYY, they typically
> describe a mask, but here we seems to abuse this as a value?
True; I'll add a separate defintion for the value.
> And apart from bit 0 missing from it (as noted above), the existing
> code writes 0x1ff into that register, presumable to cover future
> vector length extensions beyond 2048 bits (which those RAZ/WI fields
> in bits[8:4] seem to suggest).
Hmm... I went and found the SVE supplement and I can't see any rationale
for what SW *should* do, nor can I find a description of the register
(that seems to have been factored into some XML files I can't convince
anything to load on my machine).
> So shall we define ZCR_EL3_MAX_VEC_LEN to 0x1ff above, and then use that?
> Or ignore the crystal ball, and just stick with 2048 bits, by writing 0xf?
TBH, I'm not sure. In the absence of some documented guidance I'd prefer
to go with 0xf, but given we already use 0x1ff, I want to dig into this
a bit more.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-17 18:08 ` Mark Rutland
@ 2022-01-17 18:31 ` Andre Przywara
2022-01-18 16:50 ` Mark Brown
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 18:31 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin,
Wei.Chen, Mark Brown
On Mon, 17 Jan 2022 18:08:13 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi,
> On Mon, Jan 17, 2022 at 02:31:04PM +0000, Andre Przywara wrote:
> > On Fri, 14 Jan 2022 10:56:49 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Mark,
> >
> > > The majority of state that we initialize at EL3 is necessary for code at
> > > lower ELs to function, but isnt' necessary for the boot-wrapper itself.
> > > Given that, it would be better to write this in C where it can be
> > > written mode clearly, and where it will be possible to add logging/debug
> > > logic.
> >
> > Ah, thanks, that looks much nicer and easier to read now, also is more
> > robust, as keeping register values alive for more than a few assembly
> > lines always scares me.
> >
> > > This patch migrates the AArch64 EL3 initialization to C.
> > >
> > > There should be no functional change as a result of this patch.
> >
> > I compared the removed assembly code against to added C code, and also
> > checked the register bits against the ARM ARM.
> > Two (and a half) things stood out, see below:
>
> Thanks for this! I've fixed those as noted below.
>
> [...]
>
> > > -#define HCR_EL2_RES1 (BIT(1))
> > > +#define ZCR_EL3 s3_6_c1_c2_0
> > > +#define ZCR_EL3_LEN BITS(3, 1)
> >
> > The (current) actual length field should be BITS(3, 0), no?
>
> Yes, it should. I've corrected that to BITS(3, 0) now.
>
> [...]
>
> > > +void cpu_init_el3(void)
> > > +{
> > > + unsigned long scr = SCR_EL3_RES1 | SCR_EL3_NS | SCR_EL3_HCE;
> > > + unsigned long mdcr = 0;
> > > + unsigned long cptr = 0;
> > > +
> > > + if (cpu_has_pauth())
> > > + scr |= SCR_EL3_APK | SCR_EL3_API;
> > > +
> > > + if (mrs_field(ID_AA64ISAR0_EL1, TME))
> > > + scr |= SCR_EL3_TME;
> > > +
> > > + if (mrs_field(ID_AA64MMFR0_EL1, FGT))
> > > + scr |= SCR_EL3_FGTEN;
> > > +
> > > + if (mrs_field(ID_AA64MMFR0_EL1, ECV) >= 2)
> > > + scr |= SCR_EL3_ECVEN;
> > > +
> > > + if (mrs_field(ID_AA64PFR1_EL1, MTE))
> >
> > The assembly code checked for >=2, which seems correct to me, as
> > SCR_EL3_ATA is about MTE2?
>
> Yes; I botched that when converting to C. SCR_EL3.ATA is RES0 in the
> absence of MTE2. I've added `>= 2` to the condition to match that.
>
> > > + scr |= SCR_EL3_ATA;
>
> [...]
>
> > > + if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
> > > + cptr |= CPTR_EL3_EZ;
> > > + msr(CPTR_EL3, cptr);
> > > + isb();
> > > + msr(ZCR_EL3, ZCR_EL3_LEN);
> >
> > So when comparing this to the other uses of XXX_EL3_YYY, they typically
> > describe a mask, but here we seems to abuse this as a value?
>
> True; I'll add a separate defintion for the value.
>
> > And apart from bit 0 missing from it (as noted above), the existing
> > code writes 0x1ff into that register, presumable to cover future
> > vector length extensions beyond 2048 bits (which those RAZ/WI fields
> > in bits[8:4] seem to suggest).
>
> Hmm... I went and found the SVE supplement and I can't see any rationale
> for what SW *should* do, nor can I find a description of the register
> (that seems to have been factored into some XML files I can't convince
> anything to load on my machine).
>
> > So shall we define ZCR_EL3_MAX_VEC_LEN to 0x1ff above, and then use that?
> > Or ignore the crystal ball, and just stick with 2048 bits, by writing 0xf?
>
> TBH, I'm not sure. In the absence of some documented guidance I'd prefer
> to go with 0xf, but given we already use 0x1ff, I want to dig into this
> a bit more.
My impression was that this "[8:4] = RAZ/WI" compared to the "[63:9] =
RES0" fields suggests this is for a potential extension, but I guess there
would be more changes needed if SVE ever goes beyond 2048. So chances are
high we need to adopt the code then anyway, and fixing the number then is
the least of our problems.
So I feel we should stick to what's explicitly documented, and put 0xf in
there.
Cheers,
Andre
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-17 18:31 ` Andre Przywara
@ 2022-01-18 16:50 ` Mark Brown
2022-01-19 15:22 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Mark Brown @ 2022-01-18 16:50 UTC (permalink / raw)
To: Andre Przywara
Cc: Mark Rutland, linux-arm-kernel, Jaxson.Han, robin.murphy,
vladimir.murzin, Wei.Chen
[-- Attachment #1.1: Type: text/plain, Size: 2116 bytes --]
On Mon, Jan 17, 2022 at 06:31:17PM +0000, Andre Przywara wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Jan 17, 2022 at 02:31:04PM +0000, Andre Przywara wrote:
> > > On Fri, 14 Jan 2022 10:56:49 +0000
> > > Mark Rutland <mark.rutland@arm.com> wrote:
[Out of office this week so replies might be intermittent]
> > > And apart from bit 0 missing from it (as noted above), the existing
> > > code writes 0x1ff into that register, presumable to cover future
> > > vector length extensions beyond 2048 bits (which those RAZ/WI fields
> > > in bits[8:4] seem to suggest).
> > Hmm... I went and found the SVE supplement and I can't see any rationale
> > for what SW *should* do, nor can I find a description of the register
> > (that seems to have been factored into some XML files I can't convince
> > anything to load on my machine).
...
> > TBH, I'm not sure. In the absence of some documented guidance I'd prefer
> > to go with 0xf, but given we already use 0x1ff, I want to dig into this
> > a bit more.
I'm fairly sure I've seen some explicit discussion of this lurking
somewhere, though I couldn't tell you where - DDI0584 A.i doesn't spell
out the enumeration algorithm unfortunately AFAICT. The theory is that
the extra bits are reserved for any future extension of the vector
length if needed since it'd be inconvenient to have to split the vector
length field up.
> My impression was that this "[8:4] = RAZ/WI" compared to the "[63:9] =
> RES0" fields suggests this is for a potential extension, but I guess there
> would be more changes needed if SVE ever goes beyond 2048. So chances are
> high we need to adopt the code then anyway, and fixing the number then is
> the least of our problems.
> So I feel we should stick to what's explicitly documented, and put 0xf in
> there.
We shouldn't need particularly many changes if the size of the field
ever gets raised, with everything being dynamically sized already and
the existing code starting off setting the WI bits to 1 the updates that
are needed should just be on input validation.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C
2022-01-18 16:50 ` Mark Brown
@ 2022-01-19 15:22 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-19 15:22 UTC (permalink / raw)
To: Mark Brown
Cc: Andre Przywara, linux-arm-kernel, Jaxson.Han, robin.murphy,
vladimir.murzin, Wei.Chen
On Tue, Jan 18, 2022 at 04:50:12PM +0000, Mark Brown wrote:
> On Mon, Jan 17, 2022 at 06:31:17PM +0000, Andre Przywara wrote:
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Mon, Jan 17, 2022 at 02:31:04PM +0000, Andre Przywara wrote:
> > > > On Fri, 14 Jan 2022 10:56:49 +0000
> > > > Mark Rutland <mark.rutland@arm.com> wrote:
>
> [Out of office this week so replies might be intermittent]
>
> > > > And apart from bit 0 missing from it (as noted above), the existing
> > > > code writes 0x1ff into that register, presumable to cover future
> > > > vector length extensions beyond 2048 bits (which those RAZ/WI fields
> > > > in bits[8:4] seem to suggest).
>
> > > Hmm... I went and found the SVE supplement and I can't see any rationale
> > > for what SW *should* do, nor can I find a description of the register
> > > (that seems to have been factored into some XML files I can't convince
> > > anything to load on my machine).
>
> ...
>
> > > TBH, I'm not sure. In the absence of some documented guidance I'd prefer
> > > to go with 0xf, but given we already use 0x1ff, I want to dig into this
> > > a bit more.
>
> I'm fairly sure I've seen some explicit discussion of this lurking
> somewhere, though I couldn't tell you where - DDI0584 A.i doesn't spell
> out the enumeration algorithm unfortunately AFAICT. The theory is that
> the extra bits are reserved for any future extension of the vector
> length if needed since it'd be inconvenient to have to split the vector
> length field up.
>
> > My impression was that this "[8:4] = RAZ/WI" compared to the "[63:9] =
> > RES0" fields suggests this is for a potential extension, but I guess there
> > would be more changes needed if SVE ever goes beyond 2048. So chances are
> > high we need to adopt the code then anyway, and fixing the number then is
> > the least of our problems.
>
> > So I feel we should stick to what's explicitly documented, and put 0xf in
> > there.
>
> We shouldn't need particularly many changes if the size of the field
> ever gets raised, with everything being dynamically sized already and
> the existing code starting off setting the WI bits to 1 the updates that
> are needed should just be on input validation.
Taking a look around, TF-A programs 0xf, and for consistency I think
it'd be clearer to only program the currently-allocated LEN bits.
I'll spin a preparatory patch reducing ZCR_EL3_LEN_MASK to 0xf (and
renaming that to ZCR_EL3_LEN_MAX).
If we need to grow it in future it should be trivial to do so.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 10/13] aarch32: move the bulk of Secure PL1 initialization to C
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (8 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 09/13] aarch64: move the bulk of EL3 initialization to C Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 14:52 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 11/13] Announce locations of memory objects Mark Rutland
` (3 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
The majority of state that we initialize at Secure PL1 is necessary for
code at lower PLs to function, but isnt' necessary for the boot-wrapper
itself. Given that, it would be better to write this in C where it can
be written mode clearly, and where it will be possible to add
logging/debug logic.
This patch migrates the AArch32 Secure PL1 initialization to C.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch32/boot.S | 11 +----------
arch/aarch32/include/asm/cpu.h | 9 +++++++++
arch/aarch32/init.c | 12 ++++++++++++
3 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
index ee073ea..820957b 100644
--- a/arch/aarch32/boot.S
+++ b/arch/aarch32/boot.S
@@ -63,16 +63,7 @@ _monitor:
/* Move the stack to Monitor mode*/
mrs sp, sp_svc
- /* Setup secure registers and devices */
- mov r0, #1 @ Non-secure lower level
- orr r0, #(1 << 8) @ HVC enable
- mcr p15, 0, r0, c1, c1, 0 @ SCR
-
- mov r0, #(1 << 10 | 1 << 11) @ Enable NS access to CPACR
- mcr p15, 0, r0, c1, c1, 2 @ NSACR
-
- ldr r0, =COUNTER_FREQ
- mcr p15, 0, r0, c14, c0, 0 @ CNTFRQ
+ bl cpu_init_secure_pl1
bl cpu_init_bootwrapper
diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
index aa72204..c1bce9a 100644
--- a/arch/aarch32/include/asm/cpu.h
+++ b/arch/aarch32/include/asm/cpu.h
@@ -30,6 +30,11 @@
#define PSR_I (1 << 7)
#define PSR_A (1 << 8)
+#define SCR_NS BIT(0)
+#define SCR_HCE BIT(8)
+
+#define NSACR_CP10 BIT(10)
+#define NSACR_CP11 BIT(11)
#define SPSR_KERNEL (PSR_A | PSR_I | PSR_F | PSR_HYP)
@@ -55,11 +60,15 @@ static inline unsigned long read_cpsr(void)
#define MPIDR "p15, 0, %0, c0, c0, 5"
#define ID_PFR1 "p15, 0, %0, c0, c1, 1"
+#define SCR "p15, 0, %0, c1, c1, 0"
+#define NSACR "p15, 0, %0, c1, c1, 2"
#define ICIALLU "p15, 0, %0, c7, c5, 0"
#define ICC_SRE "p15, 6, %0, c12, c12, 5"
#define ICC_CTLR "p15, 6, %0, c12, c12, 4"
+#define CNTFRQ "p15, 0, %0, c14, c0, 0"
+
#define mrc(reg) \
({ \
unsigned long __mrc_val; \
diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c
index b29ebb4..5b69dcd 100644
--- a/arch/aarch32/init.c
+++ b/arch/aarch32/init.c
@@ -28,3 +28,15 @@ void announce_arch(void)
print_string(mode_string());
print_string("\r\n");
}
+
+void cpu_init_secure_pl1(void)
+{
+ unsigned long scr = SCR_NS | SCR_HCE;
+ unsigned long nsacr = NSACR_CP10 | NSACR_CP11;
+
+ mcr(SCR, scr);
+
+ mcr(NSACR, nsacr);
+
+ mcr(CNTFRQ, COUNTER_FREQ);
+}
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 10/13] aarch32: move the bulk of Secure PL1 initialization to C
2022-01-14 10:56 ` [bootwrapper PATCH v2 10/13] aarch32: move the bulk of Secure PL1 " Mark Rutland
@ 2022-01-17 14:52 ` Andre Przywara
0 siblings, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 14:52 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:50 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi,
> The majority of state that we initialize at Secure PL1 is necessary for
> code at lower PLs to function, but isnt' necessary for the boot-wrapper
> itself. Given that, it would be better to write this in C where it can
> be written mode clearly, and where it will be possible to add
> logging/debug logic.
>
> This patch migrates the AArch32 Secure PL1 initialization to C.
>
> There should be no functional change as a result of this patch.
I compared the removed assembly code against to added C code, and also
checked the register bits against the ARMv7 ARM.
Everything checks out, so:
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> arch/aarch32/boot.S | 11 +----------
> arch/aarch32/include/asm/cpu.h | 9 +++++++++
> arch/aarch32/init.c | 12 ++++++++++++
> 3 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> index ee073ea..820957b 100644
> --- a/arch/aarch32/boot.S
> +++ b/arch/aarch32/boot.S
> @@ -63,16 +63,7 @@ _monitor:
> /* Move the stack to Monitor mode*/
> mrs sp, sp_svc
>
> - /* Setup secure registers and devices */
> - mov r0, #1 @ Non-secure lower level
> - orr r0, #(1 << 8) @ HVC enable
> - mcr p15, 0, r0, c1, c1, 0 @ SCR
> -
> - mov r0, #(1 << 10 | 1 << 11) @ Enable NS access to CPACR
> - mcr p15, 0, r0, c1, c1, 2 @ NSACR
> -
> - ldr r0, =COUNTER_FREQ
> - mcr p15, 0, r0, c14, c0, 0 @ CNTFRQ
> + bl cpu_init_secure_pl1
>
> bl cpu_init_bootwrapper
>
> diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
> index aa72204..c1bce9a 100644
> --- a/arch/aarch32/include/asm/cpu.h
> +++ b/arch/aarch32/include/asm/cpu.h
> @@ -30,6 +30,11 @@
> #define PSR_I (1 << 7)
> #define PSR_A (1 << 8)
>
> +#define SCR_NS BIT(0)
> +#define SCR_HCE BIT(8)
> +
> +#define NSACR_CP10 BIT(10)
> +#define NSACR_CP11 BIT(11)
>
> #define SPSR_KERNEL (PSR_A | PSR_I | PSR_F | PSR_HYP)
>
> @@ -55,11 +60,15 @@ static inline unsigned long read_cpsr(void)
>
> #define MPIDR "p15, 0, %0, c0, c0, 5"
> #define ID_PFR1 "p15, 0, %0, c0, c1, 1"
> +#define SCR "p15, 0, %0, c1, c1, 0"
> +#define NSACR "p15, 0, %0, c1, c1, 2"
> #define ICIALLU "p15, 0, %0, c7, c5, 0"
>
> #define ICC_SRE "p15, 6, %0, c12, c12, 5"
> #define ICC_CTLR "p15, 6, %0, c12, c12, 4"
>
> +#define CNTFRQ "p15, 0, %0, c14, c0, 0"
> +
> #define mrc(reg) \
> ({ \
> unsigned long __mrc_val; \
> diff --git a/arch/aarch32/init.c b/arch/aarch32/init.c
> index b29ebb4..5b69dcd 100644
> --- a/arch/aarch32/init.c
> +++ b/arch/aarch32/init.c
> @@ -28,3 +28,15 @@ void announce_arch(void)
> print_string(mode_string());
> print_string("\r\n");
> }
> +
> +void cpu_init_secure_pl1(void)
> +{
> + unsigned long scr = SCR_NS | SCR_HCE;
> + unsigned long nsacr = NSACR_CP10 | NSACR_CP11;
> +
> + mcr(SCR, scr);
> +
> + mcr(NSACR, nsacr);
> +
> + mcr(CNTFRQ, COUNTER_FREQ);
> +}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (9 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 10/13] aarch32: move the bulk of Secure PL1 " Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-14 15:30 ` Andre Przywara
2022-01-17 14:59 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 12/13] Rework bootmethod initialization Mark Rutland
` (2 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
To make it easier to debug boot failures, log the location of memory
objects at boot time.
This is logged to the serial console as:
| Boot-wrapper v0.2
| Entered at EL3
| Memory layout:
| [0000000080000000..0000000080001f90] => boot-wrapper
| [000000008000fff8..0000000080010000] => mbox
| [0000000080200000..00000000822af200] => kernel
| [0000000088000000..0000000088002857] => dtb
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
common/init.c | 27 +++++++++++++++++++++++++++
common/platform.c | 13 +++++++++++++
include/platform.h | 2 ++
model.lds.S | 20 ++++++++++++++------
4 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/common/init.c b/common/init.c
index 2600f73..fc74b9e 100644
--- a/common/init.c
+++ b/common/init.c
@@ -14,6 +14,32 @@ static void announce_bootwrapper(void)
print_string("Boot-wrapper v0.2\r\n");
}
+#define announce_object(object, desc) \
+do { \
+ extern char object##__start[]; \
+ extern char object##__end[]; \
+ print_string("["); \
+ print_ulong_hex((unsigned long)object##__start); \
+ print_string(".."); \
+ print_ulong_hex((unsigned long)object##__end); \
+ print_string("] => " desc "\r\n"); \
+} while (0)
+
+static void announce_objects(void)
+{
+ print_string("Memory layout:\r\n");
+ announce_object(text, "boot-wrapper");
+ announce_object(mbox, "mbox");
+ announce_object(kernel, "kernel");
+#ifdef XEN
+ announce_object(xen, "xen");
+#endif
+ announce_object(dtb, "dtb");
+#ifdef USE_INITRD
+ announce_object(filesystem, "initrd");
+#endif
+}
+
void announce_arch(void);
void cpu_init_bootwrapper(void)
@@ -22,6 +48,7 @@ void cpu_init_bootwrapper(void)
init_uart();
announce_bootwrapper();
announce_arch();
+ announce_objects();
print_string("\r\n");
init_platform();
}
diff --git a/common/platform.c b/common/platform.c
index 80d0562..26e5944 100644
--- a/common/platform.c
+++ b/common/platform.c
@@ -52,6 +52,19 @@ void print_string(const char *str)
print_char(*str++);
}
+#define HEX_CHARS_PER_LONG (2 * sizeof(long))
+
+void print_ulong_hex(unsigned long val)
+{
+ const char hex_chars[16] = "0123456789abcdef";
+ int i;
+
+ for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
+ int v = (val >> (4 * i)) & 0xf;
+ print_char(hex_chars[v]);
+ }
+}
+
void init_uart(void)
{
/*
diff --git a/include/platform.h b/include/platform.h
index 237b481..c88e124 100644
--- a/include/platform.h
+++ b/include/platform.h
@@ -11,6 +11,8 @@
void print_char(char c);
void print_string(const char *str);
+void print_ulong_hex(unsigned long val);
+
void init_uart(void);
void init_platform(void);
diff --git a/model.lds.S b/model.lds.S
index d4e7e13..dacaa25 100644
--- a/model.lds.S
+++ b/model.lds.S
@@ -35,46 +35,54 @@ SECTIONS
* the boot section's *(.data)
*/
.kernel (PHYS_OFFSET + KERNEL_OFFSET): {
- kernel = .;
+ kernel__start = .;
KERNEL
+ kernel__end = .;
}
#ifdef XEN
.xen (PHYS_OFFSET + XEN_OFFSET): {
- xen = .;
+ xen__start = .;
XEN
+ xen__end = .;
}
- entrypoint = xen;
+ entrypoint = xen__start;
#else
- entrypoint = kernel;
+ entrypoint = kernel__start;
#endif
.dtb (PHYS_OFFSET + FDT_OFFSET): {
+ dtb__start = .;
dtb = .;
./fdt.dtb
+ dtb__end = .;
}
#ifdef USE_INITRD
.filesystem (PHYS_OFFSET + FS_OFFSET): {
- filesystem = .;
+ filesystem__start = .;
FILESYSTEM
- fs_size = . - filesystem;
+ filesystem__end = .;
}
#endif
.boot PHYS_OFFSET: {
+ text__start = .;
*(.init)
*(.text*)
*(.data* .rodata* .bss* COMMON)
*(.vectors)
*(.stack)
PROVIDE(etext = .);
+ text__end = .;
}
.mbox (PHYS_OFFSET + MBOX_OFFSET): {
+ mbox__start = .;
mbox = .;
QUAD(0x0)
+ mbox__end = .;
}
ASSERT(etext <= (PHYS_OFFSET + TEXT_LIMIT), ".text overflow!")
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 10:56 ` [bootwrapper PATCH v2 11/13] Announce locations of memory objects Mark Rutland
@ 2022-01-14 15:30 ` Andre Przywara
2022-01-14 16:04 ` Robin Murphy
2022-01-14 16:21 ` Mark Rutland
2022-01-17 14:59 ` Andre Przywara
1 sibling, 2 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-14 15:30 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:51 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> To make it easier to debug boot failures, log the location of memory
> objects at boot time.
>
> This is logged to the serial console as:
>
> | Boot-wrapper v0.2
> | Entered at EL3
> | Memory layout:
> | [0000000080000000..0000000080001f90] => boot-wrapper
> | [000000008000fff8..0000000080010000] => mbox
> | [0000000080200000..00000000822af200] => kernel
> | [0000000088000000..0000000088002857] => dtb
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> common/init.c | 27 +++++++++++++++++++++++++++
> common/platform.c | 13 +++++++++++++
> include/platform.h | 2 ++
> model.lds.S | 20 ++++++++++++++------
> 4 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/common/init.c b/common/init.c
> index 2600f73..fc74b9e 100644
> --- a/common/init.c
> +++ b/common/init.c
> @@ -14,6 +14,32 @@ static void announce_bootwrapper(void)
> print_string("Boot-wrapper v0.2\r\n");
> }
>
> +#define announce_object(object, desc) \
> +do { \
> + extern char object##__start[]; \
> + extern char object##__end[]; \
> + print_string("["); \
> + print_ulong_hex((unsigned long)object##__start); \
> + print_string(".."); \
> + print_ulong_hex((unsigned long)object##__end); \
> + print_string("] => " desc "\r\n"); \
> +} while (0)
> +
> +static void announce_objects(void)
> +{
> + print_string("Memory layout:\r\n");
> + announce_object(text, "boot-wrapper");
> + announce_object(mbox, "mbox");
> + announce_object(kernel, "kernel");
> +#ifdef XEN
> + announce_object(xen, "xen");
> +#endif
> + announce_object(dtb, "dtb");
> +#ifdef USE_INITRD
> + announce_object(filesystem, "initrd");
> +#endif
> +}
> +
> void announce_arch(void);
>
> void cpu_init_bootwrapper(void)
> @@ -22,6 +48,7 @@ void cpu_init_bootwrapper(void)
> init_uart();
> announce_bootwrapper();
> announce_arch();
> + announce_objects();
> print_string("\r\n");
> init_platform();
> }
> diff --git a/common/platform.c b/common/platform.c
> index 80d0562..26e5944 100644
> --- a/common/platform.c
> +++ b/common/platform.c
> @@ -52,6 +52,19 @@ void print_string(const char *str)
> print_char(*str++);
> }
>
> +#define HEX_CHARS_PER_LONG (2 * sizeof(long))
> +
> +void print_ulong_hex(unsigned long val)
> +{
> + const char hex_chars[16] = "0123456789abcdef";
This breaks the build for me (I guess because of the const?):
-------------------
ld --gc-sections arch/aarch64/boot.o arch/aarch64/stack.o arch/aarch64/utils.o arch/aarch64/init.o arch/aarch64/psci.o common/boot.o common/bakery_lock.o common/platform.o common/lib.o common/init.o common/psci.o common/gic-v3.o -o linux-system.axf --script=model.lds
ld: common/platform.o: in function `print_ulong_hex':
/src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
ld: /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
ld: /src/boot-wrapper-aarch64.git/common/platform.c:66: undefined reference to `__stack_chk_fail'
/src/boot-wrapper-aarch64.git/common/platform.c:66:(.text.print_ulong_hex+0xa0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `__stack_chk_fail'
make: *** [Makefile:684: linux-system.axf] Error 1
------------------
Adding -fno-stack-protector fixes it again.
I am still checking on each one of those compiler options, shall I send a
smaller version of that patch 3/9 of mine, meanwhile, just carrying the
uncontested -ffreestanding and -nostdlib, plus -fno-stack-protector?
Cheers,
Andre
> + int i;
> +
> + for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
> + int v = (val >> (4 * i)) & 0xf;
> + print_char(hex_chars[v]);
> + }
> +}
> +
> void init_uart(void)
> {
> /*
> diff --git a/include/platform.h b/include/platform.h
> index 237b481..c88e124 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -11,6 +11,8 @@
>
> void print_char(char c);
> void print_string(const char *str);
> +void print_ulong_hex(unsigned long val);
> +
> void init_uart(void);
>
> void init_platform(void);
> diff --git a/model.lds.S b/model.lds.S
> index d4e7e13..dacaa25 100644
> --- a/model.lds.S
> +++ b/model.lds.S
> @@ -35,46 +35,54 @@ SECTIONS
> * the boot section's *(.data)
> */
> .kernel (PHYS_OFFSET + KERNEL_OFFSET): {
> - kernel = .;
> + kernel__start = .;
> KERNEL
> + kernel__end = .;
> }
>
> #ifdef XEN
> .xen (PHYS_OFFSET + XEN_OFFSET): {
> - xen = .;
> + xen__start = .;
> XEN
> + xen__end = .;
> }
>
> - entrypoint = xen;
> + entrypoint = xen__start;
> #else
> - entrypoint = kernel;
> + entrypoint = kernel__start;
> #endif
>
> .dtb (PHYS_OFFSET + FDT_OFFSET): {
> + dtb__start = .;
> dtb = .;
> ./fdt.dtb
> + dtb__end = .;
> }
>
> #ifdef USE_INITRD
> .filesystem (PHYS_OFFSET + FS_OFFSET): {
> - filesystem = .;
> + filesystem__start = .;
> FILESYSTEM
> - fs_size = . - filesystem;
> + filesystem__end = .;
> }
> #endif
>
> .boot PHYS_OFFSET: {
> + text__start = .;
> *(.init)
> *(.text*)
> *(.data* .rodata* .bss* COMMON)
> *(.vectors)
> *(.stack)
> PROVIDE(etext = .);
> + text__end = .;
> }
>
> .mbox (PHYS_OFFSET + MBOX_OFFSET): {
> + mbox__start = .;
> mbox = .;
> QUAD(0x0)
> + mbox__end = .;
> }
>
> ASSERT(etext <= (PHYS_OFFSET + TEXT_LIMIT), ".text overflow!")
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 15:30 ` Andre Przywara
@ 2022-01-14 16:04 ` Robin Murphy
2022-01-14 16:30 ` Mark Rutland
2022-01-14 16:21 ` Mark Rutland
1 sibling, 1 reply; 43+ messages in thread
From: Robin Murphy @ 2022-01-14 16:04 UTC (permalink / raw)
To: Andre Przywara, Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, vladimir.murzin, Wei.Chen
On 2022-01-14 15:30, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:51 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
>> To make it easier to debug boot failures, log the location of memory
>> objects at boot time.
>>
>> This is logged to the serial console as:
>>
>> | Boot-wrapper v0.2
>> | Entered at EL3
>> | Memory layout:
>> | [0000000080000000..0000000080001f90] => boot-wrapper
>> | [000000008000fff8..0000000080010000] => mbox
>> | [0000000080200000..00000000822af200] => kernel
>> | [0000000088000000..0000000088002857] => dtb
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> ---
>> common/init.c | 27 +++++++++++++++++++++++++++
>> common/platform.c | 13 +++++++++++++
>> include/platform.h | 2 ++
>> model.lds.S | 20 ++++++++++++++------
>> 4 files changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/init.c b/common/init.c
>> index 2600f73..fc74b9e 100644
>> --- a/common/init.c
>> +++ b/common/init.c
>> @@ -14,6 +14,32 @@ static void announce_bootwrapper(void)
>> print_string("Boot-wrapper v0.2\r\n");
>> }
>>
>> +#define announce_object(object, desc) \
>> +do { \
>> + extern char object##__start[]; \
>> + extern char object##__end[]; \
>> + print_string("["); \
>> + print_ulong_hex((unsigned long)object##__start); \
>> + print_string(".."); \
>> + print_ulong_hex((unsigned long)object##__end); \
>> + print_string("] => " desc "\r\n"); \
>> +} while (0)
>> +
>> +static void announce_objects(void)
>> +{
>> + print_string("Memory layout:\r\n");
>> + announce_object(text, "boot-wrapper");
>> + announce_object(mbox, "mbox");
>> + announce_object(kernel, "kernel");
>> +#ifdef XEN
>> + announce_object(xen, "xen");
>> +#endif
>> + announce_object(dtb, "dtb");
>> +#ifdef USE_INITRD
>> + announce_object(filesystem, "initrd");
>> +#endif
>> +}
>> +
>> void announce_arch(void);
>>
>> void cpu_init_bootwrapper(void)
>> @@ -22,6 +48,7 @@ void cpu_init_bootwrapper(void)
>> init_uart();
>> announce_bootwrapper();
>> announce_arch();
>> + announce_objects();
>> print_string("\r\n");
>> init_platform();
>> }
>> diff --git a/common/platform.c b/common/platform.c
>> index 80d0562..26e5944 100644
>> --- a/common/platform.c
>> +++ b/common/platform.c
>> @@ -52,6 +52,19 @@ void print_string(const char *str)
>> print_char(*str++);
>> }
>>
>> +#define HEX_CHARS_PER_LONG (2 * sizeof(long))
>> +
>> +void print_ulong_hex(unsigned long val)
>> +{
>> + const char hex_chars[16] = "0123456789abcdef";
>
> This breaks the build for me (I guess because of the const?):
> -------------------
> ld --gc-sections arch/aarch64/boot.o arch/aarch64/stack.o arch/aarch64/utils.o arch/aarch64/init.o arch/aarch64/psci.o common/boot.o common/bakery_lock.o common/platform.o common/lib.o common/init.o common/psci.o common/gic-v3.o -o linux-system.axf --script=model.lds
> ld: common/platform.o: in function `print_ulong_hex':
> /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:66: undefined reference to `__stack_chk_fail'
> /src/boot-wrapper-aarch64.git/common/platform.c:66:(.text.print_ulong_hex+0xa0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `__stack_chk_fail'
> make: *** [Makefile:684: linux-system.axf] Error 1
> ------------------
I got curious and tried a quick test compiling this function in
isolation, and indeed for some reason GCC (9.3.0 at -O2) seems to end up
allocating 80 bytes of stack, which is apparently large enough to
trigger insertion of a stack check, and laboriously copying the string
from static data into it. FWIW either making hex_chars static const, or
simply '#define hex_chars "01234567abcdef"', not only avoids a stack
check but also leads to notably nicer code.
Of course, something like "print_char(v + (v > 9 ? 'a' : '0'));" is best
of all in terms of code/data, but risks venturing into "too clever"
territory.
Robin.
> Adding -fno-stack-protector fixes it again.
>
> I am still checking on each one of those compiler options, shall I send a
> smaller version of that patch 3/9 of mine, meanwhile, just carrying the
> uncontested -ffreestanding and -nostdlib, plus -fno-stack-protector?
>
> Cheers,
> Andre
>
>> + int i;
>> +
>> + for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
>> + int v = (val >> (4 * i)) & 0xf;
>> + print_char(hex_chars[v]);
>> + }
>> +}
>> +
>> void init_uart(void)
>> {
>> /*
>> diff --git a/include/platform.h b/include/platform.h
>> index 237b481..c88e124 100644
>> --- a/include/platform.h
>> +++ b/include/platform.h
>> @@ -11,6 +11,8 @@
>>
>> void print_char(char c);
>> void print_string(const char *str);
>> +void print_ulong_hex(unsigned long val);
>> +
>> void init_uart(void);
>>
>> void init_platform(void);
>> diff --git a/model.lds.S b/model.lds.S
>> index d4e7e13..dacaa25 100644
>> --- a/model.lds.S
>> +++ b/model.lds.S
>> @@ -35,46 +35,54 @@ SECTIONS
>> * the boot section's *(.data)
>> */
>> .kernel (PHYS_OFFSET + KERNEL_OFFSET): {
>> - kernel = .;
>> + kernel__start = .;
>> KERNEL
>> + kernel__end = .;
>> }
>>
>> #ifdef XEN
>> .xen (PHYS_OFFSET + XEN_OFFSET): {
>> - xen = .;
>> + xen__start = .;
>> XEN
>> + xen__end = .;
>> }
>>
>> - entrypoint = xen;
>> + entrypoint = xen__start;
>> #else
>> - entrypoint = kernel;
>> + entrypoint = kernel__start;
>> #endif
>>
>> .dtb (PHYS_OFFSET + FDT_OFFSET): {
>> + dtb__start = .;
>> dtb = .;
>> ./fdt.dtb
>> + dtb__end = .;
>> }
>>
>> #ifdef USE_INITRD
>> .filesystem (PHYS_OFFSET + FS_OFFSET): {
>> - filesystem = .;
>> + filesystem__start = .;
>> FILESYSTEM
>> - fs_size = . - filesystem;
>> + filesystem__end = .;
>> }
>> #endif
>>
>> .boot PHYS_OFFSET: {
>> + text__start = .;
>> *(.init)
>> *(.text*)
>> *(.data* .rodata* .bss* COMMON)
>> *(.vectors)
>> *(.stack)
>> PROVIDE(etext = .);
>> + text__end = .;
>> }
>>
>> .mbox (PHYS_OFFSET + MBOX_OFFSET): {
>> + mbox__start = .;
>> mbox = .;
>> QUAD(0x0)
>> + mbox__end = .;
>> }
>>
>> ASSERT(etext <= (PHYS_OFFSET + TEXT_LIMIT), ".text overflow!")
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 16:04 ` Robin Murphy
@ 2022-01-14 16:30 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 16:30 UTC (permalink / raw)
To: Robin Murphy
Cc: Andre Przywara, linux-arm-kernel, Jaxson.Han, vladimir.murzin, Wei.Chen
On Fri, Jan 14, 2022 at 04:04:19PM +0000, Robin Murphy wrote:
> On 2022-01-14 15:30, Andre Przywara wrote:
> > On Fri, 14 Jan 2022 10:56:51 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > +void print_ulong_hex(unsigned long val)
> > > +{
> > > + const char hex_chars[16] = "0123456789abcdef";
> >
> > This breaks the build for me (I guess because of the const?):
> > -------------------
> > ld --gc-sections arch/aarch64/boot.o arch/aarch64/stack.o arch/aarch64/utils.o arch/aarch64/init.o arch/aarch64/psci.o common/boot.o common/bakery_lock.o common/platform.o common/lib.o common/init.o common/psci.o common/gic-v3.o -o linux-system.axf --script=model.lds
> > ld: common/platform.o: in function `print_ulong_hex':
> > /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> > ld: /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> > ld: /src/boot-wrapper-aarch64.git/common/platform.c:66: undefined reference to `__stack_chk_fail'
> > /src/boot-wrapper-aarch64.git/common/platform.c:66:(.text.print_ulong_hex+0xa0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `__stack_chk_fail'
> > make: *** [Makefile:684: linux-system.axf] Error 1
> > ------------------
>
> I got curious and tried a quick test compiling this function in isolation,
> and indeed for some reason GCC (9.3.0 at -O2) seems to end up allocating 80
> bytes of stack, which is apparently large enough to trigger insertion of a
> stack check, and laboriously copying the string from static data into it.
> FWIW either making hex_chars static const, or simply '#define hex_chars
> "01234567abcdef"', not only avoids a stack check but also leads to notably
> nicer code.
For the stack check, I think Andre's patch to add -fno-stack-protector is the
correct fix, so I'd like to take that as a prerequisite.
To avoid using the stack unnecessarily and to make the code nicer, I'll also
take your suggestion with the fixup below.
Thanks,
Mark.
---->8----
-#define HEX_CHARS_PER_LONG (2 * sizeof(long))
+#define HEX_CHARS_PER_LONG (2 * sizeof(long))
+#define HEX_CHARS "0123456789abcdef"
void print_ulong_hex(unsigned long val)
{
- const char hex_chars[16] = "0123456789abcdef";
int i;
for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
int v = (val >> (4 * i)) & 0xf;
- print_char(hex_chars[v]);
+ print_char(HEX_CHARS[v]);
}
}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 15:30 ` Andre Przywara
2022-01-14 16:04 ` Robin Murphy
@ 2022-01-14 16:21 ` Mark Rutland
1 sibling, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 16:21 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, Jan 14, 2022 at 03:30:53PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:51 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> > To make it easier to debug boot failures, log the location of memory
> > objects at boot time.
> >
> > This is logged to the serial console as:
> >
> > | Boot-wrapper v0.2
> > | Entered at EL3
> > | Memory layout:
> > | [0000000080000000..0000000080001f90] => boot-wrapper
> > | [000000008000fff8..0000000080010000] => mbox
> > | [0000000080200000..00000000822af200] => kernel
> > | [0000000088000000..0000000088002857] => dtb
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> > +void print_ulong_hex(unsigned long val)
> > +{
> > + const char hex_chars[16] = "0123456789abcdef";
>
> This breaks the build for me (I guess because of the const?):
> -------------------
> ld --gc-sections arch/aarch64/boot.o arch/aarch64/stack.o arch/aarch64/utils.o arch/aarch64/init.o arch/aarch64/psci.o common/boot.o common/bakery_lock.o common/platform.o common/lib.o common/init.o common/psci.o common/gic-v3.o -o linux-system.axf --script=model.lds
> ld: common/platform.o: in function `print_ulong_hex':
> /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:66: undefined reference to `__stack_chk_fail'
> /src/boot-wrapper-aarch64.git/common/platform.c:66:(.text.print_ulong_hex+0xa0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `__stack_chk_fail'
> make: *** [Makefile:684: linux-system.axf] Error 1
> ------------------
>
> Adding -fno-stack-protector fixes it again.
>
> I am still checking on each one of those compiler options, shall I send a
> smaller version of that patch 3/9 of mine, meanwhile, just carrying the
> uncontested -ffreestanding and -nostdlib, plus -fno-stack-protector?
Yes please!
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects
2022-01-14 10:56 ` [bootwrapper PATCH v2 11/13] Announce locations of memory objects Mark Rutland
2022-01-14 15:30 ` Andre Przywara
@ 2022-01-17 14:59 ` Andre Przywara
1 sibling, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 14:59 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:51 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> To make it easier to debug boot failures, log the location of memory
> objects at boot time.
>
> This is logged to the serial console as:
>
> | Boot-wrapper v0.2
> | Entered at EL3
> | Memory layout:
> | [0000000080000000..0000000080001f90] => boot-wrapper
> | [000000008000fff8..0000000080010000] => mbox
> | [0000000080200000..00000000822af200] => kernel
> | [0000000088000000..0000000088002857] => dtb
That looks indeed very useful!
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> common/init.c | 27 +++++++++++++++++++++++++++
> common/platform.c | 13 +++++++++++++
> include/platform.h | 2 ++
> model.lds.S | 20 ++++++++++++++------
> 4 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/common/init.c b/common/init.c
> index 2600f73..fc74b9e 100644
> --- a/common/init.c
> +++ b/common/init.c
> @@ -14,6 +14,32 @@ static void announce_bootwrapper(void)
> print_string("Boot-wrapper v0.2\r\n");
> }
>
> +#define announce_object(object, desc) \
> +do { \
> + extern char object##__start[]; \
> + extern char object##__end[]; \
> + print_string("["); \
> + print_ulong_hex((unsigned long)object##__start); \
> + print_string(".."); \
> + print_ulong_hex((unsigned long)object##__end); \
> + print_string("] => " desc "\r\n"); \
> +} while (0)
> +
> +static void announce_objects(void)
> +{
> + print_string("Memory layout:\r\n");
> + announce_object(text, "boot-wrapper");
> + announce_object(mbox, "mbox");
> + announce_object(kernel, "kernel");
> +#ifdef XEN
> + announce_object(xen, "xen");
> +#endif
> + announce_object(dtb, "dtb");
> +#ifdef USE_INITRD
> + announce_object(filesystem, "initrd");
> +#endif
> +}
> +
> void announce_arch(void);
>
> void cpu_init_bootwrapper(void)
> @@ -22,6 +48,7 @@ void cpu_init_bootwrapper(void)
> init_uart();
> announce_bootwrapper();
> announce_arch();
> + announce_objects();
> print_string("\r\n");
> init_platform();
> }
> diff --git a/common/platform.c b/common/platform.c
> index 80d0562..26e5944 100644
> --- a/common/platform.c
> +++ b/common/platform.c
> @@ -52,6 +52,19 @@ void print_string(const char *str)
> print_char(*str++);
> }
>
> +#define HEX_CHARS_PER_LONG (2 * sizeof(long))
> +
> +void print_ulong_hex(unsigned long val)
> +{
> + const char hex_chars[16] = "0123456789abcdef";
> + int i;
> +
> + for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
> + int v = (val >> (4 * i)) & 0xf;
> + print_char(hex_chars[v]);
> + }
> +}
> +
> void init_uart(void)
> {
> /*
> diff --git a/include/platform.h b/include/platform.h
> index 237b481..c88e124 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -11,6 +11,8 @@
>
> void print_char(char c);
> void print_string(const char *str);
> +void print_ulong_hex(unsigned long val);
> +
> void init_uart(void);
>
> void init_platform(void);
> diff --git a/model.lds.S b/model.lds.S
> index d4e7e13..dacaa25 100644
> --- a/model.lds.S
> +++ b/model.lds.S
> @@ -35,46 +35,54 @@ SECTIONS
> * the boot section's *(.data)
> */
> .kernel (PHYS_OFFSET + KERNEL_OFFSET): {
> - kernel = .;
> + kernel__start = .;
> KERNEL
> + kernel__end = .;
> }
>
> #ifdef XEN
> .xen (PHYS_OFFSET + XEN_OFFSET): {
> - xen = .;
> + xen__start = .;
> XEN
> + xen__end = .;
> }
>
> - entrypoint = xen;
> + entrypoint = xen__start;
> #else
> - entrypoint = kernel;
> + entrypoint = kernel__start;
> #endif
>
> .dtb (PHYS_OFFSET + FDT_OFFSET): {
> + dtb__start = .;
> dtb = .;
> ./fdt.dtb
> + dtb__end = .;
> }
>
> #ifdef USE_INITRD
> .filesystem (PHYS_OFFSET + FS_OFFSET): {
> - filesystem = .;
> + filesystem__start = .;
> FILESYSTEM
> - fs_size = . - filesystem;
> + filesystem__end = .;
> }
> #endif
>
> .boot PHYS_OFFSET: {
> + text__start = .;
> *(.init)
> *(.text*)
> *(.data* .rodata* .bss* COMMON)
> *(.vectors)
> *(.stack)
> PROVIDE(etext = .);
> + text__end = .;
> }
>
> .mbox (PHYS_OFFSET + MBOX_OFFSET): {
> + mbox__start = .;
> mbox = .;
> QUAD(0x0)
> + mbox__end = .;
> }
>
> ASSERT(etext <= (PHYS_OFFSET + TEXT_LIMIT), ".text overflow!")
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 12/13] Rework bootmethod initialization
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (10 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 11/13] Announce locations of memory objects Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 17:43 ` Andre Przywara
2022-01-14 10:56 ` [bootwrapper PATCH v2 13/13] Unify start_el3 & start_no_el3 Mark Rutland
2022-01-14 15:09 ` [bootwrapper PATCH v2 00/13] Cleanups and improvements Andre Przywara
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
We currently initialize the bootmethod late, in assembly code. This
requires us to maintain the el3/no_el3 distintion late into the boot
process, and means we cannot produce any helpful diagnostic when booted
at an unexpected exception level.
Rework things so that we initialize the bootmethod early, with a warning
when things are wrong. The el3/no_el3 distinction is now irrelevant to
the bootmethod code, and can be removed in subsequent patches.
When a boot-wrapper configured for PSCI is entered at EL2, a warning is
looged to the serial console as:
| Boot-wrapper v0.2
| Entered at EL2
| Memory layout:
| [0000000080000000..0000000080001f90] => boot-wrapper
| [000000008000fff8..0000000080010000] => mbox
| [0000000080200000..00000000822af200] => kernel
| [0000000088000000..0000000088002857] => dtb
|
| WARNING: PSCI could not be initialized. Boot may fail
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch32/include/asm/cpu.h | 1 +
arch/aarch32/include/asm/psci.h | 28 ++++++++++++++++++++++++++++
arch/aarch32/psci.S | 8 +-------
arch/aarch32/utils.S | 9 ---------
arch/aarch64/include/asm/psci.h | 28 ++++++++++++++++++++++++++++
arch/aarch64/psci.S | 21 ++-------------------
arch/aarch64/spin.S | 3 +++
arch/aarch64/utils.S | 9 ---------
common/init.c | 7 ++++++-
common/psci.c | 22 +++++++++++++++++++---
include/boot.h | 2 ++
11 files changed, 90 insertions(+), 48 deletions(-)
create mode 100644 arch/aarch32/include/asm/psci.h
create mode 100644 arch/aarch64/include/asm/psci.h
diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
index c1bce9a..3426075 100644
--- a/arch/aarch32/include/asm/cpu.h
+++ b/arch/aarch32/include/asm/cpu.h
@@ -63,6 +63,7 @@ static inline unsigned long read_cpsr(void)
#define SCR "p15, 0, %0, c1, c1, 0"
#define NSACR "p15, 0, %0, c1, c1, 2"
#define ICIALLU "p15, 0, %0, c7, c5, 0"
+#define MVBAR "p15, 0, %0, c12, c0, 1"
#define ICC_SRE "p15, 6, %0, c12, c12, 5"
#define ICC_CTLR "p15, 6, %0, c12, c12, 4"
diff --git a/arch/aarch32/include/asm/psci.h b/arch/aarch32/include/asm/psci.h
new file mode 100644
index 0000000..50c7c70
--- /dev/null
+++ b/arch/aarch32/include/asm/psci.h
@@ -0,0 +1,28 @@
+/*
+ * arch/aarch64/include/asm/psci.h
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#ifndef __ASM_AARCH64_PSCI_H
+#define __ASM_AARCH64_PSCI_H
+
+#include <cpu.h>
+#include <stdbool.h>
+
+extern char psci_vectors[];
+
+static inline bool cpu_init_psci_arch(void)
+{
+ if (read_cpsr_mode() != PSR_MON)
+ return false;
+
+ mcr(MVBAR, (unsigned long)psci_vectors);
+ isb();
+
+ return true;
+}
+
+#endif
diff --git a/arch/aarch32/psci.S b/arch/aarch32/psci.S
index e0d2972..cdc36b0 100644
--- a/arch/aarch32/psci.S
+++ b/arch/aarch32/psci.S
@@ -15,7 +15,7 @@
.section .vectors
.align 6
-smc_vectors:
+ASM_DATA(psci_vectors)
b err_exception @ Reset
b err_exception @ Undef
b handle_smc @ SMC
@@ -39,11 +39,5 @@ handle_smc:
movs pc, lr
ASM_FUNC(start_el3)
- ldr r0, =smc_vectors
- blx setup_vector
- /* pass through */
-
ASM_FUNC(start_no_el3)
- cpuid r0, r1
- blx find_logical_id
b psci_first_spin
diff --git a/arch/aarch32/utils.S b/arch/aarch32/utils.S
index 5809f48..58279aa 100644
--- a/arch/aarch32/utils.S
+++ b/arch/aarch32/utils.S
@@ -35,12 +35,3 @@ ASM_FUNC(find_logical_id)
bx lr
3: mov r0, #MPIDR_INVALID
bx lr
-
-/*
- * Setup EL3 vectors.
- * r0: vector address
- */
-ASM_FUNC(setup_vector)
- mcr p15, 0, r0, c12, c0, 1 @ MVBAR
- isb
- bx lr
diff --git a/arch/aarch64/include/asm/psci.h b/arch/aarch64/include/asm/psci.h
new file mode 100644
index 0000000..491e685
--- /dev/null
+++ b/arch/aarch64/include/asm/psci.h
@@ -0,0 +1,28 @@
+/*
+ * arch/aarch64/include/asm/psci.h
+ *
+ * Copyright (C) 2021 ARM Limited. All rights reserved.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE.txt file.
+ */
+#ifndef __ASM_AARCH64_PSCI_H
+#define __ASM_AARCH64_PSCI_H
+
+#include <cpu.h>
+#include <stdbool.h>
+
+extern char psci_vectors[];
+
+static inline bool cpu_init_psci_arch(void)
+{
+ if (mrs(CurrentEL) != CURRENTEL_EL3)
+ return false;
+
+ msr(VBAR_EL3, (unsigned long)psci_vectors);
+ isb();
+
+ return true;
+}
+
+#endif
diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S
index 8bd224b..d6ca2eb 100644
--- a/arch/aarch64/psci.S
+++ b/arch/aarch64/psci.S
@@ -19,7 +19,7 @@
.section .vectors, "w"
.align 11
-vector:
+ASM_DATA(psci_vectors)
// current EL, SP_EL0
ventry err_exception // synchronous
ventry err_exception // IRQ
@@ -80,22 +80,5 @@ smc_exit:
eret
ASM_FUNC(start_el3)
- ldr x0, =vector
- bl setup_vector
-
- /* only boot the primary cpu (entry 0 in the table) */
- cpuid x0, x1
- bl find_logical_id
- b psci_first_spin
-
-/*
- * This PSCI implementation requires EL3. Without EL3 we'll only boot the
- * primary cpu, all others will be trapped in an infinite loop.
- */
ASM_FUNC(start_no_el3)
- cpuid x0, x1
- bl find_logical_id
- cbz x0, psci_first_spin
-spin_dead:
- wfe
- b spin_dead
+ b psci_first_spin
diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
index 1ea1c0b..764c532 100644
--- a/arch/aarch64/spin.S
+++ b/arch/aarch64/spin.S
@@ -12,6 +12,9 @@
.text
+ASM_FUNC(cpu_init_bootmethod)
+ ret
+
ASM_FUNC(start_el3)
ASM_FUNC(start_no_el3)
cpuid x0, x1
diff --git a/arch/aarch64/utils.S b/arch/aarch64/utils.S
index 85c7f8a..32393cc 100644
--- a/arch/aarch64/utils.S
+++ b/arch/aarch64/utils.S
@@ -32,12 +32,3 @@ ASM_FUNC(find_logical_id)
ret
3: mov x0, #MPIDR_INVALID
ret
-
-/*
- * Setup EL3 vectors
- * x0: vector address
- */
-ASM_FUNC(setup_vector)
- msr VBAR_EL3, x0
- isb
- ret
diff --git a/common/init.c b/common/init.c
index fc74b9e..3c05ac3 100644
--- a/common/init.c
+++ b/common/init.c
@@ -6,6 +6,7 @@
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE.txt file.
*/
+#include <boot.h>
#include <cpu.h>
#include <platform.h>
@@ -44,7 +45,9 @@ void announce_arch(void);
void cpu_init_bootwrapper(void)
{
- if (this_cpu_logical_id() == 0) {
+ unsigned int cpu = this_cpu_logical_id();
+
+ if (cpu == 0) {
init_uart();
announce_bootwrapper();
announce_arch();
@@ -52,4 +55,6 @@ void cpu_init_bootwrapper(void)
print_string("\r\n");
init_platform();
}
+
+ cpu_init_bootmethod(cpu);
}
diff --git a/common/psci.c b/common/psci.c
index a0e8700..8af6870 100644
--- a/common/psci.c
+++ b/common/psci.c
@@ -12,8 +12,11 @@
#include <bakery_lock.h>
#include <boot.h>
#include <cpu.h>
+#include <platform.h>
#include <psci.h>
+#include <asm/psci.h>
+
#ifndef CPU_IDS
#error "No MPIDRs provided"
#endif
@@ -78,12 +81,25 @@ long psci_call(unsigned long fid, unsigned long arg1, unsigned long arg2)
}
}
-void __noreturn psci_first_spin(unsigned int cpu)
+void __noreturn psci_first_spin(void)
{
- if (cpu == MPIDR_INVALID)
- while (1);
+ unsigned int cpu = this_cpu_logical_id();
first_spin(cpu, branch_table + cpu, PSCI_ADDR_INVALID);
unreachable();
}
+
+void cpu_init_bootmethod(unsigned int cpu)
+{
+ if (cpu_init_psci_arch())
+ return;
+
+ if (cpu == 0) {
+ print_string("WARNING: PSCI could not be initialized. Boot may fail\r\n\r\n");
+ return;
+ }
+
+ while (1)
+ wfe();
+}
diff --git a/include/boot.h b/include/boot.h
index d75e013..10968f8 100644
--- a/include/boot.h
+++ b/include/boot.h
@@ -16,4 +16,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry);
void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
unsigned long invalid_addr);
+void cpu_init_bootmethod(unsigned int cpu);
+
#endif
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 12/13] Rework bootmethod initialization
2022-01-14 10:56 ` [bootwrapper PATCH v2 12/13] Rework bootmethod initialization Mark Rutland
@ 2022-01-17 17:43 ` Andre Przywara
2022-01-25 14:00 ` Mark Rutland
0 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 17:43 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:52 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> We currently initialize the bootmethod late, in assembly code. This
> requires us to maintain the el3/no_el3 distintion late into the boot
> process, and means we cannot produce any helpful diagnostic when booted
> at an unexpected exception level.
>
> Rework things so that we initialize the bootmethod early, with a warning
> when things are wrong. The el3/no_el3 distinction is now irrelevant to
> the bootmethod code, and can be removed in subsequent patches.
>
> When a boot-wrapper configured for PSCI is entered at EL2, a warning is
> looged to the serial console as:
>
> | Boot-wrapper v0.2
> | Entered at EL2
> | Memory layout:
> | [0000000080000000..0000000080001f90] => boot-wrapper
> | [000000008000fff8..0000000080010000] => mbox
> | [0000000080200000..00000000822af200] => kernel
> | [0000000088000000..0000000088002857] => dtb
> |
> | WARNING: PSCI could not be initialized. Boot may fail
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> arch/aarch32/include/asm/cpu.h | 1 +
> arch/aarch32/include/asm/psci.h | 28 ++++++++++++++++++++++++++++
> arch/aarch32/psci.S | 8 +-------
> arch/aarch32/utils.S | 9 ---------
> arch/aarch64/include/asm/psci.h | 28 ++++++++++++++++++++++++++++
> arch/aarch64/psci.S | 21 ++-------------------
> arch/aarch64/spin.S | 3 +++
> arch/aarch64/utils.S | 9 ---------
> common/init.c | 7 ++++++-
> common/psci.c | 22 +++++++++++++++++++---
> include/boot.h | 2 ++
> 11 files changed, 90 insertions(+), 48 deletions(-)
> create mode 100644 arch/aarch32/include/asm/psci.h
> create mode 100644 arch/aarch64/include/asm/psci.h
>
> diff --git a/arch/aarch32/include/asm/cpu.h b/arch/aarch32/include/asm/cpu.h
> index c1bce9a..3426075 100644
> --- a/arch/aarch32/include/asm/cpu.h
> +++ b/arch/aarch32/include/asm/cpu.h
> @@ -63,6 +63,7 @@ static inline unsigned long read_cpsr(void)
> #define SCR "p15, 0, %0, c1, c1, 0"
> #define NSACR "p15, 0, %0, c1, c1, 2"
> #define ICIALLU "p15, 0, %0, c7, c5, 0"
> +#define MVBAR "p15, 0, %0, c12, c0, 1"
>
> #define ICC_SRE "p15, 6, %0, c12, c12, 5"
> #define ICC_CTLR "p15, 6, %0, c12, c12, 4"
> diff --git a/arch/aarch32/include/asm/psci.h b/arch/aarch32/include/asm/psci.h
> new file mode 100644
> index 0000000..50c7c70
> --- /dev/null
> +++ b/arch/aarch32/include/asm/psci.h
> @@ -0,0 +1,28 @@
> +/*
> + * arch/aarch64/include/asm/psci.h
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __ASM_AARCH64_PSCI_H
> +#define __ASM_AARCH64_PSCI_H
> +
> +#include <cpu.h>
> +#include <stdbool.h>
> +
> +extern char psci_vectors[];
> +
> +static inline bool cpu_init_psci_arch(void)
> +{
> + if (read_cpsr_mode() != PSR_MON)
> + return false;
> +
> + mcr(MVBAR, (unsigned long)psci_vectors);
> + isb();
> +
> + return true;
> +}
> +
> +#endif
> diff --git a/arch/aarch32/psci.S b/arch/aarch32/psci.S
> index e0d2972..cdc36b0 100644
> --- a/arch/aarch32/psci.S
> +++ b/arch/aarch32/psci.S
> @@ -15,7 +15,7 @@
>
> .section .vectors
> .align 6
> -smc_vectors:
> +ASM_DATA(psci_vectors)
> b err_exception @ Reset
> b err_exception @ Undef
> b handle_smc @ SMC
> @@ -39,11 +39,5 @@ handle_smc:
> movs pc, lr
>
> ASM_FUNC(start_el3)
> - ldr r0, =smc_vectors
> - blx setup_vector
> - /* pass through */
> -
> ASM_FUNC(start_no_el3)
> - cpuid r0, r1
> - blx find_logical_id
> b psci_first_spin
> diff --git a/arch/aarch32/utils.S b/arch/aarch32/utils.S
> index 5809f48..58279aa 100644
> --- a/arch/aarch32/utils.S
> +++ b/arch/aarch32/utils.S
> @@ -35,12 +35,3 @@ ASM_FUNC(find_logical_id)
> bx lr
> 3: mov r0, #MPIDR_INVALID
> bx lr
> -
> -/*
> - * Setup EL3 vectors.
> - * r0: vector address
> - */
> -ASM_FUNC(setup_vector)
> - mcr p15, 0, r0, c12, c0, 1 @ MVBAR
> - isb
> - bx lr
> diff --git a/arch/aarch64/include/asm/psci.h b/arch/aarch64/include/asm/psci.h
> new file mode 100644
> index 0000000..491e685
> --- /dev/null
> +++ b/arch/aarch64/include/asm/psci.h
> @@ -0,0 +1,28 @@
> +/*
> + * arch/aarch64/include/asm/psci.h
> + *
> + * Copyright (C) 2021 ARM Limited. All rights reserved.
> + *
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE.txt file.
> + */
> +#ifndef __ASM_AARCH64_PSCI_H
> +#define __ASM_AARCH64_PSCI_H
> +
> +#include <cpu.h>
> +#include <stdbool.h>
> +
> +extern char psci_vectors[];
> +
> +static inline bool cpu_init_psci_arch(void)
> +{
> + if (mrs(CurrentEL) != CURRENTEL_EL3)
> + return false;
> +
> + msr(VBAR_EL3, (unsigned long)psci_vectors);
> + isb();
> +
> + return true;
> +}
> +
> +#endif
Is there any particular reason that needs to live as a static inline in a
header file? Can't we have the prototype in, say include/boot.h, and then
have this in a proper C file, for instance arch/aarch<xx>/init.c?
The rest looks alright, though the change is somewhat hard to digest.
Cheers,
Andre
> diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S
> index 8bd224b..d6ca2eb 100644
> --- a/arch/aarch64/psci.S
> +++ b/arch/aarch64/psci.S
> @@ -19,7 +19,7 @@
> .section .vectors, "w"
>
> .align 11
> -vector:
> +ASM_DATA(psci_vectors)
> // current EL, SP_EL0
> ventry err_exception // synchronous
> ventry err_exception // IRQ
> @@ -80,22 +80,5 @@ smc_exit:
> eret
>
> ASM_FUNC(start_el3)
> - ldr x0, =vector
> - bl setup_vector
> -
> - /* only boot the primary cpu (entry 0 in the table) */
> - cpuid x0, x1
> - bl find_logical_id
> - b psci_first_spin
> -
> -/*
> - * This PSCI implementation requires EL3. Without EL3 we'll only boot the
> - * primary cpu, all others will be trapped in an infinite loop.
> - */
> ASM_FUNC(start_no_el3)
> - cpuid x0, x1
> - bl find_logical_id
> - cbz x0, psci_first_spin
> -spin_dead:
> - wfe
> - b spin_dead
> + b psci_first_spin
> diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
> index 1ea1c0b..764c532 100644
> --- a/arch/aarch64/spin.S
> +++ b/arch/aarch64/spin.S
> @@ -12,6 +12,9 @@
>
> .text
>
> +ASM_FUNC(cpu_init_bootmethod)
> + ret
> +
> ASM_FUNC(start_el3)
> ASM_FUNC(start_no_el3)
> cpuid x0, x1
> diff --git a/arch/aarch64/utils.S b/arch/aarch64/utils.S
> index 85c7f8a..32393cc 100644
> --- a/arch/aarch64/utils.S
> +++ b/arch/aarch64/utils.S
> @@ -32,12 +32,3 @@ ASM_FUNC(find_logical_id)
> ret
> 3: mov x0, #MPIDR_INVALID
> ret
> -
> -/*
> - * Setup EL3 vectors
> - * x0: vector address
> - */
> -ASM_FUNC(setup_vector)
> - msr VBAR_EL3, x0
> - isb
> - ret
> diff --git a/common/init.c b/common/init.c
> index fc74b9e..3c05ac3 100644
> --- a/common/init.c
> +++ b/common/init.c
> @@ -6,6 +6,7 @@
> * Use of this source code is governed by a BSD-style license that can be
> * found in the LICENSE.txt file.
> */
> +#include <boot.h>
> #include <cpu.h>
> #include <platform.h>
>
> @@ -44,7 +45,9 @@ void announce_arch(void);
>
> void cpu_init_bootwrapper(void)
> {
> - if (this_cpu_logical_id() == 0) {
> + unsigned int cpu = this_cpu_logical_id();
> +
> + if (cpu == 0) {
> init_uart();
> announce_bootwrapper();
> announce_arch();
> @@ -52,4 +55,6 @@ void cpu_init_bootwrapper(void)
> print_string("\r\n");
> init_platform();
> }
> +
> + cpu_init_bootmethod(cpu);
> }
> diff --git a/common/psci.c b/common/psci.c
> index a0e8700..8af6870 100644
> --- a/common/psci.c
> +++ b/common/psci.c
> @@ -12,8 +12,11 @@
> #include <bakery_lock.h>
> #include <boot.h>
> #include <cpu.h>
> +#include <platform.h>
> #include <psci.h>
>
> +#include <asm/psci.h>
> +
> #ifndef CPU_IDS
> #error "No MPIDRs provided"
> #endif
> @@ -78,12 +81,25 @@ long psci_call(unsigned long fid, unsigned long arg1, unsigned long arg2)
> }
> }
>
> -void __noreturn psci_first_spin(unsigned int cpu)
> +void __noreturn psci_first_spin(void)
> {
> - if (cpu == MPIDR_INVALID)
> - while (1);
> + unsigned int cpu = this_cpu_logical_id();
>
> first_spin(cpu, branch_table + cpu, PSCI_ADDR_INVALID);
>
> unreachable();
> }
> +
> +void cpu_init_bootmethod(unsigned int cpu)
> +{
> + if (cpu_init_psci_arch())
> + return;
> +
> + if (cpu == 0) {
> + print_string("WARNING: PSCI could not be initialized. Boot may fail\r\n\r\n");
> + return;
> + }
> +
> + while (1)
> + wfe();
> +}
> diff --git a/include/boot.h b/include/boot.h
> index d75e013..10968f8 100644
> --- a/include/boot.h
> +++ b/include/boot.h
> @@ -16,4 +16,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry);
> void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
> unsigned long invalid_addr);
>
> +void cpu_init_bootmethod(unsigned int cpu);
> +
> #endif
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 12/13] Rework bootmethod initialization
2022-01-17 17:43 ` Andre Przywara
@ 2022-01-25 14:00 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-25 14:00 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Mon, Jan 17, 2022 at 05:43:00PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:52 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
> > We currently initialize the bootmethod late, in assembly code. This
> > requires us to maintain the el3/no_el3 distintion late into the boot
> > process, and means we cannot produce any helpful diagnostic when booted
> > at an unexpected exception level.
> >
> > Rework things so that we initialize the bootmethod early, with a warning
> > when things are wrong. The el3/no_el3 distinction is now irrelevant to
> > the bootmethod code, and can be removed in subsequent patches.
> >
> > When a boot-wrapper configured for PSCI is entered at EL2, a warning is
> > looged to the serial console as:
> >
> > | Boot-wrapper v0.2
> > | Entered at EL2
> > | Memory layout:
> > | [0000000080000000..0000000080001f90] => boot-wrapper
> > | [000000008000fff8..0000000080010000] => mbox
> > | [0000000080200000..00000000822af200] => kernel
> > | [0000000088000000..0000000088002857] => dtb
> > |
> > | WARNING: PSCI could not be initialized. Boot may fail
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > diff --git a/arch/aarch64/include/asm/psci.h b/arch/aarch64/include/asm/psci.h
> > new file mode 100644
> > index 0000000..491e685
> > --- /dev/null
> > +++ b/arch/aarch64/include/asm/psci.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * arch/aarch64/include/asm/psci.h
> > + *
> > + * Copyright (C) 2021 ARM Limited. All rights reserved.
> > + *
> > + * Use of this source code is governed by a BSD-style license that can be
> > + * found in the LICENSE.txt file.
> > + */
> > +#ifndef __ASM_AARCH64_PSCI_H
> > +#define __ASM_AARCH64_PSCI_H
> > +
> > +#include <cpu.h>
> > +#include <stdbool.h>
> > +
> > +extern char psci_vectors[];
> > +
> > +static inline bool cpu_init_psci_arch(void)
> > +{
> > + if (mrs(CurrentEL) != CURRENTEL_EL3)
> > + return false;
> > +
> > + msr(VBAR_EL3, (unsigned long)psci_vectors);
> > + isb();
> > +
> > + return true;
> > +}
> > +
> > +#endif
>
> Is there any particular reason that needs to live as a static inline in a
> header file? Can't we have the prototype in, say include/boot.h, and then
> have this in a proper C file, for instance arch/aarch<xx>/init.c?
At the time I originally wrote it, I had thought that this was the simplest
option, but you're right that it's cleaner to place this in the relevant init.c
file -- done. :)
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* [bootwrapper PATCH v2 13/13] Unify start_el3 & start_no_el3
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (11 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 12/13] Rework bootmethod initialization Mark Rutland
@ 2022-01-14 10:56 ` Mark Rutland
2022-01-17 17:43 ` Andre Przywara
2022-01-14 15:09 ` [bootwrapper PATCH v2 00/13] Cleanups and improvements Andre Przywara
13 siblings, 1 reply; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
Cc: andre.przywara, Jaxson.Han, mark.rutland, robin.murphy,
vladimir.murzin, Wei.Chen
Now that the start_el3 and start_no_el3 labels point at the same place,
unify them into a start_bootmethod label and update callers.
There should be no functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/aarch32/boot.S | 5 ++---
arch/aarch32/psci.S | 3 +--
arch/aarch64/boot.S | 4 ++--
arch/aarch64/psci.S | 3 +--
arch/aarch64/spin.S | 3 +--
5 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
index 820957b..4d16c9c 100644
--- a/arch/aarch32/boot.S
+++ b/arch/aarch32/boot.S
@@ -51,7 +51,7 @@ ASM_FUNC(_start)
bl cpu_init_bootwrapper
- b start_no_el3
+ b start_bootmethod
_switch_monitor:
adr lr, _monitor
@@ -69,8 +69,7 @@ _monitor:
bl gic_secure_init
- /* Initialise boot method */
- b start_el3
+ b start_bootmethod
err_invalid_id:
b .
diff --git a/arch/aarch32/psci.S b/arch/aarch32/psci.S
index cdc36b0..6613b6f 100644
--- a/arch/aarch32/psci.S
+++ b/arch/aarch32/psci.S
@@ -38,6 +38,5 @@ handle_smc:
pop {r4 - r12, lr}
movs pc, lr
-ASM_FUNC(start_el3)
-ASM_FUNC(start_no_el3)
+ASM_FUNC(start_bootmethod)
b psci_first_spin
diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
index c0ec518..da5fa65 100644
--- a/arch/aarch64/boot.S
+++ b/arch/aarch64/boot.S
@@ -57,7 +57,7 @@ reset_at_el3:
bl gic_secure_init
- b start_el3
+ b start_bootmethod
/*
* EL2 initialization
@@ -97,7 +97,7 @@ reset_no_el3:
bl cpu_init_bootwrapper
- b start_no_el3
+ b start_bootmethod
err_invalid_id:
b .
diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S
index d6ca2eb..9709dbb 100644
--- a/arch/aarch64/psci.S
+++ b/arch/aarch64/psci.S
@@ -79,6 +79,5 @@ smc_exit:
ldp x18, x19, [sp], #16
eret
-ASM_FUNC(start_el3)
-ASM_FUNC(start_no_el3)
+ASM_FUNC(start_bootmethod)
b psci_first_spin
diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
index 764c532..375f732 100644
--- a/arch/aarch64/spin.S
+++ b/arch/aarch64/spin.S
@@ -15,8 +15,7 @@
ASM_FUNC(cpu_init_bootmethod)
ret
-ASM_FUNC(start_el3)
-ASM_FUNC(start_no_el3)
+ASM_FUNC(start_bootmethod)
cpuid x0, x1
bl find_logical_id
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 13/13] Unify start_el3 & start_no_el3
2022-01-14 10:56 ` [bootwrapper PATCH v2 13/13] Unify start_el3 & start_no_el3 Mark Rutland
@ 2022-01-17 17:43 ` Andre Przywara
0 siblings, 0 replies; 43+ messages in thread
From: Andre Przywara @ 2022-01-17 17:43 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:53 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> Now that the start_el3 and start_no_el3 labels point at the same place,
> unify them into a start_bootmethod label and update callers.
Nice one.
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> arch/aarch32/boot.S | 5 ++---
> arch/aarch32/psci.S | 3 +--
> arch/aarch64/boot.S | 4 ++--
> arch/aarch64/psci.S | 3 +--
> arch/aarch64/spin.S | 3 +--
> 5 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/arch/aarch32/boot.S b/arch/aarch32/boot.S
> index 820957b..4d16c9c 100644
> --- a/arch/aarch32/boot.S
> +++ b/arch/aarch32/boot.S
> @@ -51,7 +51,7 @@ ASM_FUNC(_start)
>
> bl cpu_init_bootwrapper
>
> - b start_no_el3
> + b start_bootmethod
>
> _switch_monitor:
> adr lr, _monitor
> @@ -69,8 +69,7 @@ _monitor:
>
> bl gic_secure_init
>
> - /* Initialise boot method */
> - b start_el3
> + b start_bootmethod
>
> err_invalid_id:
> b .
> diff --git a/arch/aarch32/psci.S b/arch/aarch32/psci.S
> index cdc36b0..6613b6f 100644
> --- a/arch/aarch32/psci.S
> +++ b/arch/aarch32/psci.S
> @@ -38,6 +38,5 @@ handle_smc:
> pop {r4 - r12, lr}
> movs pc, lr
>
> -ASM_FUNC(start_el3)
> -ASM_FUNC(start_no_el3)
> +ASM_FUNC(start_bootmethod)
> b psci_first_spin
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index c0ec518..da5fa65 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -57,7 +57,7 @@ reset_at_el3:
>
> bl gic_secure_init
>
> - b start_el3
> + b start_bootmethod
>
> /*
> * EL2 initialization
> @@ -97,7 +97,7 @@ reset_no_el3:
>
> bl cpu_init_bootwrapper
>
> - b start_no_el3
> + b start_bootmethod
>
> err_invalid_id:
> b .
> diff --git a/arch/aarch64/psci.S b/arch/aarch64/psci.S
> index d6ca2eb..9709dbb 100644
> --- a/arch/aarch64/psci.S
> +++ b/arch/aarch64/psci.S
> @@ -79,6 +79,5 @@ smc_exit:
> ldp x18, x19, [sp], #16
> eret
>
> -ASM_FUNC(start_el3)
> -ASM_FUNC(start_no_el3)
> +ASM_FUNC(start_bootmethod)
> b psci_first_spin
> diff --git a/arch/aarch64/spin.S b/arch/aarch64/spin.S
> index 764c532..375f732 100644
> --- a/arch/aarch64/spin.S
> +++ b/arch/aarch64/spin.S
> @@ -15,8 +15,7 @@
> ASM_FUNC(cpu_init_bootmethod)
> ret
>
> -ASM_FUNC(start_el3)
> -ASM_FUNC(start_no_el3)
> +ASM_FUNC(start_bootmethod)
> cpuid x0, x1
> bl find_logical_id
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [bootwrapper PATCH v2 00/13] Cleanups and improvements
2022-01-14 10:56 [bootwrapper PATCH v2 00/13] Cleanups and improvements Mark Rutland
` (12 preceding siblings ...)
2022-01-14 10:56 ` [bootwrapper PATCH v2 13/13] Unify start_el3 & start_no_el3 Mark Rutland
@ 2022-01-14 15:09 ` Andre Przywara
2022-01-14 15:23 ` Mark Rutland
13 siblings, 1 reply; 43+ messages in thread
From: Andre Przywara @ 2022-01-14 15:09 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, 14 Jan 2022 10:56:40 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
Hi Mark,
> This series reworks the aarch64 boot-wrapper, moving a fair amount of
> initialization code to C. This has a few benefits:
>
> 1) This makes the code more legible and easier to maintain.
>
> Current feature detection and system register configuration is
> written in assembly, requiring runs of *almost* identical blocks of
> assembly to read ID registers and conditionally initialize register
> values. This requires a bunch of labels (which are all named
> numerically), and all the magic numbers are hard coded, so this gets
> pretty painful to read:
>
> | mrs x1, id_aa64isar0_el1
> | ubfx x1, x1, #24, #4
> | cbz x1, 1f
> |
> | orr x0, x0, #(1 << 34) // TME enable
> |
> | 1:
>
> In C, it's much easier to add helpers which use mnemonics, which
> makes the code much easier to read, and avoids the need to manually
> allocate registers, etc:
>
> | if (mrs_field(ID_AA64ISAR0_EL1, TME))
> | scr |= SCR_EL3_TME;
>
> This should make it easier to handle new architectural extensions
> (and/or architecture variants such as ARMv8-R) in future.
>
> 2) This allows for better diagnostics.
>
> Currently a mis-configured boot-wrapper is rather unforgiving, and
> provides no indication as to what might have gone wrong. By moving
> initialization to C, we can make use to the UART code to log
> diagnostic information, and we can more easily add additional error
> handling and conditional logic.
>
> This series adds diagnostic information and error handling that can
> be used to identify problems such as the boot-wrapper being launched
> at the wrong exception level:
>
> | Boot-wrapper v0.2
> | Entered at EL2
> | Memory layout:
> | [0000000080000000..0000000080001f90] => boot-wrapper
> | [000000008000fff8..0000000080010000] => mbox
> | [0000000080200000..00000000822af200] => kernel
> | [0000000088000000..0000000088002857] => dtb
> |
> | WARNING: PSCI could not be initialized. Boot may fail
>
> Originally I had planned for this to be a more expansive set of changes,
> unifying some early control-flow, fixing some latent bugs, and making
> the boot-wrapper dynamically handle being entered at any of EL{3,2,1}
> with automated selection of a suitable DT. As the series has already
> become pretty long, I'd like to get this preparatory cleanup out of the
> way first, and handle those cases with subsequent patches.
>
> If there are no objections, I intend to apply these by the end of the
> day.
Can you hold back with that till the beginning of next week? I got stuck
halfway in the series with my review, but am on it now as we speak.
Cheers,
Andre
>
> I've pushed the series to the `cleanup` branch in the boot-wrapper repo:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-aarch64.git/
> git://git.kernel.org/pub/scm/linux/kernel/git/mark/boot-wrapper-aarch64.git
>
> ... and it should apply cleanup atop the `master` branch.
>
> Since v1 [1]:
> * Add comments for bit-field macros
> * Use BIT() for *_RES1 definitions
> * Complete start_el3/start_no_el3 cleanup
> * Remove extraneous macro definition
>
> [1] https://lore.kernel.org/r/20220111130653.2331827-1-mark.rutland@arm.com/
>
> Thanks,
> Mark.
>
> Mark Rutland (13):
> Document entry requirements
> Add bit-field macros
> aarch64: add system register accessors
> aarch32: add coprocessor accessors
> aarch64: add mov_64 macro
> aarch64: initialize SCTLR_ELx for the boot-wrapper
> Rework common init C code
> Announce boot-wrapper mode / exception level
> aarch64: move the bulk of EL3 initialization to C
> aarch32: move the bulk of Secure PL1 initialization to C
> Announce locations of memory objects
> Rework bootmethod initialization
> Unify start_el3 & start_no_el3
>
> Makefile.am | 6 +-
> arch/aarch32/boot.S | 33 +++---
> arch/aarch32/include/asm/cpu.h | 62 ++++++++---
> arch/aarch32/include/asm/gic-v3.h | 6 +-
> arch/aarch32/include/asm/psci.h | 28 +++++
> arch/aarch32/init.c | 42 ++++++++
> arch/aarch32/psci.S | 16 +--
> arch/aarch32/utils.S | 9 --
> arch/aarch64/boot.S | 169 +++++++++++++-----------------
> arch/aarch64/common.S | 10 +-
> arch/aarch64/include/asm/cpu.h | 102 +++++++++++++++---
> arch/aarch64/include/asm/gic-v3.h | 10 +-
> arch/aarch64/include/asm/psci.h | 28 +++++
> arch/aarch64/init.c | 88 ++++++++++++++++
> arch/aarch64/psci.S | 22 +---
> arch/aarch64/spin.S | 6 +-
> arch/aarch64/utils.S | 9 --
> common/boot.c | 4 -
> common/init.c | 60 +++++++++++
> common/platform.c | 45 +++++---
> common/psci.c | 22 +++-
> include/bits.h | 79 ++++++++++++++
> include/boot.h | 2 +
> include/platform.h | 20 ++++
> model.lds.S | 20 ++--
> 25 files changed, 668 insertions(+), 230 deletions(-)
> create mode 100644 arch/aarch32/include/asm/psci.h
> create mode 100644 arch/aarch32/init.c
> create mode 100644 arch/aarch64/include/asm/psci.h
> create mode 100644 arch/aarch64/init.c
> create mode 100644 common/init.c
> create mode 100644 include/bits.h
> create mode 100644 include/platform.h
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [bootwrapper PATCH v2 00/13] Cleanups and improvements
2022-01-14 15:09 ` [bootwrapper PATCH v2 00/13] Cleanups and improvements Andre Przywara
@ 2022-01-14 15:23 ` Mark Rutland
0 siblings, 0 replies; 43+ messages in thread
From: Mark Rutland @ 2022-01-14 15:23 UTC (permalink / raw)
To: Andre Przywara
Cc: linux-arm-kernel, Jaxson.Han, robin.murphy, vladimir.murzin, Wei.Chen
On Fri, Jan 14, 2022 at 03:09:57PM +0000, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:40 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
> > If there are no objections, I intend to apply these by the end of the
> > day.
>
> Can you hold back with that till the beginning of next week? I got stuck
> halfway in the series with my review, but am on it now as we speak.
Sure.
Thanks for taking a look. :)
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread