diff mbox series

[v5,2/7] arm64: trap to EL1 accesses to AMU counters from EL0

Message ID 20200226132947.29738-3-ionela.voinescu@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: ARMv8.4 Activity Monitors support | expand

Commit Message

Ionela Voinescu Feb. 26, 2020, 1:29 p.m. UTC 
The activity monitors extension is an optional extension introduced
by the ARMv8.4 CPU architecture. In order to access the activity
monitors counters safely, if desired, the kernel should detect the
presence of the extension through the feature register, and mediate
the access.

Therefore, disable direct accesses to activity monitors counters
from EL0 (userspace) and trap them to EL1 (kernel).

To be noted that the ARM64_AMU_EXTN kernel config and the disable_amu
kernel parameter do not have an effect on this code. Given that the
amuserenr_el0 resets to an UNKNOWN value, setting the trap of EL0
accesses to EL1 is always attempted for safety and security
considerations. Therefore firmware should still ensure accesses to
AMU registers are not trapped in EL2/EL3 as this code cannot be
bypassed if the CPU implements the Activity Monitors Unit.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Steve Capper <steve.capper@arm.com>
---
 arch/arm64/include/asm/assembler.h | 10 ++++++++++
 arch/arm64/mm/proc.S               |  3 +++
 2 files changed, 13 insertions(+)

Comments

James Morse Feb. 28, 2020, 4:44 p.m. UTC | #1 
Hi Ionela,

On 26/02/2020 13:29, Ionela Voinescu wrote:
> The activity monitors extension is an optional extension introduced
> by the ARMv8.4 CPU architecture. In order to access the activity
> monitors counters safely, if desired, the kernel should detect the
> presence of the extension through the feature register, and mediate
> the access.
> 
> Therefore, disable direct accesses to activity monitors counters
> from EL0 (userspace) and trap them to EL1 (kernel).
> 
> To be noted that the ARM64_AMU_EXTN kernel config and the disable_amu
> kernel parameter do not have an effect on this code. Given that the
> amuserenr_el0 resets to an UNKNOWN value, setting the trap of EL0
> accesses to EL1 is always attempted for safety and security
> considerations.

> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index aafed6902411..7103027b4e64 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S

> @@ -131,6 +131,7 @@ alternative_endif
>  	ubfx	x11, x11, #1, #1
>  	msr	oslar_el1, x11
>  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
> +	reset_amuserenr_el0 x0			// Disable AMU access from EL0
>  
>  alternative_if ARM64_HAS_RAS_EXTN
>  	msr_s	SYS_DISR_EL1, xzr

(This above hunk is in: cpu_do_resume, and this next one is __cpu_setup,)

> @@ -423,6 +424,8 @@ SYM_FUNC_START(__cpu_setup)
>  	isb					// Unmask debug exceptions now,
>  	enable_dbg				// since this is per-cpu
>  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
> +	reset_amuserenr_el0 x0			// Disable AMU access from EL0

I think you only need this in __cpu_setup. The entry-point from cpu-idle calls:
| cpu_resume
| ->__cpu_setup
| -->reset_amuserenr_el0
| ->_cpu_resume
| -->cpu_do_resume
| --->reset_amuserenr_el0

(Which means the PMU reset call is redundant too).

Its harmless, and needs cleaning up already, so regardless:
Reviewed-by: James Morse <james.morse@arm.com>



Thanks,

James
Ionela Voinescu March 4, 2020, 12:29 a.m. UTC | #2 
Hi James,

On Friday 28 Feb 2020 at 16:44:53 (+0000), James Morse wrote:
> Hi Ionela,
> 
> On 26/02/2020 13:29, Ionela Voinescu wrote:
> > The activity monitors extension is an optional extension introduced
> > by the ARMv8.4 CPU architecture. In order to access the activity
> > monitors counters safely, if desired, the kernel should detect the
> > presence of the extension through the feature register, and mediate
> > the access.
> > 
> > Therefore, disable direct accesses to activity monitors counters
> > from EL0 (userspace) and trap them to EL1 (kernel).
> > 
> > To be noted that the ARM64_AMU_EXTN kernel config and the disable_amu
> > kernel parameter do not have an effect on this code. Given that the
> > amuserenr_el0 resets to an UNKNOWN value, setting the trap of EL0
> > accesses to EL1 is always attempted for safety and security
> > considerations.
> 
> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index aafed6902411..7103027b4e64 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> 
> > @@ -131,6 +131,7 @@ alternative_endif
> >  	ubfx	x11, x11, #1, #1
> >  	msr	oslar_el1, x11
> >  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
> > +	reset_amuserenr_el0 x0			// Disable AMU access from EL0
> >  
> >  alternative_if ARM64_HAS_RAS_EXTN
> >  	msr_s	SYS_DISR_EL1, xzr
> 
> (This above hunk is in: cpu_do_resume, and this next one is __cpu_setup,)
> 
> > @@ -423,6 +424,8 @@ SYM_FUNC_START(__cpu_setup)
> >  	isb					// Unmask debug exceptions now,
> >  	enable_dbg				// since this is per-cpu
> >  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
> > +	reset_amuserenr_el0 x0			// Disable AMU access from EL0
> 
> I think you only need this in __cpu_setup. The entry-point from cpu-idle calls:
> | cpu_resume
> | ->__cpu_setup
> | -->reset_amuserenr_el0
> | ->_cpu_resume
> | -->cpu_do_resume
> | --->reset_amuserenr_el0
> 
> (Which means the PMU reset call is redundant too).
> 

Thanks, that seems to be so. I'll submit a separate fix for both amu
and pmu, if you don't mind, after this set, so it will be a specific
cleanup patch.

> Its harmless, and needs cleaning up already, so regardless:
> Reviewed-by: James Morse <james.morse@arm.com>
> 

Thank you very much for the review,
Ionela.

> 
> 
> Thanks,
> 
> James
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index aca337d79d12..c5487806273f 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -430,6 +430,16 @@  USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 9000:
 	.endm
 
+/*
+ * reset_amuserenr_el0 - reset AMUSERENR_EL0 if AMUv1 present
+ */
+	.macro	reset_amuserenr_el0, tmpreg
+	mrs	\tmpreg, id_aa64pfr0_el1	// Check ID_AA64PFR0_EL1
+	ubfx	\tmpreg, \tmpreg, #ID_AA64PFR0_AMU_SHIFT, #4
+	cbz	\tmpreg, .Lskip_\@		// Skip if no AMU present
+	msr_s	SYS_AMUSERENR_EL0, xzr		// Disable AMU access from EL0
+.Lskip_\@:
+	.endm
 /*
  * copy_page - copy src to dest using temp registers t1-t8
  */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index aafed6902411..7103027b4e64 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -131,6 +131,7 @@  alternative_endif
 	ubfx	x11, x11, #1, #1
 	msr	oslar_el1, x11
 	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
+	reset_amuserenr_el0 x0			// Disable AMU access from EL0
 
 alternative_if ARM64_HAS_RAS_EXTN
 	msr_s	SYS_DISR_EL1, xzr
@@ -423,6 +424,8 @@  SYM_FUNC_START(__cpu_setup)
 	isb					// Unmask debug exceptions now,
 	enable_dbg				// since this is per-cpu
 	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
+	reset_amuserenr_el0 x0			// Disable AMU access from EL0
+
 	/*
 	 * Memory region attributes
 	 */