Re: [PATCH 13/25] powerpc: implementation for arch_override_mprotect_pkey()

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

On Thu, Oct 19, 2017 at 10:04:40AM +1100, Balbir Singh wrote:
> On Wed, 18 Oct 2017 14:10:41 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > On Wed, Oct 18, 2017 at 03:36:35PM +1100, Balbir Singh wrote:
> > > On Fri,  8 Sep 2017 15:45:01 -0700
> > > Ram Pai <linuxram@us.ibm.com> wrote:
> > >   
> > > > arch independent code calls arch_override_mprotect_pkey()
> > > > to return a pkey that best matches the requested protection.
> > > > 
> > > > This patch provides the implementation.
> > > > 
> > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > > > ---
> > > >  arch/powerpc/include/asm/mmu_context.h |    5 +++
> > > >  arch/powerpc/include/asm/pkeys.h       |   17 ++++++++++-
> > > >  arch/powerpc/mm/pkeys.c                |   47 ++++++++++++++++++++++++++++++++
> > > >  3 files changed, 67 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> > > > index c705a5d..8e5a87e 100644
> > > > --- a/arch/powerpc/include/asm/mmu_context.h
> > > > +++ b/arch/powerpc/include/asm/mmu_context.h
> > > > @@ -145,6 +145,11 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
> > > >  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > > >  #define pkey_initialize()
> > > >  #define pkey_mm_init(mm)
> > > > +
> > > > +static inline int vma_pkey(struct vm_area_struct *vma)
> > > > +{
> > > > +	return 0;
> > > > +}
> > > >  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> > > >  
> > > >  #endif /* __KERNEL__ */
> > > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> > > > index f13e913..d2fffef 100644
> > > > --- a/arch/powerpc/include/asm/pkeys.h
> > > > +++ b/arch/powerpc/include/asm/pkeys.h
> > > > @@ -41,6 +41,16 @@ static inline u64 pkey_to_vmflag_bits(u16 pkey)
> > > >  		((pkey & 0x10UL) ? VM_PKEY_BIT4 : 0x0UL));
> > > >  }
> > > >  
> > > > +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> > > > +				VM_PKEY_BIT3 | VM_PKEY_BIT4)
> > > > +
> > > > +static inline int vma_pkey(struct vm_area_struct *vma)
> > > > +{
> > > > +	if (!pkey_inited)
> > > > +		return 0;  
> > > 
> > > We don't want pkey_inited to be present in all functions, why do we need
> > > a conditional branch for all functions. Even if we do, it should be a jump
> > > label. I would rather we just removed !pkey_inited unless really really
> > > required.  
> > 
> > No. we really really need it.  For example when we build a kernel with
> > PROTECTION_KEYS config enabled and run that kernel on a older processor
> > or on a system where the key feature is not enabled in the device tree,
> > we have fail all the calls that get called-in by the arch-neutral code.
> > 
> > Hence we need this check.
> > 
> 
> Use a mmu_feature then, it's already designed and optimized for that
> purpose

We rely on a combination of cpu_feature and firmware_feature. But that
is still not sufficient. It is also gated on !radix_enabled().

In other words,  the pkey system is enabled if
a) if we are in a lpar and device tree has the feature enabled, and
	there are more than zero keys enabled and radix is not enabled.

	OR

b) if we in baremetal, and the CPU is power5 or later and radix is not
	enabled.

All these criteria determine the value of 'pkey_inited'.

> 
> > BTW: jump labels are awkward IMHO, unless absolutely needed.
> >
> 
> The if checks across the place will hurt performance and we want to have
> this enabled by default, we may need a mmu feature or jump labels

I like the idea of jump_label now that I googled for it.

Thanks,
RP

> 
> Balbir Singh.

-- 
Ram Pai