Re: [PATCH v4] arm64: Add workaround for Fujitsu A64FX erratum 010001

[James Morse]

Hi guys,

On 14/02/2019 15:56, Mark Rutland wrote:
> On Thu, Feb 14, 2019 at 07:26:24AM +0000, Zhang, Lei wrote:
>> On the Fujitsu-A64FX cores ver(1.0, 1.1), memory access may
>> cause an undefined fault (Data abort, DFSC=0b111111).
>> This fault occurs under a specific hardware condition when a
>> load/store instruction performs an address translation.
>> Any load/store instruction, except non-fault access
>> including Armv8 and SVE might cause this undefined fault.
>>
>> Since this erratum occurs only when TCR_ELx.NFD1=1,
>> I keep TCR_ELx.NFD1=0 during EL1/EL2.
>>
>> By doing above, the erratum occurs only in EL0.
>> I deal with this erratum in EL0 by a new fault handler
>> which ignores this undefined fault.

>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index a4168d3..7c76c66 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -643,6 +643,29 @@ config QCOM_FALKOR_ERRATUM_E1041
>>  
>>  	  If unsure, say Y.
>>  
>> +config FUJITSU_ERRATUM_010001
>> +	bool "Fujitsu-A64FX erratum E#010001: Undefined fault may occur wrongly"
>> +        depends on RANDOMIZE_BASE
>> +        default RANDOMIZE_BASE
>> +	help
>> +	  This option adds workaround for Fujitsu-A64FX erratum E#010001.
>> +	  On some variants of the Fujitsu-A64FX cores ver(1.0, 1.1), memory accesses
>> +	  may cause undefined fault (Data abort, DFSC=0b111111).
>> +	  This fault occurs under a specific hardware condition when a load/store
>> +          instruction performs an address translation using:
>> +	  case-1  TTBR0_EL1 with TCR_EL1.NFD0 == 1.
>> +	  case-2  TTBR0_EL2 with TCR_EL2.NFD0 == 1.
>> +	  case-3  TTBR1_EL1 with TCR_EL1.NFD1 == 1.
>> +	  case-4  TTBR1_EL2 with TCR_EL2.NFD1 == 1.
>> +
>> +	  The workaround is to set '0' to TCR_ELx.NFD1 at kernel-entry,
>> +	  to set '1' at kernel-exit. And also replace the fault handler
>> +	  for Data abort DFSC=0b111111 with a new fault handler to ignore this
>> +	  undefined fault.
>> +	  The workaround only affect the Fujitsu-A64FX.
> 
> I think that per Will's last comment, the expectation was that NFD1
> would be configured once in C code, rather than in the entry assembly.
> 
> Your code seems to expect KPTI is enabled, and if that's the case, NFD1
> doesn't provide much security benefit,

We only set NFD1 when KASLR is in use, which would also enable KPTI if
CONFIG_UNMAP_KERNEL_AT_EL0 is compiled in. Details in e03e61c3173 ("arm64: kaslr: Set
TCR_EL1.NFD1 when CONFIG_RANDOMIZE_BASE=y")


> and it would be vastly simpler to set this in a cpufeature callback.

A64FX needs it to be unset unless you can handle the spurious fault. The
cpufeature callback would need to run for un-affected CPUs to set the TCR bit.


>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 0ec0c46..34a4f44 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -940,6 +940,14 @@ alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
>>  	dsb	nsh
>>  alternative_else_nop_endif
>>  #endif /* CONFIG_QCOM_FALKOR_ERRATUM_1003 */
>> +#ifdef CONFIG_FUJITSU_CPU_PART_A64FX
>> +alternative_if ARM64_WORKAROUND_FUJITSU_A64FX_0100001
>> +	mrs	\tmp, tcr_el1
>> +	and	\tmp, \tmp, #0xffbfffffffffffff
> 
> If this has to be written in assembly, it would be better as:
> 
> 	bic	\tmp, \tmp, #TCR_NFD1
> 	
>> +	msr	tcr_el1,\tmp
>> +	isb
>> +alternative_else_nop_endif
>> +#endif /* CONFIG_FUJITSU_CPU_PART_A64FX */
>>  	.endm

This TCR swivel has to be done before we make any memory accesses. If we ever
need another register in the trampoline we're going to be in trouble.


>>  	.macro tramp_unmap_kernel, tmp
>> @@ -952,6 +960,14 @@ alternative_else_nop_endif
>>  	 * it's only needed by Cavium ThunderX, which requires KPTI to be
>>  	 * disabled.
>>  	 */
>> +#ifdef CONFIG_FUJITSU_CPU_PART_A64FX
>> +alternative_if ARM64_WORKAROUND_FUJITSU_A64FX_0100001
>> +	mrs	\tmp, tcr_el1
>> +	orr	\tmp, \tmp, #0x40000000000000
> 
> Likewise:
> 	
> 	orr	\tmp, \tmp, #TCR_NFD1
> 
>> +	msr	tcr_el1,\tmp
>> +	isb
>> +alternative_else_nop_endif
>> +#endif /* CONFIG_FUJITSU_CPU_PART_A64FX */
>>  	.endm
>>  
>>  	.macro tramp_ventry, regsize = 64

>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index efb7b2c..1bf0377 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -666,6 +666,20 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>  	return 0;
>>  }
>>  
>> +static int do_bad_unknown_63(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>> +{
>> +	/*
>> +	 * On some variants of the Fujitsu-A64FX cores ver(1.0, 1.1),
>> +	 * memory accesses may spuriously trigger data aborts with
>> +	 * DFSC=0b111111.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_FUJITSU_ERRATUM_010001) &&
>> +		cpus_have_cap(ARM64_WORKAROUND_FUJITSU_A64FX_0100001))
>> +			return 0;
>> +	return do_bad(addr, esr, regs);
>> +}
> 
> If we always left NFD1 clear on A64FX, we would not need this exception
> handler, as this should never occur.

I think we should do this: never set NFDx on A64FX. I don't think we can maintain the TCR
swivel before any memory access in the KPTI trampoline. (It already uses the FAR as a
scratch register!)

The errata means we can't use these bits. Its simpler than trying to work around the symptoms.


>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 73886a5..75f7d99 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -453,9 +453,29 @@ ENTRY(__cpu_setup)
>>  	 * Set/prepare TCR and TTBR. We use 512GB (39-bit) address range for
>>  	 * both user and kernel.
>>  	 */
>> +#ifdef CONFIG_FUJITSU_ERRATUM_010001
>>  	ldr	x10, =TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \
>>  			TCR_TG_FLAGS | TCR_KASLR_FLAGS | TCR_ASID16 | \
>>  			TCR_TBI0 | TCR_A1 | TCR_KASAN_FLAGS
>> +	/* Can use x19/x20/x5 */
>> +	mrs     x19, midr_el1
>> +	/* ERRATA_MIDR_RANGE(MIDR_FUJITSU_A64FX, 0, 0, 1, 0) */
>> +	mov	w20, #0x10	//#16
>> +	movk	w20, #0x460f, lsl #16
>> +	mov	w5, #0xffefffff

Hmmm.... this is masking out the least-significant variant bit, to make it match
your 1.0 and 1.1. This could be much more readable with some pre-processor trickery.

We should also move it out to a macro so the average reader of __cpu_setup() doesn't need
to parse it.


> Please do this detection and reconfiguration of TCR in C code, rather
> than in the __cpu_setup code.
> 
> In proc.S you can have:
> 
> #if defined(CONFIG_RANDOMIZE_BASE) && !defined(CONFIG_FUJITSU_ERRATUM_010001)
> #define KASLR_FLAGS	TCR_NFD1
> #else
> #define KASLR_FLAGS	0
> #endif
> 
> ... and in cpu_errata.c you can enable NFD1 on any CPU which is not
> A64FX.

I think having an errata callback that runs on unaffected cores is tricky. ("CPU
feature: Not affected by E010001").
As a halfway house, I have the below[0] simplified version of Zhang Lei's patch. It
doesn't do the TCR swivel in C, just masks out the TCR values on affected CPUs. I
obviously haven't tested this on an affected platform.


Thanks,

James

--------------------%<--------------------