Subject: Re: [PATCH v6 09/18] arm64: enable ptrauth earlier

From: Vincenzo Frascino <[email-masked]>
Date: Wed, 11 Mar 2020 10:49:25 +0000

Hi Amit,

On 3/11/20 10:46 AM, Amit Kachhap wrote:
> Hi Vincenzo,
> 
> On 3/11/20 3:56 PM, Vincenzo Frascino wrote:
>> Hi Amit,
>>
>> On 3/11/20 6:26 AM, Amit Kachhap wrote:
>>
>> [...]
>>
>>>>
>>>> My expectation is that you should call early_park_cpu to do that if the
>>>> secondary does not support PTRAUTH similar to what you did in v2 of this
>>>> series:
>>>>
>>>> ENTRY(__cpu_secondary_checkptrauth)
>>>> #ifdef CONFIG_ARM64_PTR_AUTH
>>>>          /* Check if the CPU supports ptrauth */
>>>>          mrs     x2, id_aa64isar1_el1
>>>>          ubfx    x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8
>>>>          cbnz    x2, 1f
>>>> alternative_if ARM64_HAS_ADDRESS_AUTH
>>>>          mov     x3, 1
>>>> alternative_else
>>>>          mov     x3, 0
>>>> alternative_endif
>>>>          cbz     x3, 1f
>>>>          /* Park the mismatched secondary CPU */
>>>>          early_park_cpu CPU_STUCK_REASON_NO_PTRAUTH
>>>> #endif
>>>> 1:     ret
>>>> ENDPROC(__cpu_secondary_checkptrauth)
>>>>
>>>> and then check it during the secondary_startup, similar to what happens for
>>>> 52BIT_VA for example.
>>>>
>>>> In this way "update_early_cpu_boot_status" would update the
>>>> CPU_STUCK_REASON_NO_PTRAUTH flag.
>>>
>>> This way it was implemented earlier. Catalin suggested the pointer auth
>>> variation in cpus is not critical enough and cpufeature framework can park it
>>> little later [1].
>>>
>>> Agree that I should have removed all definitions of CPU_STUCK_REASON_NO_PTRAUTH
>>> and prevent unnecessary confusions.
>>>
>>> [1] : https://www.spinics.net/lists/arm-kernel/msg780766.html
>>
>> It was either or :) Sorry I did not see Catalin's comment, please go ahead and
>> remove the definition of CPU_STUCK_REASON_NO_PTRAUTH and the code that uses it
>> in this case.
> 
> ok
> 
>>
>> Maybe you want to expand as well your commit message (which already seems
>> covering this case) to explain why it is possible to let the cpu framework deal
>> with this case. This should make things clear according to me.
> 
> sure.
> 
>>
>> Another question that still remains is: do we need to introduce early_park_cpu
>> as part of this series? Since it does not seem you are using it anywhere.
> 
> I should probably drop this cleanup patch from this series and may be
> send it separately.
>

Thanks!

With this comments addressed:
Reviewed-by: Vincenzo Frascino <[email-masked]>

>>>> [...]

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel