From fd1dd4cb2c88f64a411c8482007e4669a563b80d Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Wed, 25 Jan 2023 12:26:14 +0000 Subject: [PATCH 1/6] refactor(cpufeat): wrap CPU ID register field isolation Some MISRA test complains about our code to isolate CPU ID register fields: the ID registers (and associated masks) are 64 bits wide, but the eventual field is always 4 bits wide only, so we use an unsigned int to represent that. MISRA dislikes the differing width here. Since the code to extract a feature field from a CPU ID register is very schematic already, provide a wrapper macro to make this more readable, and do the proper casting in one central place on the way. While at it, use the same macro for the AArch32 feature detection side. Change-Id: Ie102a9e7007a386f5879ec65e159ff041504a4ee Signed-off-by: Andre Przywara --- include/arch/aarch32/arch_features.h | 11 ++++++----- include/arch/aarch64/arch_features.h | 14 +++++++------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/include/arch/aarch32/arch_features.h b/include/arch/aarch32/arch_features.h index ddf09680b..a7d3fe605 100644 --- a/include/arch/aarch32/arch_features.h +++ b/include/arch/aarch32/arch_features.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, Arm Limited. All rights reserved. + * Copyright (c) 2019-2023, Arm Limited. All rights reserved. * * SPDX-License-Identifier: BSD-3-Clause */ @@ -11,16 +11,17 @@ #include +#define ISOLATE_FIELD(reg, feat) \ + ((unsigned int)(((reg) >> (feat ## _SHIFT)) & (feat ## _MASK))) + static inline bool is_armv7_gentimer_present(void) { - return ((read_id_pfr1() >> ID_PFR1_GENTIMER_SHIFT) & - ID_PFR1_GENTIMER_MASK) != 0U; + return ISOLATE_FIELD(read_id_pfr1(), ID_PFR1_GENTIMER) != 0U; } static inline bool is_armv8_2_ttcnp_present(void) { - return ((read_id_mmfr4() >> ID_MMFR4_CNP_SHIFT) & - ID_MMFR4_CNP_MASK) != 0U; + return ISOLATE_FIELD(read_id_mmfr4(), ID_MMFR4_CNP) != 0U; } #endif /* ARCH_FEATURES_H */ diff --git a/include/arch/aarch64/arch_features.h b/include/arch/aarch64/arch_features.h index 2b801ac84..14f5cc775 100644 --- a/include/arch/aarch64/arch_features.h +++ b/include/arch/aarch64/arch_features.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019-2022, Arm Limited. All rights reserved. + * Copyright (c) 2019-2023, Arm Limited. All rights reserved. * * SPDX-License-Identifier: BSD-3-Clause */ @@ -12,6 +12,9 @@ #include #include +#define ISOLATE_FIELD(reg, feat) \ + ((unsigned int)(((reg) >> (feat ## _SHIFT)) & (feat ## _MASK))) + static inline bool is_armv7_gentimer_present(void) { /* The Generic Timer is always present in an ARMv8-A implementation */ @@ -100,8 +103,7 @@ static inline bool is_armv8_6_twed_present(void) static unsigned int read_feat_fgt_id_field(void) { - return (read_id_aa64mmfr0_el1() >> ID_AA64MMFR0_EL1_FGT_SHIFT) & - ID_AA64MMFR0_EL1_FGT_MASK; + return ISOLATE_FIELD(read_id_aa64mmfr0_el1(), ID_AA64MMFR0_EL1_FGT); } static inline bool is_feat_fgt_supported(void) @@ -134,8 +136,7 @@ static inline bool is_armv8_5_rng_present(void) ******************************************************************************/ static unsigned int read_feat_amu_id_field(void) { - return (read_id_aa64pfr0_el1() >> ID_AA64PFR0_AMU_SHIFT) & - ID_AA64PFR0_AMU_MASK; + return ISOLATE_FIELD(read_id_aa64pfr0_el1(), ID_AA64PFR0_AMU); } static inline bool is_feat_amu_supported(void) @@ -175,8 +176,7 @@ static inline unsigned int get_mpam_version(void) static inline unsigned int read_feat_hcx_id_field(void) { - return (read_id_aa64mmfr1_el1() >> ID_AA64MMFR1_EL1_HCX_SHIFT) & - ID_AA64MMFR1_EL1_HCX_MASK; + return ISOLATE_FIELD(read_id_aa64mmfr1_el1(), ID_AA64MMFR1_EL1_HCX); } static inline bool is_feat_hcx_supported(void) From a4cccb4f6cbbb35d12bd5f8779f3c6d8d762619c Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Wed, 1 Feb 2023 11:46:31 +0000 Subject: [PATCH 2/6] feat(cpufeat): extend check_feature() to deal with min/max So far the check_feature() function compares the subfield of a CPU ID register against 0, to learn if a feature is enabled or not. This is problematic for checks that require a certain revision of a feature, so we should check against a minimum version number instead. On top of that we might need to add code to support newer versions of a feature, so we should be alerted if new hardware introduces a higher number. Extend the check_feature() function to take two extra arguments: the minimum version, and the greatest currently known number. Then make sure that the CPU ID field is in this range. Change-Id: I425b68535a2ba9eafd31854e74d142183b521cd5 Signed-off-by: Andre Przywara --- common/feat_detect.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/common/feat_detect.c b/common/feat_detect.c index a8c40f70d..cbe78c057 100644 --- a/common/feat_detect.c +++ b/common/feat_detect.c @@ -36,19 +36,28 @@ static inline void feature_panic(char *feat_name) /******************************************************************************* * Function : check_feature * Check for a valid combination of build time flags (ENABLE_FEAT_xxx) and - * feature availability on the hardware. - * Panics if a feature is forcefully enabled, but not available on the PE. + * feature availability on the hardware. is the smallest feature + * ID field value that is required for that feature. + * Triggers a panic later if a feature is forcefully enabled, but not + * available on the PE. Also will panic if the hardware feature ID field + * is larger than the maximum known and supported number, specified by . * * We force inlining here to let the compiler optimise away the whole check * if the feature is disabled at build time (FEAT_STATE_DISABLED). ******************************************************************************/ static inline void __attribute((__always_inline__)) -check_feature(int state, unsigned long field, const char *feat_name) +check_feature(int state, unsigned long field, const char *feat_name, + unsigned int min, unsigned int max) { - if (state == FEAT_STATE_ALWAYS && field == 0U) { + if (state == FEAT_STATE_ALWAYS && field < min) { ERROR("FEAT_%s not supported by the PE\n", feat_name); tainted = true; } + if (state >= FEAT_STATE_ALWAYS && field > max) { + ERROR("FEAT_%s is version %ld, but is only known up to version %d\n", + feat_name, field, max); + tainted = true; + } } /****************************************** @@ -312,7 +321,8 @@ void detect_arch_features(void) /* v8.4 features */ read_feat_dit(); - check_feature(ENABLE_FEAT_AMUv1, read_feat_amu_id_field(), "AMUv1"); + check_feature(ENABLE_FEAT_AMUv1, read_feat_amu_id_field(), + "AMUv1", 1, 2); read_feat_mpam(); read_feat_nv2(); read_feat_sel2(); @@ -326,12 +336,12 @@ void detect_arch_features(void) /* v8.6 features */ read_feat_amuv1p1(); - check_feature(ENABLE_FEAT_FGT, read_feat_fgt_id_field(), "FGT"); + check_feature(ENABLE_FEAT_FGT, read_feat_fgt_id_field(), "FGT", 1, 1); read_feat_ecv(); read_feat_twed(); /* v8.7 features */ - check_feature(ENABLE_FEAT_HCX, read_feat_hcx_id_field(), "HCX"); + check_feature(ENABLE_FEAT_HCX, read_feat_hcx_id_field(), "HCX", 1, 1); /* v9.0 features */ read_feat_brbe(); From de8c489247458c00f7b48301fb5c5273c7a628fc Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Wed, 15 Feb 2023 15:56:15 +0000 Subject: [PATCH 3/6] fix(cpufeat): context-switch: move FGT availability check to callers To be inline with other features, and to allow the availability to be checked for different contexts, move the FGT availability check out of the save/restore functions. This is instead now checked at the caller. Change-Id: I96e0638714f9d1b6fdadc1cb989cbd33bd48b1f6 Signed-off-by: Andre Przywara --- lib/el3_runtime/aarch64/context_mgmt.c | 40 +++++++++++++------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/el3_runtime/aarch64/context_mgmt.c b/lib/el3_runtime/aarch64/context_mgmt.c index dab25d681..341c4a5d0 100644 --- a/lib/el3_runtime/aarch64/context_mgmt.c +++ b/lib/el3_runtime/aarch64/context_mgmt.c @@ -805,30 +805,26 @@ void cm_prepare_el3_exit(uint32_t security_state) static void el2_sysregs_context_save_fgt(el2_sysregs_t *ctx) { - if (is_feat_fgt_supported()) { - write_ctx_reg(ctx, CTX_HDFGRTR_EL2, read_hdfgrtr_el2()); - if (is_feat_amu_supported()) { - write_ctx_reg(ctx, CTX_HAFGRTR_EL2, read_hafgrtr_el2()); - } - write_ctx_reg(ctx, CTX_HDFGWTR_EL2, read_hdfgwtr_el2()); - write_ctx_reg(ctx, CTX_HFGITR_EL2, read_hfgitr_el2()); - write_ctx_reg(ctx, CTX_HFGRTR_EL2, read_hfgrtr_el2()); - write_ctx_reg(ctx, CTX_HFGWTR_EL2, read_hfgwtr_el2()); + write_ctx_reg(ctx, CTX_HDFGRTR_EL2, read_hdfgrtr_el2()); + if (is_feat_amu_supported()) { + write_ctx_reg(ctx, CTX_HAFGRTR_EL2, read_hafgrtr_el2()); } + write_ctx_reg(ctx, CTX_HDFGWTR_EL2, read_hdfgwtr_el2()); + write_ctx_reg(ctx, CTX_HFGITR_EL2, read_hfgitr_el2()); + write_ctx_reg(ctx, CTX_HFGRTR_EL2, read_hfgrtr_el2()); + write_ctx_reg(ctx, CTX_HFGWTR_EL2, read_hfgwtr_el2()); } static void el2_sysregs_context_restore_fgt(el2_sysregs_t *ctx) { - if (is_feat_fgt_supported()) { - write_hdfgrtr_el2(read_ctx_reg(ctx, CTX_HDFGRTR_EL2)); - if (is_feat_amu_supported()) { - write_hafgrtr_el2(read_ctx_reg(ctx, CTX_HAFGRTR_EL2)); - } - write_hdfgwtr_el2(read_ctx_reg(ctx, CTX_HDFGWTR_EL2)); - write_hfgitr_el2(read_ctx_reg(ctx, CTX_HFGITR_EL2)); - write_hfgrtr_el2(read_ctx_reg(ctx, CTX_HFGRTR_EL2)); - write_hfgwtr_el2(read_ctx_reg(ctx, CTX_HFGWTR_EL2)); + write_hdfgrtr_el2(read_ctx_reg(ctx, CTX_HDFGRTR_EL2)); + if (is_feat_amu_supported()) { + write_hafgrtr_el2(read_ctx_reg(ctx, CTX_HAFGRTR_EL2)); } + write_hdfgwtr_el2(read_ctx_reg(ctx, CTX_HDFGWTR_EL2)); + write_hfgitr_el2(read_ctx_reg(ctx, CTX_HFGITR_EL2)); + write_hfgrtr_el2(read_ctx_reg(ctx, CTX_HFGRTR_EL2)); + write_hfgwtr_el2(read_ctx_reg(ctx, CTX_HFGWTR_EL2)); } /******************************************************************************* @@ -863,7 +859,9 @@ void cm_el2_sysregs_context_save(uint32_t security_state) el2_sysregs_context_save_mpam(el2_sysregs_ctx); #endif - el2_sysregs_context_save_fgt(el2_sysregs_ctx); + if (is_feat_fgt_supported()) { + el2_sysregs_context_save_fgt(el2_sysregs_ctx); + } #if ENABLE_FEAT_ECV el2_sysregs_context_save_ecv(el2_sysregs_ctx); @@ -921,7 +919,9 @@ void cm_el2_sysregs_context_restore(uint32_t security_state) el2_sysregs_context_restore_mpam(el2_sysregs_ctx); #endif - el2_sysregs_context_restore_fgt(el2_sysregs_ctx); + if (is_feat_fgt_supported()) { + el2_sysregs_context_restore_fgt(el2_sysregs_ctx); + } #if ENABLE_FEAT_ECV el2_sysregs_context_restore_ecv(el2_sysregs_ctx); From f5360cfa819f13ed1caf1cfb774a876bc3f29377 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 17 Nov 2022 16:42:09 +0000 Subject: [PATCH 4/6] refactor(trbe): enable FEAT_TRBE for FEAT_STATE_CHECKED At the moment we only support FEAT_TRBE to be either unconditionally compiled in, or to be not supported at all. Add support for runtime detection (ENABLE_TRBE_FOR_NS=2), by splitting is_feat_trbe_present() into an ID register reading function and a second function to report the support status. That function considers both build time settings and runtime information (if needed), and is used before we access TRBE related registers. The FVP platform decided to compile in support unconditionally (=1), even though FEAT_TRBE is an ARMv9 feature, so is not available with the FVP model's default command line. Change that to the now supported dynamic option (=2), so the right decision can be made by the code at runtime. Change-Id: Iee7f88ea930119049543a8a4a105389997e7692c Signed-off-by: Andre Przywara --- bl31/bl31.mk | 2 +- common/feat_detect.c | 13 ++----------- include/arch/aarch64/arch_features.h | 18 +++++++++++++++--- lib/el3_runtime/aarch64/context_mgmt.c | 6 +++--- lib/extensions/trbe/trbe.c | 22 ++++++++++------------ plat/arm/board/fvp/platform.mk | 2 +- 6 files changed, 32 insertions(+), 31 deletions(-) diff --git a/bl31/bl31.mk b/bl31/bl31.mk index e6609fe86..4d2fc874d 100644 --- a/bl31/bl31.mk +++ b/bl31/bl31.mk @@ -112,7 +112,7 @@ ifeq (${ENABLE_MPAM_FOR_LOWER_ELS},1) BL31_SOURCES += lib/extensions/mpam/mpam.c endif -ifeq (${ENABLE_TRBE_FOR_NS},1) +ifneq (${ENABLE_TRBE_FOR_NS},0) BL31_SOURCES += lib/extensions/trbe/trbe.c endif diff --git a/common/feat_detect.c b/common/feat_detect.c index cbe78c057..3012c8bbe 100644 --- a/common/feat_detect.c +++ b/common/feat_detect.c @@ -258,16 +258,6 @@ static void read_feat_brbe(void) #endif } -/****************************************************** - * Feature : FEAT_TRBE (Trace Buffer Extension) - *****************************************************/ -static void read_feat_trbe(void) -{ -#if (ENABLE_TRBE_FOR_NS == FEAT_STATE_ALWAYS) - feat_detect_panic(is_feat_trbe_present(), "TRBE"); -#endif -} - /****************************************************************** * Feature : FEAT_RNG_TRAP (Trapping support for RNDR/RNDRRS) *****************************************************************/ @@ -345,7 +335,8 @@ void detect_arch_features(void) /* v9.0 features */ read_feat_brbe(); - read_feat_trbe(); + check_feature(ENABLE_TRBE_FOR_NS, read_feat_trbe_id_field(), + "TRBE", 1, 1); /* v9.2 features */ read_feat_rme(); diff --git a/include/arch/aarch64/arch_features.h b/include/arch/aarch64/arch_features.h index 14f5cc775..ded42d477 100644 --- a/include/arch/aarch64/arch_features.h +++ b/include/arch/aarch64/arch_features.h @@ -297,10 +297,22 @@ static inline bool is_feat_brbe_present(void) /******************************************************************************* * Function to identify the presence of FEAT_TRBE (Trace Buffer Extension) ******************************************************************************/ -static inline bool is_feat_trbe_present(void) +static inline unsigned int read_feat_trbe_id_field(void) { - return (((read_id_aa64dfr0_el1() >> ID_AA64DFR0_TRACEBUFFER_SHIFT) & - ID_AA64DFR0_TRACEBUFFER_MASK) == ID_AA64DFR0_TRACEBUFFER_SUPPORTED); + return ISOLATE_FIELD(read_id_aa64dfr0_el1(), ID_AA64DFR0_TRACEBUFFER); } +static inline bool is_feat_trbe_supported(void) +{ + if (ENABLE_TRBE_FOR_NS == FEAT_STATE_DISABLED) { + return false; + } + + if (ENABLE_TRBE_FOR_NS == FEAT_STATE_ALWAYS) { + return true; + } + + return read_feat_trbe_id_field() != 0U; + +} #endif /* ARCH_FEATURES_H */ diff --git a/lib/el3_runtime/aarch64/context_mgmt.c b/lib/el3_runtime/aarch64/context_mgmt.c index 341c4a5d0..1dfb71f76 100644 --- a/lib/el3_runtime/aarch64/context_mgmt.c +++ b/lib/el3_runtime/aarch64/context_mgmt.c @@ -495,9 +495,9 @@ static void manage_extensions_nonsecure(bool el2_unused, cpu_context_t *ctx) mpam_enable(el2_unused); #endif -#if ENABLE_TRBE_FOR_NS - trbe_enable(); -#endif /* ENABLE_TRBE_FOR_NS */ + if (is_feat_trbe_supported()) { + trbe_enable(); + } #if ENABLE_BRBE_FOR_NS brbe_enable(); diff --git a/lib/extensions/trbe/trbe.c b/lib/extensions/trbe/trbe.c index b3463872b..fa139cad2 100644 --- a/lib/extensions/trbe/trbe.c +++ b/lib/extensions/trbe/trbe.c @@ -23,22 +23,20 @@ void trbe_enable(void) { uint64_t val; - if (is_feat_trbe_present()) { - /* - * MDCR_EL3.NSTB = 0b11 - * Allow access of trace buffer control registers from NS-EL1 - * and NS-EL2, tracing is prohibited in Secure and Realm state - * (if implemented). - */ - val = read_mdcr_el3(); - val |= MDCR_NSTB(MDCR_NSTB_EL1); - write_mdcr_el3(val); - } + /* + * MDCR_EL3.NSTB = 0b11 + * Allow access of trace buffer control registers from NS-EL1 + * and NS-EL2, tracing is prohibited in Secure and Realm state + * (if implemented). + */ + val = read_mdcr_el3(); + val |= MDCR_NSTB(MDCR_NSTB_EL1); + write_mdcr_el3(val); } static void *trbe_drain_trace_buffers_hook(const void *arg __unused) { - if (is_feat_trbe_present()) { + if (is_feat_trbe_supported()) { /* * Before switching from normal world to secure world * the trace buffers need to be drained out to memory. This is diff --git a/plat/arm/board/fvp/platform.mk b/plat/arm/board/fvp/platform.mk index efbf68f00..7991c14f6 100644 --- a/plat/arm/board/fvp/platform.mk +++ b/plat/arm/board/fvp/platform.mk @@ -446,7 +446,7 @@ DYN_DISABLE_AUTH := 1 endif # enable trace buffer control registers access to NS by default -ENABLE_TRBE_FOR_NS := 1 +ENABLE_TRBE_FOR_NS := 2 # enable branch record buffer control registers access in NS by default # only enable for aarch64 From ff491036603c34bd80137f5fd43878eae5585197 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 17 Nov 2022 16:42:09 +0000 Subject: [PATCH 5/6] refactor(brbe): enable FEAT_BRBE for FEAT_STATE_CHECKED At the moment we only support FEAT_BRBE to be either unconditionally compiled in, or to be not supported at all. Add support for runtime detection (ENABLE_BRBE_FOR_NS=2), by splitting is_feat_brbe_present() into an ID register reading function and a second function to report the support status. That function considers both build time settings and runtime information (if needed), and is used before we access BRBE related registers. The FVP platform decided to compile in support unconditionally (=1), even though FEAT_BRBE is an ARMv9 feature, so is not available with the FVP model's default command line. Change that to the now supported dynamic option (=2), so the right decision can be made by the code at runtime. Change-Id: I5f2e2c9648300f65f0fa9a5f8e2f34e73529d053 Signed-off-by: Andre Przywara --- bl31/bl31.mk | 2 +- common/feat_detect.c | 13 ++----------- include/arch/aarch64/arch_features.h | 18 +++++++++++++++--- lib/el3_runtime/aarch64/context_mgmt.c | 6 +++--- lib/extensions/brbe/brbe.c | 22 ++++++++++------------ plat/arm/board/fvp/platform.mk | 2 +- 6 files changed, 32 insertions(+), 31 deletions(-) diff --git a/bl31/bl31.mk b/bl31/bl31.mk index 4d2fc874d..ebb664e9a 100644 --- a/bl31/bl31.mk +++ b/bl31/bl31.mk @@ -116,7 +116,7 @@ ifneq (${ENABLE_TRBE_FOR_NS},0) BL31_SOURCES += lib/extensions/trbe/trbe.c endif -ifeq (${ENABLE_BRBE_FOR_NS},1) +ifneq (${ENABLE_BRBE_FOR_NS},0) BL31_SOURCES += lib/extensions/brbe/brbe.c endif diff --git a/common/feat_detect.c b/common/feat_detect.c index 3012c8bbe..4c54cb5f8 100644 --- a/common/feat_detect.c +++ b/common/feat_detect.c @@ -248,16 +248,6 @@ static void read_feat_rme(void) #endif } -/****************************************************** - * Feature : FEAT_BRBE (Branch Record Buffer Extension) - *****************************************************/ -static void read_feat_brbe(void) -{ -#if (ENABLE_BRBE_FOR_NS == FEAT_STATE_ALWAYS) - feat_detect_panic(is_feat_brbe_present(), "BRBE"); -#endif -} - /****************************************************************** * Feature : FEAT_RNG_TRAP (Trapping support for RNDR/RNDRRS) *****************************************************************/ @@ -334,7 +324,8 @@ void detect_arch_features(void) check_feature(ENABLE_FEAT_HCX, read_feat_hcx_id_field(), "HCX", 1, 1); /* v9.0 features */ - read_feat_brbe(); + check_feature(ENABLE_BRBE_FOR_NS, read_feat_brbe_id_field(), + "BRBE", 1, 2); check_feature(ENABLE_TRBE_FOR_NS, read_feat_trbe_id_field(), "TRBE", 1, 1); diff --git a/include/arch/aarch64/arch_features.h b/include/arch/aarch64/arch_features.h index ded42d477..9f6af39af 100644 --- a/include/arch/aarch64/arch_features.h +++ b/include/arch/aarch64/arch_features.h @@ -288,10 +288,22 @@ static inline unsigned int get_armv8_4_feat_nv_support(void) * Function to identify the presence of FEAT_BRBE (Branch Record Buffer * Extension) ******************************************************************************/ -static inline bool is_feat_brbe_present(void) +static inline unsigned int read_feat_brbe_id_field(void) { - return (((read_id_aa64dfr0_el1() >> ID_AA64DFR0_BRBE_SHIFT) & - ID_AA64DFR0_BRBE_MASK) == ID_AA64DFR0_BRBE_SUPPORTED); + return ISOLATE_FIELD(read_id_aa64dfr0_el1(), ID_AA64DFR0_BRBE); +} + +static inline bool is_feat_brbe_supported(void) +{ + if (ENABLE_BRBE_FOR_NS == FEAT_STATE_DISABLED) { + return false; + } + + if (ENABLE_BRBE_FOR_NS == FEAT_STATE_ALWAYS) { + return true; + } + + return read_feat_brbe_id_field() != 0U; } /******************************************************************************* diff --git a/lib/el3_runtime/aarch64/context_mgmt.c b/lib/el3_runtime/aarch64/context_mgmt.c index 1dfb71f76..54e257b75 100644 --- a/lib/el3_runtime/aarch64/context_mgmt.c +++ b/lib/el3_runtime/aarch64/context_mgmt.c @@ -499,9 +499,9 @@ static void manage_extensions_nonsecure(bool el2_unused, cpu_context_t *ctx) trbe_enable(); } -#if ENABLE_BRBE_FOR_NS - brbe_enable(); -#endif /* ENABLE_BRBE_FOR_NS */ + if (is_feat_brbe_supported()) { + brbe_enable(); + } #if ENABLE_SYS_REG_TRACE_FOR_NS sys_reg_trace_enable(ctx); diff --git a/lib/extensions/brbe/brbe.c b/lib/extensions/brbe/brbe.c index 1982619b7..329cf982a 100644 --- a/lib/extensions/brbe/brbe.c +++ b/lib/extensions/brbe/brbe.c @@ -12,16 +12,14 @@ void brbe_enable(void) { uint64_t val; - if (is_feat_brbe_present()) { - /* - * MDCR_EL3.SBRBE = 0b01 - * - * Allows BRBE usage in non-secure world and prohibited in - * secure world. - */ - val = read_mdcr_el3(); - val &= ~(MDCR_SBRBE_MASK << MDCR_SBRBE_SHIFT); - val |= (0x1UL << MDCR_SBRBE_SHIFT); - write_mdcr_el3(val); - } + /* + * MDCR_EL3.SBRBE = 0b01 + * + * Allows BRBE usage in non-secure world and prohibited in + * secure world. + */ + val = read_mdcr_el3(); + val &= ~(MDCR_SBRBE_MASK << MDCR_SBRBE_SHIFT); + val |= (0x1UL << MDCR_SBRBE_SHIFT); + write_mdcr_el3(val); } diff --git a/plat/arm/board/fvp/platform.mk b/plat/arm/board/fvp/platform.mk index 7991c14f6..dda7e6cfe 100644 --- a/plat/arm/board/fvp/platform.mk +++ b/plat/arm/board/fvp/platform.mk @@ -453,7 +453,7 @@ ENABLE_TRBE_FOR_NS := 2 # do not enable when ENABLE_RME=1 ifeq (${ARCH}, aarch64) ifeq (${ENABLE_RME},0) - ENABLE_BRBE_FOR_NS := 1 + ENABLE_BRBE_FOR_NS := 2 endif endif From fc8d2d3980352f92cf378155c1f4449b4a0ab4c0 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Thu, 17 Nov 2022 17:30:43 +0000 Subject: [PATCH 6/6] refactor(trf): enable FEAT_TRF for FEAT_STATE_CHECKED At the moment we only support FEAT_TRF to be either unconditionally compiled in, or to be not supported at all. Add support for runtime detection (ENABLE_TRF_FOR_NS=2), by splitting is_feat_trf_present() into an ID register reading function and a second function to report the support status. That function considers both build time settings and runtime information (if needed), and is used before we access TRF related registers. Also move the context saving code from assembly to C, and use the new is_feat_trf_supported() function to guard its execution. The FVP platform decided to compile in support unconditionally (=1), even though FEAT_TRF is an ARMv8.4 feature, so is not available with the FVP model's default command line. Change that to the now supported dynamic option (=2), so the right decision can be made by the code at runtime. Change-Id: Ia97b01adbe24970a4d837afd463dc5506b7295a3 Signed-off-by: Andre Przywara --- bl31/bl31.mk | 2 +- bl32/sp_min/sp_min.mk | 2 +- common/feat_detect.c | 13 ++-------- include/arch/aarch32/arch_features.h | 19 ++++++++++++++ include/arch/aarch64/arch_features.h | 18 +++++++++++--- include/arch/aarch64/arch_helpers.h | 3 +++ include/lib/el3_runtime/aarch64/context.h | 4 --- lib/el3_runtime/aarch32/context_mgmt.c | 7 +++--- lib/el3_runtime/aarch64/context.S | 24 ------------------ lib/el3_runtime/aarch64/context_mgmt.c | 18 +++++++------- lib/extensions/trf/aarch32/trf.c | 25 ++++++------------- lib/extensions/trf/aarch64/trf.c | 30 +++++++---------------- plat/arm/board/fvp/platform.mk | 2 +- 13 files changed, 71 insertions(+), 96 deletions(-) diff --git a/bl31/bl31.mk b/bl31/bl31.mk index ebb664e9a..e9590d5d6 100644 --- a/bl31/bl31.mk +++ b/bl31/bl31.mk @@ -124,7 +124,7 @@ ifeq (${ENABLE_SYS_REG_TRACE_FOR_NS},1) BL31_SOURCES += lib/extensions/sys_reg_trace/aarch64/sys_reg_trace.c endif -ifeq (${ENABLE_TRF_FOR_NS},1) +ifneq (${ENABLE_TRF_FOR_NS},0) BL31_SOURCES += lib/extensions/trf/aarch64/trf.c endif diff --git a/bl32/sp_min/sp_min.mk b/bl32/sp_min/sp_min.mk index b2f4e4c09..2a6612ad9 100644 --- a/bl32/sp_min/sp_min.mk +++ b/bl32/sp_min/sp_min.mk @@ -50,7 +50,7 @@ ifeq (${ENABLE_SYS_REG_TRACE_FOR_NS},1) BL32_SOURCES += lib/extensions/sys_reg_trace/aarch32/sys_reg_trace.c endif -ifeq (${ENABLE_TRF_FOR_NS},1) +ifneq (${ENABLE_TRF_FOR_NS},0) BL32_SOURCES += lib/extensions/trf/aarch32/trf.c endif diff --git a/common/feat_detect.c b/common/feat_detect.c index 4c54cb5f8..5fb56b992 100644 --- a/common/feat_detect.c +++ b/common/feat_detect.c @@ -162,16 +162,6 @@ static void read_feat_sel2(void) #endif } -/**************************************************** - * Feature : FEAT_TRF (Self-hosted Trace Extensions) - ***************************************************/ -static void read_feat_trf(void) -{ -#if (ENABLE_TRF_FOR_NS == FEAT_STATE_ALWAYS) - feat_detect_panic(is_arm8_4_feat_trf_present(), "TRF"); -#endif -} - /************************************************ * Feature : FEAT_MTE (Memory Tagging Extension) ***********************************************/ @@ -306,7 +296,8 @@ void detect_arch_features(void) read_feat_mpam(); read_feat_nv2(); read_feat_sel2(); - read_feat_trf(); + check_feature(ENABLE_TRF_FOR_NS, read_feat_trf_id_field(), + "TRF", 1, 1); /* v8.5 features */ read_feat_mte(); diff --git a/include/arch/aarch32/arch_features.h b/include/arch/aarch32/arch_features.h index a7d3fe605..a5a5e278b 100644 --- a/include/arch/aarch32/arch_features.h +++ b/include/arch/aarch32/arch_features.h @@ -10,6 +10,7 @@ #include #include +#include #define ISOLATE_FIELD(reg, feat) \ ((unsigned int)(((reg) >> (feat ## _SHIFT)) & (feat ## _MASK))) @@ -24,4 +25,22 @@ static inline bool is_armv8_2_ttcnp_present(void) return ISOLATE_FIELD(read_id_mmfr4(), ID_MMFR4_CNP) != 0U; } +static inline unsigned int read_feat_trf_id_field(void) +{ + return ISOLATE_FIELD(read_id_dfr0(), ID_DFR0_TRACEFILT); +} + +static inline bool is_feat_trf_supported(void) +{ + if (ENABLE_TRF_FOR_NS == FEAT_STATE_DISABLED) { + return false; + } + + if (ENABLE_TRF_FOR_NS == FEAT_STATE_ALWAYS) { + return true; + } + + return read_feat_trf_id_field() != 0U; +} + #endif /* ARCH_FEATURES_H */ diff --git a/include/arch/aarch64/arch_features.h b/include/arch/aarch64/arch_features.h index 9f6af39af..9ff81aa10 100644 --- a/include/arch/aarch64/arch_features.h +++ b/include/arch/aarch64/arch_features.h @@ -268,10 +268,22 @@ static inline bool is_armv8_4_feat_dit_present(void) /************************************************************************* * Function to identify the presence of FEAT_TRF (TraceLift) ************************************************************************/ -static inline bool is_arm8_4_feat_trf_present(void) +static inline unsigned int read_feat_trf_id_field(void) { - return (((read_id_aa64dfr0_el1() >> ID_AA64DFR0_TRACEFILT_SHIFT) & - ID_AA64DFR0_TRACEFILT_MASK) == ID_AA64DFR0_TRACEFILT_SUPPORTED); + return ISOLATE_FIELD(read_id_aa64dfr0_el1(), ID_AA64DFR0_TRACEFILT); +} + +static inline bool is_feat_trf_supported(void) +{ + if (ENABLE_TRF_FOR_NS == FEAT_STATE_DISABLED) { + return false; + } + + if (ENABLE_TRF_FOR_NS == FEAT_STATE_ALWAYS) { + return true; + } + + return read_feat_trf_id_field() != 0U; } /******************************************************************************** diff --git a/include/arch/aarch64/arch_helpers.h b/include/arch/aarch64/arch_helpers.h index 86c1dbe27..5d99778c7 100644 --- a/include/arch/aarch64/arch_helpers.h +++ b/include/arch/aarch64/arch_helpers.h @@ -555,6 +555,9 @@ DEFINE_RENAME_SYSREG_RW_FUNCS(apiakeylo_el1, APIAKeyLo_EL1) /* Armv8.4 Data Independent Timing Register */ DEFINE_RENAME_SYSREG_RW_FUNCS(dit, DIT) +/* Armv8.4 FEAT_TRF Register */ +DEFINE_RENAME_SYSREG_RW_FUNCS(trfcr_el2, TRFCR_EL2) + /* Armv8.5 MTE Registers */ DEFINE_RENAME_SYSREG_RW_FUNCS(tfsre0_el1, TFSRE0_EL1) DEFINE_RENAME_SYSREG_RW_FUNCS(tfsr_el1, TFSR_EL1) diff --git a/include/lib/el3_runtime/aarch64/context.h b/include/lib/el3_runtime/aarch64/context.h index 6986e0e51..57cf5f04d 100644 --- a/include/lib/el3_runtime/aarch64/context.h +++ b/include/lib/el3_runtime/aarch64/context.h @@ -539,10 +539,6 @@ void el2_sysregs_context_restore_ras(el2_sysregs_t *regs); void el2_sysregs_context_save_nv2(el2_sysregs_t *regs); void el2_sysregs_context_restore_nv2(el2_sysregs_t *regs); #endif /* CTX_INCLUDE_NEVE_REGS */ -#if ENABLE_TRF_FOR_NS -void el2_sysregs_context_save_trf(el2_sysregs_t *regs); -void el2_sysregs_context_restore_trf(el2_sysregs_t *regs); -#endif /* ENABLE_TRF_FOR_NS */ #if ENABLE_FEAT_CSV2_2 void el2_sysregs_context_save_csv2(el2_sysregs_t *regs); void el2_sysregs_context_restore_csv2(el2_sysregs_t *regs); diff --git a/lib/el3_runtime/aarch32/context_mgmt.c b/lib/el3_runtime/aarch32/context_mgmt.c index af8edf598..e494a86cf 100644 --- a/lib/el3_runtime/aarch32/context_mgmt.c +++ b/lib/el3_runtime/aarch32/context_mgmt.c @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -143,9 +144,9 @@ static void enable_extensions_nonsecure(bool el2_unused) sys_reg_trace_enable(); #endif /* ENABLE_SYS_REG_TRACE_FOR_NS */ -#if ENABLE_TRF_FOR_NS - trf_enable(); -#endif /* ENABLE_TRF_FOR_NS */ + if (is_feat_trf_supported()) { + trf_enable(); + } #endif } diff --git a/lib/el3_runtime/aarch64/context.S b/lib/el3_runtime/aarch64/context.S index 722b8ae21..d43914848 100644 --- a/lib/el3_runtime/aarch64/context.S +++ b/lib/el3_runtime/aarch64/context.S @@ -41,10 +41,6 @@ .global el2_sysregs_context_save_nv2 .global el2_sysregs_context_restore_nv2 #endif /* CTX_INCLUDE_NEVE_REGS */ -#if ENABLE_TRF_FOR_NS - .global el2_sysregs_context_save_trf - .global el2_sysregs_context_restore_trf -#endif /* ENABLE_TRF_FOR_NS */ #if ENABLE_FEAT_CSV2_2 .global el2_sysregs_context_save_csv2 .global el2_sysregs_context_restore_csv2 @@ -536,26 +532,6 @@ func el2_sysregs_context_restore_nv2 endfunc el2_sysregs_context_restore_nv2 #endif /* CTX_INCLUDE_NEVE_REGS */ -#if ENABLE_TRF_FOR_NS -func el2_sysregs_context_save_trf - /* - * TRFCR_EL2 register is saved only when FEAT_TRF is supported. - */ - mrs x12, TRFCR_EL2 - str x12, [x0, #CTX_TRFCR_EL2] - ret -endfunc el2_sysregs_context_save_trf - -func el2_sysregs_context_restore_trf - /* - * TRFCR_EL2 register is restored only when FEAT_TRF is supported. - */ - ldr x12, [x0, #CTX_TRFCR_EL2] - msr TRFCR_EL2, x12 - ret -endfunc el2_sysregs_context_restore_trf -#endif /* ENABLE_TRF_FOR_NS */ - #if ENABLE_FEAT_CSV2_2 func el2_sysregs_context_save_csv2 /* diff --git a/lib/el3_runtime/aarch64/context_mgmt.c b/lib/el3_runtime/aarch64/context_mgmt.c index 54e257b75..4d2079d23 100644 --- a/lib/el3_runtime/aarch64/context_mgmt.c +++ b/lib/el3_runtime/aarch64/context_mgmt.c @@ -507,9 +507,9 @@ static void manage_extensions_nonsecure(bool el2_unused, cpu_context_t *ctx) sys_reg_trace_enable(ctx); #endif /* ENABLE_SYS_REG_TRACE_FOR_NS */ -#if ENABLE_TRF_FOR_NS - trf_enable(); -#endif /* ENABLE_TRF_FOR_NS */ + if (is_feat_trf_supported()) { + trf_enable(); + } #endif } @@ -875,9 +875,9 @@ void cm_el2_sysregs_context_save(uint32_t security_state) #if CTX_INCLUDE_NEVE_REGS el2_sysregs_context_save_nv2(el2_sysregs_ctx); #endif -#if ENABLE_TRF_FOR_NS - el2_sysregs_context_save_trf(el2_sysregs_ctx); -#endif + if (is_feat_trf_supported()) { + write_ctx_reg(el2_sysregs_ctx, CTX_TRFCR_EL2, read_trfcr_el2()); + } #if ENABLE_FEAT_CSV2_2 el2_sysregs_context_save_csv2(el2_sysregs_ctx); #endif @@ -935,9 +935,9 @@ void cm_el2_sysregs_context_restore(uint32_t security_state) #if CTX_INCLUDE_NEVE_REGS el2_sysregs_context_restore_nv2(el2_sysregs_ctx); #endif -#if ENABLE_TRF_FOR_NS - el2_sysregs_context_restore_trf(el2_sysregs_ctx); -#endif + if (is_feat_trf_supported()) { + write_trfcr_el2(read_ctx_reg(el2_sysregs_ctx, CTX_TRFCR_EL2)); + } #if ENABLE_FEAT_CSV2_2 el2_sysregs_context_restore_csv2(el2_sysregs_ctx); #endif diff --git a/lib/extensions/trf/aarch32/trf.c b/lib/extensions/trf/aarch32/trf.c index 834092d5a..0c63efa70 100644 --- a/lib/extensions/trf/aarch32/trf.c +++ b/lib/extensions/trf/aarch32/trf.c @@ -10,26 +10,15 @@ #include #include -static bool trf_supported(void) -{ - uint32_t features; - - features = read_id_dfr0() >> ID_DFR0_TRACEFILT_SHIFT; - return ((features & ID_DFR0_TRACEFILT_MASK) == - ID_DFR0_TRACEFILT_SUPPORTED); -} - void trf_enable(void) { uint32_t val; - if (trf_supported()) { - /* - * Allow access of trace filter control registers from - * non-monitor mode - */ - val = read_sdcr(); - val &= ~SDCR_TTRF_BIT; - write_sdcr(val); - } + /* + * Allow access of trace filter control registers from + * non-monitor mode + */ + val = read_sdcr(); + val &= ~SDCR_TTRF_BIT; + write_sdcr(val); } diff --git a/lib/extensions/trf/aarch64/trf.c b/lib/extensions/trf/aarch64/trf.c index 1da5dcee0..941692bb4 100644 --- a/lib/extensions/trf/aarch64/trf.c +++ b/lib/extensions/trf/aarch64/trf.c @@ -4,33 +4,21 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include - #include +#include #include #include -static bool trf_supported(void) -{ - uint64_t features; - - features = read_id_aa64dfr0_el1() >> ID_AA64DFR0_TRACEFILT_SHIFT; - return ((features & ID_AA64DFR0_TRACEFILT_MASK) == - ID_AA64DFR0_TRACEFILT_SUPPORTED); -} - void trf_enable(void) { uint64_t val; - if (trf_supported()) { - /* - * MDCR_EL3.TTRF = b0 - * Allow access of trace filter control registers from NS-EL2 - * and NS-EL1 when NS-EL2 is implemented but not used - */ - val = read_mdcr_el3(); - val &= ~MDCR_TTRF_BIT; - write_mdcr_el3(val); - } + /* + * MDCR_EL3.TTRF = b0 + * Allow access of trace filter control registers from NS-EL2 + * and NS-EL1 when NS-EL2 is implemented but not used + */ + val = read_mdcr_el3(); + val &= ~MDCR_TTRF_BIT; + write_mdcr_el3(val); } diff --git a/plat/arm/board/fvp/platform.mk b/plat/arm/board/fvp/platform.mk index dda7e6cfe..9b09a6bb9 100644 --- a/plat/arm/board/fvp/platform.mk +++ b/plat/arm/board/fvp/platform.mk @@ -461,7 +461,7 @@ endif ENABLE_SYS_REG_TRACE_FOR_NS := 1 # enable trace filter control registers access to NS by default -ENABLE_TRF_FOR_NS := 1 +ENABLE_TRF_FOR_NS := 2 # Linux relies on EL3 enablement if those features are present ENABLE_FEAT_FGT := 2