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

[Ram Pai <linuxram@us.ibm.com>]

On Thu, Aug 17, 2017 at 05:30:27PM -0300, Thiago Jung Bauermann wrote:
> 
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Thu, Aug 10, 2017 at 06:27:34PM -0300, Thiago Jung Bauermann wrote:
> >> 
> >> 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.
> >
> > 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.