[06/25] powerpc: cleaup AMR, iAMR when a key is allocated or freed

Message ID 1504910713-7094-15-git-send-email-linuxram@us.ibm.com
State Changes Requested
Headers show
Series
  • Untitled series #2311
Related show

Commit Message

Ram Pai Sept. 8, 2017, 10:44 p.m.
cleanup the bits corresponding to a key in the AMR, and IAMR
register, when the key is newly allocated/activated or is freed.
We dont want some residual bits cause the hardware enforce
unintended behavior when the key is activated or freed.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Comments

Balbir Singh Oct. 18, 2017, 3:34 a.m. | #1
On Fri,  8 Sep 2017 15:44:54 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> cleanup the bits corresponding to a key in the AMR, and IAMR
> register, when the key is newly allocated/activated or is freed.
> We dont want some residual bits cause the hardware enforce
> unintended behavior when the key is activated or freed.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 5a83ed7..53bf13b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  		mm_set_pkey_is_allocated(mm, pkey));
>  }
>  
> +extern void __arch_activate_pkey(int pkey);
> +extern void __arch_deactivate_pkey(int pkey);
>  /*
>   * Returns a positive, 5-bit key on success, or -1 on failure.
>   */
> @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>  
>  	ret = ffz((u32)mm_pkey_allocation_map(mm));
>  	mm_set_pkey_allocated(mm, ret);
> +
> +	/*
> +	 * enable the key in the hardware
> +	 */
> +	if (ret > 0)
> +		__arch_activate_pkey(ret);
>  	return ret;
>  }
>  
> @@ -91,6 +99,10 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>  	if (!mm_pkey_is_allocated(mm, pkey))
>  		return -EINVAL;
>  
> +	/*
> +	 * Disable the key in the hardware
> +	 */
> +	__arch_deactivate_pkey(pkey);
>  	mm_set_pkey_free(mm, pkey);
>  
>  	return 0;

I think some of these patches can be merged, too much fine granularity
is hurting my ability to see the larger function/implementation.

Balbir Singh.
Aneesh Kumar K.V Oct. 23, 2017, 9:43 a.m. | #2
Ram Pai <linuxram@us.ibm.com> writes:

> cleanup the bits corresponding to a key in the AMR, and IAMR
> register, when the key is newly allocated/activated or is freed.
> We dont want some residual bits cause the hardware enforce
> unintended behavior when the key is activated or freed.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 5a83ed7..53bf13b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  		mm_set_pkey_is_allocated(mm, pkey));
>  }
>
> +extern void __arch_activate_pkey(int pkey);
> +extern void __arch_deactivate_pkey(int pkey);
>  /*
>   * Returns a positive, 5-bit key on success, or -1 on failure.
>   */
> @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>
>  	ret = ffz((u32)mm_pkey_allocation_map(mm));
>  	mm_set_pkey_allocated(mm, ret);
> +
> +	/*
> +	 * enable the key in the hardware
> +	 */
> +	if (ret > 0)
> +		__arch_activate_pkey(ret);
>  	return ret;
>  }

We are already arch specific because we are defining them in
arch/powerpc/include/asm/, then why __arch_activate_pkey() ? 

>
> @@ -91,6 +99,10 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>  	if (!mm_pkey_is_allocated(mm, pkey))
>  		return -EINVAL;
>
> +	/*
> +	 * Disable the key in the hardware
> +	 */
> +	__arch_deactivate_pkey(pkey);
>  	mm_set_pkey_free(mm, pkey);
>
>  	return 0;
> -- 
> 1.7.1
Aneesh Kumar K.V Oct. 23, 2017, 9:43 a.m. | #3
Balbir Singh <bsingharora@gmail.com> writes:

> On Fri,  8 Sep 2017 15:44:54 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
>
>> cleanup the bits corresponding to a key in the AMR, and IAMR
>> register, when the key is newly allocated/activated or is freed.
>> We dont want some residual bits cause the hardware enforce
>> unintended behavior when the key is activated or freed.
>> 
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> ---
>>  arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> index 5a83ed7..53bf13b 100644
>> --- a/arch/powerpc/include/asm/pkeys.h
>> +++ b/arch/powerpc/include/asm/pkeys.h
>> @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>>  		mm_set_pkey_is_allocated(mm, pkey));
>>  }
>>  
>> +extern void __arch_activate_pkey(int pkey);
>> +extern void __arch_deactivate_pkey(int pkey);
>>  /*
>>   * Returns a positive, 5-bit key on success, or -1 on failure.
>>   */
>> @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
>>  
>>  	ret = ffz((u32)mm_pkey_allocation_map(mm));
>>  	mm_set_pkey_allocated(mm, ret);
>> +
>> +	/*
>> +	 * enable the key in the hardware
>> +	 */
>> +	if (ret > 0)
>> +		__arch_activate_pkey(ret);
>>  	return ret;
>>  }
>>  
>> @@ -91,6 +99,10 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>>  	if (!mm_pkey_is_allocated(mm, pkey))
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * Disable the key in the hardware
>> +	 */
>> +	__arch_deactivate_pkey(pkey);
>>  	mm_set_pkey_free(mm, pkey);
>>  
>>  	return 0;
>
> I think some of these patches can be merged, too much fine granularity
> is hurting my ability to see the larger function/implementation.


Completely agree

-aneesh
Ram Pai Oct. 23, 2017, 6:29 p.m. | #4
On Mon, Oct 23, 2017 at 03:13:33PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > cleanup the bits corresponding to a key in the AMR, and IAMR
> > register, when the key is newly allocated/activated or is freed.
> > We dont want some residual bits cause the hardware enforce
> > unintended behavior when the key is activated or freed.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> > index 5a83ed7..53bf13b 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> >  		mm_set_pkey_is_allocated(mm, pkey));
> >  }
> >
> > +extern void __arch_activate_pkey(int pkey);
> > +extern void __arch_deactivate_pkey(int pkey);
> >  /*
> >   * Returns a positive, 5-bit key on success, or -1 on failure.
> >   */
> > @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
> >
> >  	ret = ffz((u32)mm_pkey_allocation_map(mm));
> >  	mm_set_pkey_allocated(mm, ret);
> > +
> > +	/*
> > +	 * enable the key in the hardware
> > +	 */
> > +	if (ret > 0)
> > +		__arch_activate_pkey(ret);
> >  	return ret;
> >  }
> 
> We are already arch specific because we are defining them in
> arch/powerpc/include/asm/, then why __arch_activate_pkey() ? 

almost all the memory-key internal functions are named with a two
leading underbars.  So just *loosely* following a convention within that
file. The corresponding functions without the two underbars are the one
exposed to the arch-neutral code.

RP
Ram Pai Oct. 23, 2017, 6:36 p.m. | #5
On Mon, Oct 23, 2017 at 03:13:45PM +0530, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
> > On Fri,  8 Sep 2017 15:44:54 -0700
> > Ram Pai <linuxram@us.ibm.com> wrote:
> >
> >> cleanup the bits corresponding to a key in the AMR, and IAMR
> >> register, when the key is newly allocated/activated or is freed.
> >> We dont want some residual bits cause the hardware enforce
> >> unintended behavior when the key is activated or freed.
> >> 
> >> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> ---
> >>  arch/powerpc/include/asm/pkeys.h |   12 ++++++++++++
> >>  1 files changed, 12 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> >> index 5a83ed7..53bf13b 100644
> >> --- a/arch/powerpc/include/asm/pkeys.h
> >> +++ b/arch/powerpc/include/asm/pkeys.h
> >> @@ -54,6 +54,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> >>  		mm_set_pkey_is_allocated(mm, pkey));
> >>  }
> >>  
> >> +extern void __arch_activate_pkey(int pkey);
> >> +extern void __arch_deactivate_pkey(int pkey);
> >>  /*
> >>   * Returns a positive, 5-bit key on success, or -1 on failure.
> >>   */
> >> @@ -80,6 +82,12 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
> >>  
> >>  	ret = ffz((u32)mm_pkey_allocation_map(mm));
> >>  	mm_set_pkey_allocated(mm, ret);
> >> +
> >> +	/*
> >> +	 * enable the key in the hardware
> >> +	 */
> >> +	if (ret > 0)
> >> +		__arch_activate_pkey(ret);
> >>  	return ret;
> >>  }
> >>  
> >> @@ -91,6 +99,10 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> >>  	if (!mm_pkey_is_allocated(mm, pkey))
> >>  		return -EINVAL;
> >>  
> >> +	/*
> >> +	 * Disable the key in the hardware
> >> +	 */
> >> +	__arch_deactivate_pkey(pkey);
> >>  	mm_set_pkey_free(mm, pkey);
> >>  
> >>  	return 0;
> >
> > I think some of these patches can be merged, too much fine granularity
> > is hurting my ability to see the larger function/implementation.
> 
> 
> Completely agree

Me agree too :) Had to fine-grain it to satisfy comments
received during the initial versions.  I had about 10-12 patches in total.

Thanks, will merge a couple of these patches.

RP

Patch

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 5a83ed7..53bf13b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -54,6 +54,8 @@  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 		mm_set_pkey_is_allocated(mm, pkey));
 }
 
+extern void __arch_activate_pkey(int pkey);
+extern void __arch_deactivate_pkey(int pkey);
 /*
  * Returns a positive, 5-bit key on success, or -1 on failure.
  */
@@ -80,6 +82,12 @@  static inline int mm_pkey_alloc(struct mm_struct *mm)
 
 	ret = ffz((u32)mm_pkey_allocation_map(mm));
 	mm_set_pkey_allocated(mm, ret);
+
+	/*
+	 * enable the key in the hardware
+	 */
+	if (ret > 0)
+		__arch_activate_pkey(ret);
 	return ret;
 }
 
@@ -91,6 +99,10 @@  static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 	if (!mm_pkey_is_allocated(mm, pkey))
 		return -EINVAL;
 
+	/*
+	 * Disable the key in the hardware
+	 */
+	__arch_deactivate_pkey(pkey);
 	mm_set_pkey_free(mm, pkey);
 
 	return 0;