[PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar at linux.ibm.com> writes:
> max_pkey now represents max key value that userspace can allocate.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar at linux.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h |  7 +++++--
>  arch/powerpc/mm/book3s64/pkeys.c | 14 +++++++-------
>  2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 75d2a2c19c04..652bad7334f3 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -12,7 +12,7 @@
>  #include <asm/firmware.h>
>  
>  DECLARE_STATIC_KEY_FALSE(pkey_disabled);
> -extern int pkeys_total; /* total pkeys as per device tree */
> +extern int max_pkey;
>  extern u32 initial_allocation_mask; /*  bits set for the initially allocated keys */
>  extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>  
> @@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>  	return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
>  }
>  
> -#define arch_max_pkey() pkeys_total
> +static inline int arch_max_pkey(void)
> +{
> +	return max_pkey;
> +}

If pkeys_total = 32 then max_pkey = 31.

So we can't just substitute one for the other. ie. arch_max_pkey() must
have been wrong, or it is wrong now.

> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 87d882a9aaf2..a4d7287082a8 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -14,7 +14,7 @@
>  
>  DEFINE_STATIC_KEY_FALSE(pkey_disabled);
>  DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
> -int  pkeys_total;		/* Total pkeys as per device tree */
> +int  max_pkey;			/* Maximum key value supported */
>  u32  initial_allocation_mask;   /* Bits set for the initially allocated keys */
>  /*
>   *  Keys marked in the reservation list cannot be allocated by  userspace
> @@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
>  
>  static int pkey_initialize(void)
>  {
> -	int os_reserved, i;
> +	int pkeys_total, i;
>  
>  	/*
>  	 * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> @@ -122,12 +122,12 @@ static int pkey_initialize(void)
>  	 * The OS can manage only 8 pkeys due to its inability to represent them
>  	 * in the Linux 4K PTE. Mark all other keys reserved.
>  	 */
> -	os_reserved = pkeys_total - 8;
> +	max_pkey = min(8, pkeys_total);

Shouldn't it be 7 ?

>  #else
> -	os_reserved = 0;
> +	max_pkey = pkeys_total;
>  #endif
>  
> -	if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
> +	if (unlikely(max_pkey <= execute_only_key)) {

Isn't that an off-by-one now?

This is one-off boot time code, there's no need to clutter it with
unlikely.

>  		/*
>  		 * Insufficient number of keys to support
>  		 * execute only key. Mark it unavailable.
> @@ -174,10 +174,10 @@ static int pkey_initialize(void)
>  	default_uamor &= ~(0x3ul << pkeyshift(1));
>  
>  	/*
> -	 * Prevent the usage of OS reserved the keys. Update UAMOR
> +	 * Prevent the usage of OS reserved keys. Update UAMOR
>  	 * for those keys.
>  	 */
> -	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
> +	for (i = max_pkey; i < pkeys_total; i++) {

Another off-by-one? Shouldn't we start from max_pkey + 1 ?

>  		reserved_allocation_mask |= (0x1 << i);
>  		default_uamor &= ~(0x3ul << pkeyshift(i));
>  	}

cheers