Re: [bootwrapper PATCH v2 03/13] aarch64: add system register accessors

Andre Przywara
2022-01-14T15:33:44Z

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