linux-kernel - Re: [PATCH v2] arm64: Add support for new control bits CTR_EL0.DIC and CTR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <23d70753-a628-f2e4-84df-39e4021337f5@codeaurora.org>
Date:   Wed, 21 Feb 2018 07:10:34 -0600
From:   Shanker Donthineni <shankerd@...eaurora.org>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Thomas Speier <tspeier@...eaurora.org>,
        Vikram Sethi <vikrams@...eaurora.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Philip Elcan <pelcan@...eaurora.org>,
        kvmarm <kvmarm@...ts.cs.columbia.edu>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] arm64: Add support for new control bits CTR_EL0.DIC
 and CTR_EL0.IDC

Hi Catalin,

On 02/21/2018 05:12 AM, Catalin Marinas wrote:
> On Mon, Feb 19, 2018 at 08:59:06PM -0600, Shanker Donthineni wrote:
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index f55fe5b..4061210 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1095,6 +1095,27 @@ config ARM64_RAS_EXTN
>>  	  and access the new registers if the system supports the extension.
>>  	  Platform RAS features may additionally depend on firmware support.
>>  
>> +config ARM64_CACHE_IDC
>> +	bool "Enable support for DCache clean PoU optimization"
>> +	default y
>> +	help
>> +	  The data cache clean to the point of unification is not required
>> +	  for instruction to be data coherence if CTR_EL0.IDC has value 1.
>> +
>> +	  Selecting this feature will allow the kernel to optimize the POU
>> +	  cache maintaince operations where it requires 'DC CVAU'.
>> +
>> +config ARM64_CACHE_DIC
>> +	bool "Enable support for ICache invalidation PoU optimization"
>> +	default y
>> +	help
>> +	  Instruction cache invalidation to the point of unification is not
>> +	  required for instruction to be data coherence if CTR_EL0.DIC has
>> +	  value 1.
>> +
>> +	  Selecting this feature will allow the kernel to optimize the POU
>> +	  cache maintaince operations where it requires 'IC IVAU'.
> 
> A single Kconfig entry is sufficient for both features.
> 

I'll do in v3 patch.

>> @@ -864,6 +864,22 @@ static bool has_no_fpsimd(const struct arm64_cpu_capabilities *entry, int __unus
>>  					ID_AA64PFR0_FP_SHIFT) < 0;
>>  }
>>  
>> +#ifdef CONFIG_ARM64_CACHE_IDC
>> +static bool has_cache_idc(const struct arm64_cpu_capabilities *entry,
>> +			  int __unused)
>> +{
>> +	return !!(read_sanitised_ftr_reg(SYS_CTR_EL0) & (1UL << CTR_IDC_SHIFT));
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_ARM64_CACHE_DIC
>> +static bool has_cache_dic(const struct arm64_cpu_capabilities *entry,
>> +			  int __unused)
>> +{
>> +	return !!(read_sanitised_ftr_reg(SYS_CTR_EL0) & (1UL << CTR_DIC_SHIFT));
>> +}
>> +#endif
> 
> Nitpick: no need for !! since the function type is bool already.
> 

Sure, I'll remove '!!'.

>> diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
>> index 758bde7..7d37d71 100644
>> --- a/arch/arm64/mm/cache.S
>> +++ b/arch/arm64/mm/cache.S
>> @@ -50,6 +50,9 @@ ENTRY(flush_icache_range)
>>   */
>>  ENTRY(__flush_cache_user_range)
>>  	uaccess_ttbr0_enable x2, x3, x4
>> +alternative_if ARM64_HAS_CACHE_IDC
>> +	b	8f
>> +alternative_else_nop_endif
>>  	dcache_line_size x2, x3
>>  	sub	x3, x2, #1
>>  	bic	x4, x0, x3
>> @@ -60,6 +63,11 @@ user_alt 9f, "dc cvau, x4",  "dc civac, x4",  ARM64_WORKAROUND_CLEAN_CACHE
>>  	b.lo	1b
>>  	dsb	ish
>>  
>> +8:
>> +alternative_if ARM64_HAS_CACHE_DIC
>> +	mov	x0, #0
>> +	b	1f
>> +alternative_else_nop_endif
>> 	invalidate_icache_by_line x0, x1, x2, x3, 9f
>> 	mov	x0, #0
>>  1:
> 
> You can add another label at mov x0, #0 below this hunk and keep a
> single instruction in the alternative path.
> 
> However, my worry is that in an implementation with DIC set, we also
> skip the DSB/ISB sequence in the invalidate_icache_by_line macro. For
> example, in an implementation with transparent PoU, we could have:
> 
> 	str	<some instr>, [addr]
> 	// no cache maintenance or barrier
> 	br	<addr>
> 

Thanks for pointing out the missing barriers. I think it make sense to follow
the existing barrier semantics in order to avoid the unknown things.
 
> Is an ISB required between the instruction store and execution? I would
> say yes but maybe Will has a better opinion here.
> 
Agree, an ISB is required especially for self-modifying code. I'll include in v3 patch. 

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.