Re: [PATCH v2 1/2] arm64: Move handling of erratum 1418040 into C code

[Marc Zyngier <maz@kernel.org>]

On 2020-07-31 11:41, Will Deacon wrote:
> On Fri, Jul 31, 2020 at 09:33:57AM +0100, Marc Zyngier wrote:
>> Instead of dealing with erratum 1418040 on each entry and exit,
>> let's move the handling to __switch_to() instead, which has
>> several advantages:
>> 
>> - It can be applied when it matters (switching between 32 and 64
>>   bit tasks).
>> - It is written in C (yay!)
>> - It can rely on static keys rather than alternatives
>> 
>> Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kernel/entry.S   | 21 ---------------------
>>  arch/arm64/kernel/process.c | 35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+), 21 deletions(-)
> 
> [...]
> 
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 6089638c7d43..8bbf066224ab 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -515,6 +515,40 @@ static void entry_task_switch(struct task_struct 
>> *next)
>>  	__this_cpu_write(__entry_task, next);
>>  }
>> 
>> +/*
>> + * ARM erratum 1418040 handling, affecting the 32bit view of CNTVCT.
>> + * Assuming the virtual counter is enabled at the beginning of times:
>> + *
>> + * - disable access when switching from a 64bit task to a 32bit task
>> + * - enable access when switching from a 32bit task to a 64bit task
>> + */
>> +static __always_inline
> 
> Do we need the __always_inline? None of the other calls from 
> __switch_to()
> have it.

Suggestion from Stephen. In my experience, it doesn't change much as
most things get inlined anyway. Happy to drop it.

> 
>> +void erratum_1418040_thread_switch(struct task_struct *prev,
>> +				   struct task_struct *next)
>> +{
>> +	bool prev32, next32;
>> +	u64 val;
>> +
>> +	if (!(IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&
>> +	      cpus_have_const_cap(ARM64_WORKAROUND_1418040)))
>> +		return;
>> +
>> +	prev32 = is_compat_thread(task_thread_info(prev));
>> +	next32 = is_compat_thread(task_thread_info(next));
>> +
>> +	if (prev32 == next32)
>> +		return;
>> +
>> +	val = read_sysreg(cntkctl_el1);
>> +
>> +	if (prev32 & !next32)
> 
> I know they're bools but this is perverse!

Well, this is me writing this code, so don't attribute to perversity
what can adequately be explained by a silly typo... ;-)

> Why can't it just be:
> 
> 	if (next32)
> 		val &= ~ARCH_TIMER_USR_VCT_ACCESS_EN;
> 	else
> 		val |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> 
> ?

Yup, that's better. Do you want to apply these changes directly,
or should I repin it?

         M.
-- 
Jazz is not dead. It just smells funny...

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