Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround

From: D Scott Phillips <scott@os.amperecomputing.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
	Darren Hart <darren@os.amperecomputing.com>,
	patches@amperecomputing.com
Subject: Re: [PATCH v2] arm64: errata: Fix exec handling in erratum 1418040 workaround
Date: Fri, 17 Dec 2021 11:29:06 -0800	[thread overview]
Message-ID: <86o85ft4hp.fsf@scott-ph-mail.amperecomputing.com> (raw)
In-Reply-To: <YbzZQexVR6eULEVa@arm.com>

Catalin Marinas <catalin.marinas@arm.com> writes:

> On Fri, Dec 17, 2021 at 08:42:01AM -0800, D Scott Phillips wrote:
>> Catalin Marinas <catalin.marinas@arm.com> writes:
>> > On Thu, Dec 16, 2021 at 11:16:18AM -0800, D Scott Phillips wrote:
>> >> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>> >> index 97932fbf973d..24036b914226 100644
>> >> --- a/arch/arm64/include/asm/elf.h
>> >> +++ b/arch/arm64/include/asm/elf.h
>> >> @@ -160,8 +160,16 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>> >>  
>> >>  #define SET_PERSONALITY(ex)						\
>> >>  ({									\
>> >> +	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040))			\
>> >> +		preempt_disable();					\
>> >>  	clear_thread_flag(TIF_32BIT);					\
>> >>  	current->personality &= ~READ_IMPLIES_EXEC;			\
>> >> +	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) {			\
>> >> +		if (this_cpu_has_cap(ARM64_WORKAROUND_1418040))		\
>> >> +			sysreg_clear_set(cntkctl_el1, 0,		\
>> >> +					 ARCH_TIMER_USR_VCT_ACCESS_EN);	\
>> >> +		preempt_enable();					\
>> >> +	}								\
>> >>  })
>> >>  
>> >>  /* update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes */
>> >> @@ -223,7 +231,16 @@ int compat_elf_check_arch(const struct elf32_hdr *);
>> >>   */
>> >>  #define COMPAT_SET_PERSONALITY(ex)					\
>> >>  ({									\
>> >> +	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040))			\
>> >> +		preempt_disable();					\
>> >>  	set_thread_flag(TIF_32BIT);					\
>> >> +	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040)) {			\
>> >> +		if (this_cpu_has_cap(ARM64_WORKAROUND_1418040))		\
>> >> +			sysreg_clear_set(cntkctl_el1,			\
>> >> +					 ARCH_TIMER_USR_VCT_ACCESS_EN,	\
>> >> +					 0);				\
>> >> +		preempt_enable();					\
>> >> +	}								\
>> >
>> > I don't particularly like adding more to these macros. There's
>> > arch_setup_new_exec() that gets called after SET_PERSONALITY, so you can
>> > check whether the task is compat or not.
>> 
>> If the task is preemptible between the update to TIF_32BIT and the
>> update to cntkctl then a window exists where preemption will confuse the
>> workaround logic we have in switch_to, causing the desync between compat
>> state and counter access trapping to propagate into other tasks.
>> 
>> Agreed that this change gunks up a previously simple macro though. Maybe
>> we could have SET_PERSONALITY only set a flag in arch_elf_state and
>> arch_setup_new_exec() take over setting TIF_32BIT? Or move
>> set_personality's implementation to a helper? Or something else?
>
> Is reading ckntkctl_el1 as expensive as the write? If not we could
> change the __switch_to() logic to simply check that the bit is set or
> cleared depending on is_compat_thread() and skip the comparison with the
> previous task.

Ah, yes that's a good idea. From a quick check on an N1 core, it looks
like reading cntkctl has about the same time cost as reading a general
purpose register. I'd assume the other cores affected by the erratum
would be similar, so I'll go that route. Thanks,

Scott

> -- 
> Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel