lkml.org 
[lkml]   [2018]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] arm64: Add support for new control bits CTR_EL0.DIC and CTR_EL0.IDC
From
Date
On 21/02/18 16:14, Shanker Donthineni wrote:
[...]
>>> @@ -1100,6 +1114,20 @@ static int cpu_copy_el2regs(void *__unused)
>>> .enable = cpu_clear_disr,
>>> },
>>> #endif /* CONFIG_ARM64_RAS_EXTN */
>>> +#ifdef CONFIG_ARM64_SKIP_CACHE_POU
>>> + {
>>> + .desc = "DCache clean to POU",
>>
>> This description is confusing, and sounds like it's describing DC CVAU, rather
>> than the ability to ellide it. How about:
>>
>
> Sure, I'll take your suggestion.

Can we at least spell "elision" correctly please? ;)

Personally I read DIC and IDC as "D-cache to I-cache coherency" and
"I-cache to D-cache coherency" respectively (just my interpretation,
I've not looked into the spec work for any hints of rationale), but out
loud those do sound so poorly-defined that keeping things in terms of
the required maintenance probably is better.

>> .desc = "D-cache maintenance ellision (IDC)"
>>
>>> + .capability = ARM64_HAS_CACHE_IDC,
>>> + .def_scope = SCOPE_SYSTEM,
>>> + .matches = has_cache_idc,
>>> + },
>>> + {
>>> + .desc = "ICache invalidation to POU",
>>
>> ... and correspondingly:
>>
>> .desc = "I-cache maintenance ellision (DIC)"
>>
>>> + .capability = ARM64_HAS_CACHE_DIC,
>>> + .def_scope = SCOPE_SYSTEM,
>>> + .matches = has_cache_dic,
>>> + },
>>> +#endif /* CONFIG_ARM64_CACHE_DIC */
>>> {},
>>> };
[...]
>>> +alternative_if ARM64_HAS_CACHE_DIC
>>> + isb
>>
>> Why have we gained an ISB here if DIC is set?
>>
>
> I believe synchronization barrier (ISB) is required here to support self-modifying/jump-labels
> code.
>
>> This is for a user address, and I can't see why DIC would imply we need an
>> extra ISB kernel-side.
>>
>
> This is for user and kernel addresses, alternatives and jumplabel patching logic
> calls flush_icache_range().

There's an ISB hidden in invalidate_icache_by_line(), so it probably
would be unsafe to start implicitly skipping that.

>>> + b 8f
>>> +alternative_else_nop_endif
>>> invalidate_icache_by_line x0, x1, x2, x3, 9f
>>> - mov x0, #0
>>> +8: mov x0, #0
>>> 1:
>>> uaccess_ttbr0_disable x1, x2
>>> ret
>>> @@ -80,6 +87,12 @@ ENDPROC(__flush_cache_user_range)
>>> * - end - virtual end address of region
>>> */
>>> ENTRY(invalidate_icache_range)
>>> +alternative_if ARM64_HAS_CACHE_DIC
>>> + mov x0, xzr
>>> + dsb ish
>>
>> Do we actually need a DSB in this case?
>>
>
> I'll remove if everyone agree.
>
> Will, Can you comment on this?
>
>> As-is, this function *only* invalidates the I-cache, so we already assume that
>> the data is visible at the PoU at this point. I don't see what extra gaurantee
>> we'd need the DSB for.

If so, then ditto for the existing invalidate_icache_by_line() code
presumably.

Robin.

\
 
 \ /
  Last update: 2018-02-21 17:53    [W:0.070 / U:0.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site