Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support

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

Nicholas Piggin <npiggin@gmail.com> writes:
> ISA v3.1 does not support the SAO storage control attribute required to
> implement PROT_SAO. PROT_SAO was used by specialised system software
> (Lx86) that has been discontinued for about 7 years, and is not thought
> to be used elsewhere, so removal should not cause problems.
>
> We rather remove it than keep support for older processors, because
> live migrating guest partitions to newer processors may not be possible
> if SAO is in use.

They key details being:
 - you don't remove PROT_SAO from the uapi header, so code using the
   definition will still build.
 - you change arch_validate_prot() to reject PROT_SAO, which means code
   using it will see a failure from mmap() at runtime.


This obviously risks breaking userspace, even if we think it won't in
practice. I guess we don't really have any option given the hardware
support is being dropped.

Can you repost with a wider Cc list, including linux-mm and linux-arch?

I wonder if we should add a comment to the uapi header, eg?

diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index c0c737215b00..d4fdbe768997 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -11,7 +11,7 @@
 #include <asm-generic/mman-common.h>
 
 
-#define PROT_SAO	0x10		/* Strong Access Ordering */
+#define PROT_SAO	0x10		/* Unsupported since v5.9 */
 
 #define MAP_RENAME      MAP_ANONYMOUS   /* In SunOS terminology */
 #define MAP_NORESERVE   0x40            /* don't reserve swap pages */


> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index f17442c3a092..d9e92586f8dc 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -20,9 +20,13 @@
>  #define _PAGE_RW		(_PAGE_READ | _PAGE_WRITE)
>  #define _PAGE_RWX		(_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
>  #define _PAGE_PRIVILEGED	0x00008 /* kernel access only */
> -#define _PAGE_SAO		0x00010 /* Strong access order */
> +
> +#define _PAGE_CACHE_CTL		0x00030 /* Bits for the folowing cache modes */
> +			/*	No bits set is normal cacheable memory */
> +			/*	0x00010 unused, is SAO bit on radix POWER9 */
>  #define _PAGE_NON_IDEMPOTENT	0x00020 /* non idempotent memory */
>  #define _PAGE_TOLERANT		0x00030 /* tolerant memory, cache inhibited */
> +

Why'd you do it that way vs just dropping _PAGE_SAO from the or below?

> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index bac2252c839e..c7e923b0000a 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -191,7 +191,6 @@ static inline void cpu_feature_keys_init(void) { }
>  #define CPU_FTR_SPURR			LONG_ASM_CONST(0x0000000001000000)
>  #define CPU_FTR_DSCR			LONG_ASM_CONST(0x0000000002000000)
>  #define CPU_FTR_VSX			LONG_ASM_CONST(0x0000000004000000)
> -#define CPU_FTR_SAO			LONG_ASM_CONST(0x0000000008000000)

Can you do:

+// Free				LONG_ASM_CONST(0x0000000008000000)

> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 9bb9bb370b53..579c9229124b 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -400,7 +400,8 @@ static inline bool hpte_cache_flags_ok(unsigned long hptel, bool is_ci)
>  
>  	/* Handle SAO */
>  	if (wimg == (HPTE_R_W | HPTE_R_I | HPTE_R_M) &&
> -	    cpu_has_feature(CPU_FTR_ARCH_206))
> +	    cpu_has_feature(CPU_FTR_ARCH_206) &&
> +	    !cpu_has_feature(CPU_FTR_ARCH_31))
>  		wimg = HPTE_R_M;

Shouldn't it reject that combination if the host can't support it?

Or I guess it does, but yikes that code is not clear.

> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index d610c2e07b28..43a62f3e21a0 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -13,38 +13,24 @@
>  #include <linux/pkeys.h>
>  #include <asm/cpu_has_feature.h>
>  
> -/*
> - * This file is included by linux/mman.h, so we can't use cacl_vm_prot_bits()
> - * here.  How important is the optimization?
> - */

This comment seems confused, but also unrelated to this patch?

> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 3a409517c031..8d2e4043702f 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -622,7 +622,7 @@ static struct dt_cpu_feature_match __initdata
>  	{"processor-control-facility-v3", feat_enable_dbell, CPU_FTR_DBELL},
>  	{"processor-utilization-of-resources-register", feat_enable_purr, 0},
>  	{"no-execute", feat_enable, 0},
> -	{"strong-access-ordering", feat_enable, CPU_FTR_SAO},
> +	{"strong-access-ordering", feat_enable, 0},

Would it make more sense to drop it entirely? Or leave it commented out.


cheers