On Thu, Aug 10, 2017 at 06:27:34PM -0300, Thiago Jung Bauermann wrote: > > Ram Pai <linux...@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.
did'nt David say P7 supports both? P6, i think, only support data. my pkey tests have passed on p7. > > > #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. Good point. we should just ignore total_execute. It should be the same value as total_data on the latest platforms. On older platforms it will continue to be zero. > > 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.". Yes. RP