Nicholas Piggin <npig...@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
