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

[Balbir Singh <bsingharora@gmail.com>]

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

> 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

Balbir Singh.