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

From: Vincenzo Frascino <vincenzo.frascino@arm.com>
To: Amit Kachhap <amit.kachhap@arm.com>,
	linux-arm-kernel@lists.infradead.org
Cc: Mark Rutland <mark.rutland@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Kristina Martsenko <kristina.martsenko@arm.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Mark Brown <broonie@kernel.org>,
	James Morse <james.morse@arm.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Will Deacon <will@kernel.org>, Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH v6 09/18] arm64: enable ptrauth earlier
Date: Wed, 11 Mar 2020 10:49:25 +0000	[thread overview]
Message-ID: <cb5b7eae-49e5-cff7-7058-8fc31ac478ca@arm.com> (raw)
In-Reply-To: <436a6d6a-d5c3-14db-0a5f-afdb62125eff@arm.com>

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 <Vincenzo.Frascino@arm.com>

>>
>>>>
>>>> [...]
>>>>
>>

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel