On Thu, Aug 17, 2017 at 05:30:27PM -0300, Thiago Jung Bauermann wrote: > > Ram Pai <linux...@us.ibm.com> writes: > > > 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. > > He said that P7 was the first processor to support 32 keys, but if you > look at the Virtual Page Class Key Protection section in ISA 2.06, > there's no IAMR. > > There was a bug in the code where init_iamr was calling write_amr > instead of write_iamr, perhaps that's why it worked when you tested 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. > > Indeed. There should just be a special case to disable execute > protection for P7.
Ok. we should disable execute protection for P7 and earlier generations of CPU. RP.