Re: [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY

[Michael Ellerman <mpe@ellerman.id.au>]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Parse storage keys related device tree entry in early_init_devtree
> and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
>
> MMU feature is used instead of CPU feature because this enables us
> to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.

Subject should probably be "Add MMU_FTR_PKEY".

> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index f4ac25d4df05..72966d3d8f64 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -23,6 +23,7 @@
>  
>  /* Radix page table supported and enabled */
>  #define MMU_FTR_TYPE_RADIX		ASM_CONST(0x00000040)
> +#define MMU_FTR_PKEY			ASM_CONST(0x00000080)

It's not a type, so it should be with the individual feature bits below:

>  /*
>   * Individual features below.

> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 6a3bac357e24..6d70797352d8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)
>  	/* Now try to figure out if we are running on LPAR and so on */
>  	pseries_probe_fw_features();
>  
> +	/*
> +	 * Initialize pkey features and default AMR/IAMR values
> +	 */
> +	pkey_early_init_devtree();

Not your fault, but the fact that we're having to do more and more
initialisation this early, based on the flat device tree, makes me
wonder if we need to rethink how we're doing the CPU/MMU feature setup.

> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 0ff59acdbb84..bbba9c601e14 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -38,38 +39,45 @@ static int execute_only_key = 2;
>  #define PKEY_REG_BITS (sizeof(u64) * 8)
>  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
>  
> +static int __init dt_scan_storage_keys(unsigned long node,
> +				       const char *uname, int depth,
> +				       void *data)
> +{
> +	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> +	const __be32 *prop;
> +	int pkeys_total;
> +
> +	/* We are scanning "cpu" nodes only */
> +	if (type == NULL || strcmp(type, "cpu") != 0)
> +		return 0;
> +
> +	prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
> +	if (!prop)
> +		return 0;
> +	pkeys_total = be32_to_cpu(prop[0]);
> +	return pkeys_total;

That's not really the way the return value is meant to be used for these
scanning functions.

The usual return values are 0 to keep scanning and !0 means we've found
the node we're looking for and we should stop scanning.

Doing it this way means if you find 0 pkeys it will keep scanning.

Instead you should pass &pkeys_total as the data pointer and set that.

> +}
> +
>  static int scan_pkey_feature(void)
>  {
> -	u32 vals[2];
> -	int pkeys_total = 0;
> -	struct device_node *cpu;
> +	int pkeys_total;
>  
>  	/*
>  	 * Pkey is not supported with Radix translation.
>  	 */
> -	if (radix_enabled())
> +	if (early_radix_enabled())
>  		return 0;
>  
> -	cpu = of_find_node_by_type(NULL, "cpu");
> -	if (!cpu)
> -		return 0;
> +	pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
> +	if (pkeys_total == 0) {
>  
> -	if (of_property_read_u32_array(cpu,
> -				       "ibm,processor-storage-keys", vals, 2) == 0) {
> -		/*
> -		 * Since any pkey can be used for data or execute, we will
> -		 * just treat all keys as equal and track them as one entity.
> -		 */
> -		pkeys_total = vals[0];
> -		/*  Should we check for IAMR support FIXME!! */

???

> -	} else {

This loses the ability to differentiate between no storage-keys property
at all vs a property that specifies zero keys, which I don't think is a
good change.

>  		/*
>  		 * Let's assume 32 pkeys on P8 bare metal, if its not defined by device
>  		 * tree. We make this exception since skiboot forgot to expose this
>  		 * property on power8.
>  		 */
>  		if (!firmware_has_feature(FW_FEATURE_LPAR) &&
> -		    cpu_has_feature(CPU_FTRS_POWER8))
> +		    early_cpu_has_feature(CPU_FTRS_POWER8))
>  			pkeys_total = 32;

That's not how cpu_has_feature() works, we'll need to fix that.

cheers