Re: [PATCH 05/25] powerpc: helper functions to initialize AMR, IAMR and UAMOR registers

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Ram Pai <linuxram@us.ibm.com> writes:

> Introduce  helper functions that can initialize the bits in the AMR,
> IAMR and UAMOR register; the bits that correspond to the given pkey.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h |    1 +
>  arch/powerpc/mm/pkeys.c          |   46 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 133f8c4..5a83ed7 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -26,6 +26,7 @@
>  #define arch_max_pkey()  pkeys_total
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>  				VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +#define AMR_BITS_PER_PKEY 2
>
>  #define pkey_alloc_mask(pkey) (0x1 << pkey)
>
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index ebc9e84..178aa33 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -59,3 +59,49 @@ void __init pkey_initialize(void)
>  	for (i = 2; i < (pkeys_total - os_reserved); i++)
>  		initial_allocation_mask &= ~(0x1<<i);
>  }
> +
> +#define PKEY_REG_BITS (sizeof(u64)*8)
> +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> +
> +static inline void init_amr(int pkey, u8 init_bits)
> +{
> +	u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> +	u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +
> +	write_amr(old_amr | new_amr_bits);
> +}
> +
> +static inline void init_iamr(int pkey, u8 init_bits)
> +{
> +	u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> +	u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +
> +	write_iamr(old_iamr | new_iamr_bits);
> +}
> +
> +static void pkey_status_change(int pkey, bool enable)
> +{
> +	u64 old_uamor;
> +
> +	/* reset the AMR and IAMR bits for this key */
> +	init_amr(pkey, 0x0);
> +	init_iamr(pkey, 0x0);
> +
> +	/* enable/disable key */
> +	old_uamor = read_uamor();
> +	if (enable)
> +		old_uamor |= (0x3ul << pkeyshift(pkey));
> +	else
> +		old_uamor &= ~(0x3ul << pkeyshift(pkey));
> +	write_uamor(old_uamor);
> +}

That one is confusing, we discussed this outside the list, but want to
bring the list to further discussion. So now the core kernel request
for a key via mm_pkey_alloc(). Why not do the below there

static inline int mm_pkey_alloc(struct mm_struct *mm)
{
	/*
	 * Note: this is the one and only place we make sure
	 * that the pkey is valid as far as the hardware is
	 * concerned.  The rest of the kernel trusts that
	 * only good, valid pkeys come out of here.
	 */
	u32 all_pkeys_mask = (u32)(~(0x0));
	int ret;

	if (!pkey_inited)
		return -1;
	/*
	 * Are we out of pkeys?  We must handle this specially
	 * because ffz() behavior is undefined if there are no
	 * zeros.
	 */
	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
		return -1;

	ret = ffz((u32)mm_pkey_allocation_map(mm));
	mm_set_pkey_allocated(mm, ret);

	return ret;
}

your mm_pkey_allocation_map() should have the keys specified in AMOR and
UAMOR marked as allocatied. It is in use by hypervisor and OS respectively.


There is no need of __arch_activate_key() etc. and by default if the OS
has not requested for a key for its internal use UAMOR should be
0xFFFFFFFF and that AMOR value you derive from the device tree based of
what keys hypervisor has reserved.

-aneesh