Re: [PATCH v4 4/5] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

On 1/19/21 2:34 PM, Catalin Marinas wrote:
> On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote:
>>  static void update_sctlr_el1_tcf0(u64 tcf0)
>>  {
>>  	/* ISB required for the kernel uaccess routines */
>> @@ -235,6 +273,15 @@ void mte_thread_switch(struct task_struct *next)
>>  	/* avoid expensive SCTLR_EL1 accesses if no change */
>>  	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
>>  		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
>> +
>> +	/*
>> +	 * Check if an async tag exception occurred at EL1.
>> +	 *
>> +	 * Note: On the context switch path we rely on the dsb() present
>> +	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
>> +	 * are synchronized before this point.
>> +	 */
>> +	mte_check_tfsr_el1();
>>  }
> 
> We need an isb() before mte_check_tfsr_el1() here as well, we only have
> a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0()
> but only if the check passed. Now, it's worth benchmarking how expensive
> update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with
> something like hackbench) and we could probably remove the check
> altogether. In the meantime, you can add an isb() on the "else" path of
> the above check.
> 

Good catch, I saw the isb() in update_sctlr_el1_tcf0() and for some reasons that
it is not escaping me I thought it was sufficient, but clearly it is not.

I am happy to benchmark what you are suggesting and provide some data after this
series is merged (if it works for you) so that we can decide. In the meantime as
you suggested I will fix the "else" for v5.

-- 
Regards,
Vincenzo