| Message ID | 1501459946-11619-26-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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.".
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. > > > #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
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. >> 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?
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.
Ram Pai <linuxram@us.ibm.com> writes: > 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: >> >> > @@ -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. You should do what the device tree says you can do. If it says there are no execute keys then you shouldn't touch the IAMR. If you don't want to handle the case where there are 0 execute keys but some data keys then you should do: total_keys = min(data_keys, exec_keys); cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Ram Pai <linuxram@us.ibm.com> writes: >> 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: >>> >> > @@ -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. > > You should do what the device tree says you can do. > > If it says there are no execute keys then you shouldn't touch the IAMR. The downside of that approach is that the device tree in P8 LPARs currently says there are no execute keys even though there are. We'd have to require customers to upgrade their firmware to a fixed version if they want to use execute keys.
On Fri, Aug 18, 2017 at 12:26:33PM -0300, Thiago Jung Bauermann wrote: > > Michael Ellerman <mpe@ellerman.id.au> writes: > > > Ram Pai <linuxram@us.ibm.com> writes: > >> 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: > >>> >> > @@ -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. > > > > You should do what the device tree says you can do. > > > > If it says there are no execute keys then you shouldn't touch the IAMR. > > The downside of that approach is that the device tree in P8 LPARs > currently says there are no execute keys even though there are. We'd > have to require customers to upgrade their firmware to a fixed version > if they want to use execute keys. Correct. the device tree for this property currently does not correctly capture the number of execute keys. On skiboot based systems, there is not device tree property to refer to aswell. Thiago has a patch to fix it, but existing systems without the skiboot fix, will not expose that property. So unfortunately we will have to rely on multiple peices of information to enable the pkey system in the kernel. RP > > -- > Thiago Jung Bauermann > IBM Linux Technology Center
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h index d02ad93..1d9ed84 100644 --- 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) #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; +} + +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 /* diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index f830562..f61da26 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -35,6 +35,7 @@ #include <linux/of_fdt.h> #include <linux/libfdt.h> #include <linux/cpu.h> +#include <linux/pkeys.h> #include <asm/prom.h> #include <asm/rtas.h> @@ -228,6 +229,23 @@ static void __init check_cpu_pa_features(unsigned long node) ibm_pa_features, ARRAY_SIZE(ibm_pa_features)); } +static void __init check_cpu_pkey_feature(unsigned long node) +{ + const __be32 *ftrs; + int len, total_data, total_execute; + + ftrs = of_get_flat_dt_prop(node, + "ibm,processor-storage-keys", &len); + if (ftrs == NULL) + return; + + len /= sizeof(int); + total_execute = (len >= 2) ? be32_to_cpu(ftrs[1]) : 0; + total_data = (len >= 1) ? be32_to_cpu(ftrs[0]) : 0; + pkey_mmu_values(total_data, total_execute); +} + + #ifdef CONFIG_PPC_STD_MMU_64 static void __init init_mmu_slb_size(unsigned long node) { @@ -391,6 +409,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node, check_cpu_feature_properties(node); check_cpu_pa_features(node); + check_cpu_pkey_feature(node); } identical_pvr_fixup(node);
PAPR defines 'ibm,processor-storage-keys' property. It exports two values.The first value indicates the number of data-access keys and the second indicates the number of instruction-access keys. Though this alludes that keys can be either data access or instructon access only, that is not the case in reality.Any key can be of any kind. This patch adds all the keys and uses that as the total number of keys available to us. Non PAPR platforms do not define this property in the device tree yet. Here, we hardcode CPUs that support pkey by consulting PowerISA3.0 Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/powerpc/include/asm/cputable.h | 8 +++++--- arch/powerpc/include/asm/mmu_context.h | 1 + arch/powerpc/include/asm/pkeys.h | 32 ++++++++++++++++++++++++++++++-- arch/powerpc/kernel/prom.c | 19 +++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-)