Re: [RFC v7 25/25] powerpc: Enable pkey subsystem

[Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>]

Ram Pai <linuxram@us.ibm.com> writes:
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -214,6 +214,7 @@ enum {
>  #define CPU_FTR_DAWR			LONG_ASM_CONST(0x0400000000000000)
>  #define CPU_FTR_DABRX			LONG_ASM_CONST(0x0800000000000000)
>  #define CPU_FTR_PMAO_BUG		LONG_ASM_CONST(0x1000000000000000)
> +#define CPU_FTR_PKEY			LONG_ASM_CONST(0x2000000000000000)
>  #define CPU_FTR_POWER9_DD1		LONG_ASM_CONST(0x4000000000000000)
>
>  #ifndef __ASSEMBLY__
> @@ -452,7 +453,7 @@ enum {
>  	    CPU_FTR_DSCR | CPU_FTR_SAO  | CPU_FTR_ASYM_SMT | \
>  	    CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
>  	    CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
> -	    CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX)
> +	    CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX | CPU_FTR_PKEY)

P7 supports protection keys for data access (AMR) but not for
instruction access (IAMR), right? There's nothing in the code making
this distinction, so either CPU_FTR_PKEY shouldn't be enabled in P7 or
separate feature bits for AMR and IAMR should be used and checked before
trying to access the IAMR.

>  #define CPU_FTRS_POWER8 (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
>  	    CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | CPU_FTR_ARCH_206 |\
>  	    CPU_FTR_MMCRA | CPU_FTR_SMT | \
> @@ -462,7 +463,7 @@ enum {
>  	    CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
>  	    CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
>  	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
> -	    CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP)
> +	    CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP | CPU_FTR_PKEY)
>  #define CPU_FTRS_POWER8E (CPU_FTRS_POWER8 | CPU_FTR_PMAO_BUG)
>  #define CPU_FTRS_POWER8_DD1 (CPU_FTRS_POWER8 & ~CPU_FTR_DBELL)
>  #define CPU_FTRS_POWER9 (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
> @@ -474,7 +475,8 @@ enum {
>  	    CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
>  	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
>  	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
> -	    CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP | CPU_FTR_ARCH_300)
> +	    CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | \
> +	    CPU_FTR_PKEY)
>  #define CPU_FTRS_POWER9_DD1 ((CPU_FTRS_POWER9 | CPU_FTR_POWER9_DD1) & \
>  			     (~CPU_FTR_SAO))
>  #define CPU_FTRS_CELL	(CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index a1cfcca..acd59d8 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -188,6 +188,7 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
>
>  #define pkey_initialize()
>  #define pkey_mm_init(mm)
> +#define pkey_mmu_values(total_data, total_execute)
>
>  static inline int vma_pkey(struct vm_area_struct *vma)
>  {
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index ba7bff6..e61ed6c 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -1,6 +1,8 @@
>  #ifndef _ASM_PPC64_PKEYS_H
>  #define _ASM_PPC64_PKEYS_H
>
> +#include <asm/firmware.h>
> +
>  extern bool pkey_inited;
>  extern int pkeys_total; /* total pkeys as per device tree */
>  extern u32 initial_allocation_mask;/* bits set for reserved keys */
> @@ -227,6 +229,24 @@ static inline void pkey_mm_init(struct mm_struct *mm)
>  	mm->context.execute_only_pkey = -1;
>  }
>
> +static inline void pkey_mmu_values(int total_data, int total_execute)
> +{
> +	/*
> +	 * since any pkey can be used for data or execute, we
> +	 * will  just  treat all keys as equal and track them
> +	 * as one entity.
> +	 */
> +	pkeys_total = total_data + total_execute;
> +}

Right now this works because the firmware reports 0 execute keys in the
device tree, but if (when?) it is fixed to report 32 execute keys as
well as 32 data keys (which are the same keys), any place using
pkeys_total expecting it to mean the number of keys that are available
will be broken. This includes pkey_initialize and mm_pkey_is_allocated.

Perhaps pkeys_total should use total_data as the number of keys
supported in the system, and total_execute just as a flag to say whether
there's a IAMR? Or, since P8 and later have IAMR and P7 is unlikely to
have the firmware fixed, maybe the kernel should just ignore
total_execute altogether?

> +static inline bool pkey_mmu_enabled(void)
> +{
> +	if (firmware_has_feature(FW_FEATURE_LPAR))
> +		return pkeys_total;
> +	else
> +		return cpu_has_feature(CPU_FTR_PKEY);
> +}
> +
>  static inline void pkey_initialize(void)
>  {
>  	int os_reserved, i;
> @@ -236,9 +256,17 @@ static inline void pkey_initialize(void)
>  	 * line will enable it.
>  	 */
>  	pkey_inited = false;
> +	if (pkey_mmu_enabled())
> +		pkey_inited = !radix_enabled();
> +
> +	if (!pkey_inited)
> +		return;
>
> -	/* Lets assume 32 keys */
> -	pkeys_total = 32;
> +	/* Lets assume 32 keys if we are not told
> +	 * the number of pkeys.
> +	 */
> +	if (!pkeys_total)
> +		pkeys_total = 32;
>
>  #ifdef CONFIG_PPC_4K_PAGES
>  	/*

This patch should remove the comment "disable the pkey system till
everything is in place. A patch further down the line will enable it.".

-- 
Thiago Jung Bauermann
IBM Linux Technology Center