[PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar at linux.ibm.com> writes:
> Use execute_pkey_disabled static key to check for execute key support instead
> of pkey_disabled.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar at linux.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h | 10 +---------
>  arch/powerpc/mm/book3s64/pkeys.c |  5 ++++-
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 47c81d41ea9a..09fbaa409ac4 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>   * Try to dedicate one of the protection keys to be used as an
>   * execute-only protection key.
>   */
> -extern int __execute_only_pkey(struct mm_struct *mm);
> -static inline int execute_only_pkey(struct mm_struct *mm)
> -{
> -	if (static_branch_likely(&pkey_disabled))
> -		return -1;
> -
> -	return __execute_only_pkey(mm);
> -}
> -
> +extern int execute_only_pkey(struct mm_struct *mm);
>  extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
>  					 int prot, int pkey);
>  static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index bbba9c601e14..fed4f159011b 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>  	write_uamor(default_uamor);
>  }
>  
> -int __execute_only_pkey(struct mm_struct *mm)
> +int execute_only_pkey(struct mm_struct *mm)
>  {
> +	if (static_branch_likely(&execute_pkey_disabled))
> +		return -1;
> +
>  	return mm->context.execute_only_pkey;
>  }

That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.

Anyway this seems unnecessary:

pkey_early_init_devtree()
{
	...
	if (unlikely(max_pkey <= execute_only_key)) {
		/*
		 * Insufficient number of keys to support
		 * execute only key. Mark it unavailable.
		 */
		execute_only_key = -1;

void pkey_mm_init(struct mm_struct *mm)
{
	...
	mm->context.execute_only_pkey = execute_only_key;
}


ie. Can't it just be:

static inline int execute_only_pkey(struct mm_struct *mm)
{
	return mm->context.execute_only_pkey;
}

cheers