* [PATCH 1/2] powerpc/64s: remove PROT_SAO support
@ 2020-06-07 12:02 Nicholas Piggin
2020-06-07 12:02 ` [PATCH 2/2] powerpc/64s/hash: disable subpage_prot syscall by default Nicholas Piggin
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-06-07 12:02 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
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.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 8 ++--
arch/powerpc/include/asm/cputable.h | 9 ++--
arch/powerpc/include/asm/kvm_book3s_64.h | 3 +-
arch/powerpc/include/asm/mman.h | 24 +++--------
arch/powerpc/include/asm/nohash/64/pgtable.h | 2 -
arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
arch/powerpc/mm/book3s64/hash_utils.c | 2 -
include/linux/mm.h | 2 -
include/trace/events/mmflags.h | 2 -
mm/ksm.c | 4 --
tools/testing/selftests/powerpc/mm/.gitignore | 1 -
tools/testing/selftests/powerpc/mm/Makefile | 4 +-
tools/testing/selftests/powerpc/mm/prot_sao.c | 42 -------------------
13 files changed, 18 insertions(+), 87 deletions(-)
delete mode 100644 tools/testing/selftests/powerpc/mm/prot_sao.c
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 */
+
#define _PAGE_DIRTY 0x00080 /* C: page changed */
#define _PAGE_ACCESSED 0x00100 /* R: page referenced */
/*
@@ -825,8 +829,6 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
return hash__set_pte_at(mm, addr, ptep, pte, percpu);
}
-#define _PAGE_CACHE_CTL (_PAGE_SAO | _PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT)
-
#define pgprot_noncached pgprot_noncached
static inline pgprot_t pgprot_noncached(pgprot_t prot)
{
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)
#define CPU_FTR_CP_USE_DCBTZ LONG_ASM_CONST(0x0000000010000000)
#define CPU_FTR_UNALIGNED_LD_STD LONG_ASM_CONST(0x0000000020000000)
#define CPU_FTR_ASYM_SMT LONG_ASM_CONST(0x0000000040000000)
@@ -435,7 +434,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_MMCRA | CPU_FTR_SMT | \
CPU_FTR_COHERENT_ICACHE | \
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
- CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
+ CPU_FTR_DSCR | CPU_FTR_ASYM_SMT | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | \
CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX | CPU_FTR_PKEY)
@@ -444,7 +443,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_MMCRA | CPU_FTR_SMT | \
CPU_FTR_COHERENT_ICACHE | \
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
- CPU_FTR_DSCR | CPU_FTR_SAO | \
+ CPU_FTR_DSCR | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
@@ -455,7 +454,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_MMCRA | CPU_FTR_SMT | \
CPU_FTR_COHERENT_ICACHE | \
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
- CPU_FTR_DSCR | CPU_FTR_SAO | \
+ CPU_FTR_DSCR | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
@@ -473,7 +472,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_MMCRA | CPU_FTR_SMT | \
CPU_FTR_COHERENT_ICACHE | \
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
- CPU_FTR_DSCR | CPU_FTR_SAO | \
+ CPU_FTR_DSCR | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
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;
if (!is_ci)
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?
- */
+#ifdef CONFIG_PPC_MEM_KEYS
static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
unsigned long pkey)
{
-#ifdef CONFIG_PPC_MEM_KEYS
- return (((prot & PROT_SAO) ? VM_SAO : 0) | pkey_to_vmflag_bits(pkey));
-#else
- return ((prot & PROT_SAO) ? VM_SAO : 0);
-#endif
+ return pkey_to_vmflag_bits(pkey);
}
#define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
{
-#ifdef CONFIG_PPC_MEM_KEYS
- return (vm_flags & VM_SAO) ?
- __pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
- __pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
-#else
- return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
-#endif
+ return __pgprot(vmflag_to_pte_pkey_bits(vm_flags));
}
#define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
+#endif
static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
{
- if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
- return false;
- if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
+ if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM))
return false;
return true;
}
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 3424381b81da..2fd528ef48e0 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -82,8 +82,6 @@
*/
#include <asm/nohash/pte-book3e.h>
-#define _PAGE_SAO 0
-
#define PTE_RPN_MASK (~((1UL << PTE_RPN_SHIFT) - 1))
/*
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},
{"cache-inhibited-large-page", feat_enable_large_ci, 0},
{"coprocessor-icswx", feat_enable, 0},
{"hypervisor-virtualization-interrupt", feat_enable_hvi, 0},
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 0124003e60d0..14b6abdc3bd8 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -232,8 +232,6 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)
rflags |= HPTE_R_I;
else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT)
rflags |= (HPTE_R_I | HPTE_R_G);
- else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO)
- rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M);
else
/*
* Add memory coherence if cache inhibited is not set
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 86adc71a972f..bdcaae914120 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -316,8 +316,6 @@ extern unsigned int kobjsize(const void *objp);
#if defined(CONFIG_X86)
# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
-#elif defined(CONFIG_PPC)
-# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
#elif defined(CONFIG_PARISC)
# define VM_GROWSUP VM_ARCH_1
#elif defined(CONFIG_IA64)
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 5fb752034386..939092dbcb8b 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -114,8 +114,6 @@ IF_HAVE_PG_IDLE(PG_idle, "idle" )
#if defined(CONFIG_X86)
#define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" }
-#elif defined(CONFIG_PPC)
-#define __VM_ARCH_SPECIFIC_1 {VM_SAO, "sao" }
#elif defined(CONFIG_PARISC) || defined(CONFIG_IA64)
#define __VM_ARCH_SPECIFIC_1 {VM_GROWSUP, "growsup" }
#elif !defined(CONFIG_MMU)
diff --git a/mm/ksm.c b/mm/ksm.c
index 18c5d005bd01..b225b0e16111 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2452,10 +2452,6 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
if (vma_is_dax(vma))
return 0;
-#ifdef VM_SAO
- if (*vm_flags & VM_SAO)
- return 0;
-#endif
#ifdef VM_SPARC_ADI
if (*vm_flags & VM_SPARC_ADI)
return 0;
diff --git a/tools/testing/selftests/powerpc/mm/.gitignore b/tools/testing/selftests/powerpc/mm/.gitignore
index 2ca523255b1b..ff296c94f627 100644
--- a/tools/testing/selftests/powerpc/mm/.gitignore
+++ b/tools/testing/selftests/powerpc/mm/.gitignore
@@ -2,7 +2,6 @@
hugetlb_vs_thp_test
subpage_prot
tempfile
-prot_sao
segv_errors
wild_bctr
large_vm_fork_separation
diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
index b9103c4bb414..9b8a7b3069c5 100644
--- a/tools/testing/selftests/powerpc/mm/Makefile
+++ b/tools/testing/selftests/powerpc/mm/Makefile
@@ -2,7 +2,7 @@
noarg:
$(MAKE) -C ../
-TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
+TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot segv_errors wild_bctr \
large_vm_fork_separation bad_accesses
TEST_GEN_PROGS_EXTENDED := tlbie_test
TEST_GEN_FILES := tempfile
@@ -12,8 +12,6 @@ include ../../lib.mk
$(TEST_GEN_PROGS): ../harness.c
-$(OUTPUT)/prot_sao: ../utils.c
-
$(OUTPUT)/wild_bctr: CFLAGS += -m64
$(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
$(OUTPUT)/bad_accesses: CFLAGS += -m64
diff --git a/tools/testing/selftests/powerpc/mm/prot_sao.c b/tools/testing/selftests/powerpc/mm/prot_sao.c
deleted file mode 100644
index e2eed65b7735..000000000000
--- a/tools/testing/selftests/powerpc/mm/prot_sao.c
+++ /dev/null
@@ -1,42 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright 2016, Michael Ellerman, IBM Corp.
- */
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/mman.h>
-
-#include <asm/cputable.h>
-
-#include "utils.h"
-
-#define SIZE (64 * 1024)
-
-int test_prot_sao(void)
-{
- char *p;
-
- /* 2.06 or later should support SAO */
- SKIP_IF(!have_hwcap(PPC_FEATURE_ARCH_2_06));
-
- /*
- * Ensure we can ask for PROT_SAO.
- * We can't really verify that it does the right thing, but at least we
- * confirm the kernel will accept it.
- */
- p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE | PROT_SAO,
- MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
- FAIL_IF(p == MAP_FAILED);
-
- /* Write to the mapping, to at least cause a fault */
- memset(p, 0xaa, SIZE);
-
- return 0;
-}
-
-int main(void)
-{
- return test_harness(test_prot_sao, "prot-sao");
-}
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] powerpc/64s/hash: disable subpage_prot syscall by default
2020-06-07 12:02 [PATCH 1/2] powerpc/64s: remove PROT_SAO support Nicholas Piggin
@ 2020-06-07 12:02 ` Nicholas Piggin
2020-06-12 6:14 ` [PATCH 1/2] powerpc/64s: remove PROT_SAO support Michael Ellerman
2020-08-17 19:14 ` Shawn Anastasio
2 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-06-07 12:02 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
The subpage_prot syscall was added for specialised system software
(Lx86) that has been discontinued for about 7 years, and is not thought
to be used elsewhere, so disable it by default.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/configs/powernv_defconfig | 1 -
arch/powerpc/configs/pseries_defconfig | 1 -
3 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 9fa23eb320ff..1701646f845d 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -834,6 +834,7 @@ config FORCE_MAX_ZONEORDER
config PPC_SUBPAGE_PROT
bool "Support setting protections for 4k subpages"
+ default n
depends on PPC_BOOK3S_64 && PPC_64K_PAGES
help
This option adds support for a system call to allow user programs
diff --git a/arch/powerpc/configs/powernv_defconfig b/arch/powerpc/configs/powernv_defconfig
index 2de9aadf0f50..afc0dd73a1e6 100644
--- a/arch/powerpc/configs/powernv_defconfig
+++ b/arch/powerpc/configs/powernv_defconfig
@@ -64,7 +64,6 @@ CONFIG_HWPOISON_INJECT=m
CONFIG_TRANSPARENT_HUGEPAGE=y
CONFIG_DEFERRED_STRUCT_PAGE_INIT=y
CONFIG_PPC_64K_PAGES=y
-CONFIG_PPC_SUBPAGE_PROT=y
CONFIG_SCHED_SMT=y
CONFIG_PM=y
CONFIG_HOTPLUG_PCI=y
diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
index dfa4a726333b..894e8d85fb48 100644
--- a/arch/powerpc/configs/pseries_defconfig
+++ b/arch/powerpc/configs/pseries_defconfig
@@ -57,7 +57,6 @@ CONFIG_MEMORY_HOTREMOVE=y
CONFIG_KSM=y
CONFIG_TRANSPARENT_HUGEPAGE=y
CONFIG_PPC_64K_PAGES=y
-CONFIG_PPC_SUBPAGE_PROT=y
CONFIG_SCHED_SMT=y
CONFIG_HOTPLUG_PCI=y
CONFIG_HOTPLUG_PCI_RPA=m
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-06-07 12:02 [PATCH 1/2] powerpc/64s: remove PROT_SAO support Nicholas Piggin
2020-06-07 12:02 ` [PATCH 2/2] powerpc/64s/hash: disable subpage_prot syscall by default Nicholas Piggin
@ 2020-06-12 6:14 ` Michael Ellerman
2020-06-29 4:50 ` Nicholas Piggin
2020-08-17 19:14 ` Shawn Anastasio
2 siblings, 1 reply; 8+ messages in thread
From: Michael Ellerman @ 2020-06-12 6:14 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-06-12 6:14 ` [PATCH 1/2] powerpc/64s: remove PROT_SAO support Michael Ellerman
@ 2020-06-29 4:50 ` Nicholas Piggin
0 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-06-29 4:50 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Excerpts from Michael Ellerman's message of June 12, 2020 4:14 pm:
> 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.
>
Thakns for the review, sorry got distracted...
> 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.
Yes.
> 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?
Will do.
> 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 */
Yeah that makes sense.
>> 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?
Just didn't like _PAGE_CACHE_CTL depending on values of the variants
that we use.
>> 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)
Yes.
>
>> 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.
Yeah, took me a bit to work that out.
>> 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?
Yeah.
>> 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.
Probably would, yes.
Thanks,
Nick
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-06-07 12:02 [PATCH 1/2] powerpc/64s: remove PROT_SAO support Nicholas Piggin
2020-06-07 12:02 ` [PATCH 2/2] powerpc/64s/hash: disable subpage_prot syscall by default Nicholas Piggin
2020-06-12 6:14 ` [PATCH 1/2] powerpc/64s: remove PROT_SAO support Michael Ellerman
@ 2020-08-17 19:14 ` Shawn Anastasio
2020-08-18 7:11 ` Nicholas Piggin
2 siblings, 1 reply; 8+ messages in thread
From: Shawn Anastasio @ 2020-08-17 19:14 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
I'm a bit concerned about the removal of PROT_SAO.
From what I can see, a feature like this would be extremely useful for
emulating architectures with stronger memory models. QEMU's multi-
threaded TCG project in particular looks like it would be a good
candidate, since as far as I'm aware it is currently completely
unable to perform strong-on-weak emulation.
Without hardware support like SAO provides, the only way I could see
to achieve this would be by emitting tons of unnecessary and costly
memory barrier instructions.
I understand that ISA 3.1 and POWER10 have dropped SAO, but as a POWER9
user it seems a bit silly to have a potentially useful feature dropped
from the kernel just because a future processor doesn't support it.
Curious to hear more thoughts on this.
Regards,
Shawn
On 6/7/20 7:02 AM, Nicholas Piggin wrote:
> 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.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/book3s/64/pgtable.h | 8 ++--
> arch/powerpc/include/asm/cputable.h | 9 ++--
> arch/powerpc/include/asm/kvm_book3s_64.h | 3 +-
> arch/powerpc/include/asm/mman.h | 24 +++--------
> arch/powerpc/include/asm/nohash/64/pgtable.h | 2 -
> arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
> arch/powerpc/mm/book3s64/hash_utils.c | 2 -
> include/linux/mm.h | 2 -
> include/trace/events/mmflags.h | 2 -
> mm/ksm.c | 4 --
> tools/testing/selftests/powerpc/mm/.gitignore | 1 -
> tools/testing/selftests/powerpc/mm/Makefile | 4 +-
> tools/testing/selftests/powerpc/mm/prot_sao.c | 42 -------------------
> 13 files changed, 18 insertions(+), 87 deletions(-)
> delete mode 100644 tools/testing/selftests/powerpc/mm/prot_sao.c
>
> 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 */
> +
> #define _PAGE_DIRTY 0x00080 /* C: page changed */
> #define _PAGE_ACCESSED 0x00100 /* R: page referenced */
> /*
> @@ -825,8 +829,6 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
> return hash__set_pte_at(mm, addr, ptep, pte, percpu);
> }
>
> -#define _PAGE_CACHE_CTL (_PAGE_SAO | _PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT)
> -
> #define pgprot_noncached pgprot_noncached
> static inline pgprot_t pgprot_noncached(pgprot_t prot)
> {
> 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)
> #define CPU_FTR_CP_USE_DCBTZ LONG_ASM_CONST(0x0000000010000000)
> #define CPU_FTR_UNALIGNED_LD_STD LONG_ASM_CONST(0x0000000020000000)
> #define CPU_FTR_ASYM_SMT LONG_ASM_CONST(0x0000000040000000)
> @@ -435,7 +434,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_MMCRA | CPU_FTR_SMT | \
> CPU_FTR_COHERENT_ICACHE | \
> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> - CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
> + CPU_FTR_DSCR | CPU_FTR_ASYM_SMT | \
> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> CPU_FTR_CFAR | CPU_FTR_HVMODE | \
> CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX | CPU_FTR_PKEY)
> @@ -444,7 +443,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_MMCRA | CPU_FTR_SMT | \
> CPU_FTR_COHERENT_ICACHE | \
> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> - CPU_FTR_DSCR | CPU_FTR_SAO | \
> + CPU_FTR_DSCR | \
> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
> CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
> @@ -455,7 +454,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_MMCRA | CPU_FTR_SMT | \
> CPU_FTR_COHERENT_ICACHE | \
> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> - CPU_FTR_DSCR | CPU_FTR_SAO | \
> + CPU_FTR_DSCR | \
> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
> CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
> @@ -473,7 +472,7 @@ static inline void cpu_feature_keys_init(void) { }
> CPU_FTR_MMCRA | CPU_FTR_SMT | \
> CPU_FTR_COHERENT_ICACHE | \
> CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
> - CPU_FTR_DSCR | CPU_FTR_SAO | \
> + CPU_FTR_DSCR | \
> CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
> CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
> 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;
>
> if (!is_ci)
> 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?
> - */
> +#ifdef CONFIG_PPC_MEM_KEYS
> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> unsigned long pkey)
> {
> -#ifdef CONFIG_PPC_MEM_KEYS
> - return (((prot & PROT_SAO) ? VM_SAO : 0) | pkey_to_vmflag_bits(pkey));
> -#else
> - return ((prot & PROT_SAO) ? VM_SAO : 0);
> -#endif
> + return pkey_to_vmflag_bits(pkey);
> }
> #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey)
>
> static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
> {
> -#ifdef CONFIG_PPC_MEM_KEYS
> - return (vm_flags & VM_SAO) ?
> - __pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
> - __pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
> -#else
> - return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
> -#endif
> + return __pgprot(vmflag_to_pte_pkey_bits(vm_flags));
> }
> #define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
> +#endif
>
> static inline bool arch_validate_prot(unsigned long prot, unsigned long addr)
> {
> - if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
> - return false;
> - if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
> + if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM))
> return false;
> return true;
> }
> diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
> index 3424381b81da..2fd528ef48e0 100644
> --- a/arch/powerpc/include/asm/nohash/64/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
> @@ -82,8 +82,6 @@
> */
> #include <asm/nohash/pte-book3e.h>
>
> -#define _PAGE_SAO 0
> -
> #define PTE_RPN_MASK (~((1UL << PTE_RPN_SHIFT) - 1))
>
> /*
> 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},
> {"cache-inhibited-large-page", feat_enable_large_ci, 0},
> {"coprocessor-icswx", feat_enable, 0},
> {"hypervisor-virtualization-interrupt", feat_enable_hvi, 0},
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
> index 0124003e60d0..14b6abdc3bd8 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -232,8 +232,6 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags)
> rflags |= HPTE_R_I;
> else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT)
> rflags |= (HPTE_R_I | HPTE_R_G);
> - else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO)
> - rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M);
> else
> /*
> * Add memory coherence if cache inhibited is not set
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 86adc71a972f..bdcaae914120 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -316,8 +316,6 @@ extern unsigned int kobjsize(const void *objp);
>
> #if defined(CONFIG_X86)
> # define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
> -#elif defined(CONFIG_PPC)
> -# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
> #elif defined(CONFIG_PARISC)
> # define VM_GROWSUP VM_ARCH_1
> #elif defined(CONFIG_IA64)
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index 5fb752034386..939092dbcb8b 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -114,8 +114,6 @@ IF_HAVE_PG_IDLE(PG_idle, "idle" )
>
> #if defined(CONFIG_X86)
> #define __VM_ARCH_SPECIFIC_1 {VM_PAT, "pat" }
> -#elif defined(CONFIG_PPC)
> -#define __VM_ARCH_SPECIFIC_1 {VM_SAO, "sao" }
> #elif defined(CONFIG_PARISC) || defined(CONFIG_IA64)
> #define __VM_ARCH_SPECIFIC_1 {VM_GROWSUP, "growsup" }
> #elif !defined(CONFIG_MMU)
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 18c5d005bd01..b225b0e16111 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2452,10 +2452,6 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
> if (vma_is_dax(vma))
> return 0;
>
> -#ifdef VM_SAO
> - if (*vm_flags & VM_SAO)
> - return 0;
> -#endif
> #ifdef VM_SPARC_ADI
> if (*vm_flags & VM_SPARC_ADI)
> return 0;
> diff --git a/tools/testing/selftests/powerpc/mm/.gitignore b/tools/testing/selftests/powerpc/mm/.gitignore
> index 2ca523255b1b..ff296c94f627 100644
> --- a/tools/testing/selftests/powerpc/mm/.gitignore
> +++ b/tools/testing/selftests/powerpc/mm/.gitignore
> @@ -2,7 +2,6 @@
> hugetlb_vs_thp_test
> subpage_prot
> tempfile
> -prot_sao
> segv_errors
> wild_bctr
> large_vm_fork_separation
> diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
> index b9103c4bb414..9b8a7b3069c5 100644
> --- a/tools/testing/selftests/powerpc/mm/Makefile
> +++ b/tools/testing/selftests/powerpc/mm/Makefile
> @@ -2,7 +2,7 @@
> noarg:
> $(MAKE) -C ../
>
> -TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
> +TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot segv_errors wild_bctr \
> large_vm_fork_separation bad_accesses
> TEST_GEN_PROGS_EXTENDED := tlbie_test
> TEST_GEN_FILES := tempfile
> @@ -12,8 +12,6 @@ include ../../lib.mk
>
> $(TEST_GEN_PROGS): ../harness.c
>
> -$(OUTPUT)/prot_sao: ../utils.c
> -
> $(OUTPUT)/wild_bctr: CFLAGS += -m64
> $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
> $(OUTPUT)/bad_accesses: CFLAGS += -m64
> diff --git a/tools/testing/selftests/powerpc/mm/prot_sao.c b/tools/testing/selftests/powerpc/mm/prot_sao.c
> deleted file mode 100644
> index e2eed65b7735..000000000000
> --- a/tools/testing/selftests/powerpc/mm/prot_sao.c
> +++ /dev/null
> @@ -1,42 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright 2016, Michael Ellerman, IBM Corp.
> - */
> -
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <sys/mman.h>
> -
> -#include <asm/cputable.h>
> -
> -#include "utils.h"
> -
> -#define SIZE (64 * 1024)
> -
> -int test_prot_sao(void)
> -{
> - char *p;
> -
> - /* 2.06 or later should support SAO */
> - SKIP_IF(!have_hwcap(PPC_FEATURE_ARCH_2_06));
> -
> - /*
> - * Ensure we can ask for PROT_SAO.
> - * We can't really verify that it does the right thing, but at least we
> - * confirm the kernel will accept it.
> - */
> - p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE | PROT_SAO,
> - MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> - FAIL_IF(p == MAP_FAILED);
> -
> - /* Write to the mapping, to at least cause a fault */
> - memset(p, 0xaa, SIZE);
> -
> - return 0;
> -}
> -
> -int main(void)
> -{
> - return test_harness(test_prot_sao, "prot-sao");
> -}
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-08-17 19:14 ` Shawn Anastasio
@ 2020-08-18 7:11 ` Nicholas Piggin
2020-08-18 20:59 ` Shawn Anastasio
0 siblings, 1 reply; 8+ messages in thread
From: Nicholas Piggin @ 2020-08-18 7:11 UTC (permalink / raw)
To: linuxppc-dev, Shawn Anastasio
Excerpts from Shawn Anastasio's message of August 18, 2020 5:14 am:
> I'm a bit concerned about the removal of PROT_SAO.
>
> From what I can see, a feature like this would be extremely useful for
> emulating architectures with stronger memory models. QEMU's multi-
> threaded TCG project in particular looks like it would be a good
> candidate, since as far as I'm aware it is currently completely
> unable to perform strong-on-weak emulation.
>
> Without hardware support like SAO provides, the only way I could see
> to achieve this would be by emitting tons of unnecessary and costly
> memory barrier instructions.
>
> I understand that ISA 3.1 and POWER10 have dropped SAO, but as a POWER9
> user it seems a bit silly to have a potentially useful feature dropped
> from the kernel just because a future processor doesn't support it.
>
> Curious to hear more thoughts on this.
Very reasonable point.
The problem we're trying to get a handle on is live partition migration
where a running guest might be using SAO then get migrated to a P10. I
don't think we have a good way to handle this case. Potentially the
hypervisor could revoke the page tables if the guest is running in hash
mode and the guest kernel could be taught about that and sigbus the
process, but in radix the guest controls those page tables and the SAO
state and I don't think there's a way to cause it to take a fault.
I also don't know what the proprietary hypervisor does here.
We could add it back, default to n, or make it bare metal only, or
somehow try to block live migration to a later CPU without the faciliy.
I wouldn't be against that.
It would be very interesting to know how it performs in such a "real"
situation. I don't know how well POWER9 has optimised it -- it's
possible that it's not much better than putting lwsync after every load
or store.
Thanks,
Nick
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-08-18 7:11 ` Nicholas Piggin
@ 2020-08-18 20:59 ` Shawn Anastasio
2020-08-20 1:05 ` Nicholas Piggin
0 siblings, 1 reply; 8+ messages in thread
From: Shawn Anastasio @ 2020-08-18 20:59 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
On 8/18/20 2:11 AM, Nicholas Piggin wrote> Very reasonable point.
>
> The problem we're trying to get a handle on is live partition migration
> where a running guest might be using SAO then get migrated to a P10. I
> don't think we have a good way to handle this case. Potentially the
> hypervisor could revoke the page tables if the guest is running in hash
> mode and the guest kernel could be taught about that and sigbus the
> process, but in radix the guest controls those page tables and the SAO
> state and I don't think there's a way to cause it to take a fault.
>
> I also don't know what the proprietary hypervisor does here.
>
> We could add it back, default to n, or make it bare metal only, or
> somehow try to block live migration to a later CPU without the faciliy.
> I wouldn't be against that.
Admittedly I'm not too familiar with the specifics of live migration
or guest memory management, but restoring the functionality and adding
a way to prevent migration of SAO-using guests seems like a reasonable
choice to me. Would this be done with help from the guest using some
sort of infrastructure to signal to the hypervisor that SAO is in use,
or entirely on the hypervisor by e.g. scanning the through the process
table for SAO pages?
> It would be very interesting to know how it performs in such a "real"
> situation. I don't know how well POWER9 has optimised it -- it's
> possible that it's not much better than putting lwsync after every load
> or store.
This is definitely worth investigating in depth. That said, even if the
performance on P9 isn't super great, I think the feature could still be
useful, since it would offer more granularity than the sledgehammer
approach of emitting lwsync everywhere.
I'd be happy to put in some of the work required to get this to a point
where it can be reintroduced without breaking guest migration - I'd just
need some pointers on getting started with whatever approach is decided on.
Thanks,
Shawn
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
2020-08-18 20:59 ` Shawn Anastasio
@ 2020-08-20 1:05 ` Nicholas Piggin
0 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-08-20 1:05 UTC (permalink / raw)
To: linuxppc-dev, Shawn Anastasio
Excerpts from Shawn Anastasio's message of August 19, 2020 6:59 am:
> On 8/18/20 2:11 AM, Nicholas Piggin wrote> Very reasonable point.
>>
>> The problem we're trying to get a handle on is live partition migration
>> where a running guest might be using SAO then get migrated to a P10. I
>> don't think we have a good way to handle this case. Potentially the
>> hypervisor could revoke the page tables if the guest is running in hash
>> mode and the guest kernel could be taught about that and sigbus the
>> process, but in radix the guest controls those page tables and the SAO
>> state and I don't think there's a way to cause it to take a fault.
>>
>> I also don't know what the proprietary hypervisor does here.
>>
>> We could add it back, default to n, or make it bare metal only, or
>> somehow try to block live migration to a later CPU without the faciliy.
>> I wouldn't be against that.
>
>
> Admittedly I'm not too familiar with the specifics of live migration
> or guest memory management, but restoring the functionality and adding
> a way to prevent migration of SAO-using guests seems like a reasonable
> choice to me. Would this be done with help from the guest using some
> sort of infrastructure to signal to the hypervisor that SAO is in use,
> or entirely on the hypervisor by e.g. scanning the through the process
> table for SAO pages?
The first step might be to just re-add the functionality but disable
it by default if firmware_has_feature(FW_FEATURE_LPAR). You could have
a config or boot option to allow guests to use it at the cost of
migration compatibility.
That would probably be good enough for experimenting with the feature.
I think modifying the hypervisor and/or guest to deal with migration
is probably too much work to be justified at the moment.
>> It would be very interesting to know how it performs in such a "real"
>> situation. I don't know how well POWER9 has optimised it -- it's
>> possible that it's not much better than putting lwsync after every load
>> or store.
>
>
> This is definitely worth investigating in depth. That said, even if the
> performance on P9 isn't super great, I think the feature could still be
> useful, since it would offer more granularity than the sledgehammer
> approach of emitting lwsync everywhere.
Sure, we'd be interested to hear of results.
> I'd be happy to put in some of the work required to get this to a point
> where it can be reintroduced without breaking guest migration - I'd just
> need some pointers on getting started with whatever approach is decided on.
I think re-adding it as I said above would be okay. The code itself is
not complex so that was not the reason for removal.
Thanks,
Nick
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-20 1:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-07 12:02 [PATCH 1/2] powerpc/64s: remove PROT_SAO support Nicholas Piggin
2020-06-07 12:02 ` [PATCH 2/2] powerpc/64s/hash: disable subpage_prot syscall by default Nicholas Piggin
2020-06-12 6:14 ` [PATCH 1/2] powerpc/64s: remove PROT_SAO support Michael Ellerman
2020-06-29 4:50 ` Nicholas Piggin
2020-08-17 19:14 ` Shawn Anastasio
2020-08-18 7:11 ` Nicholas Piggin
2020-08-18 20:59 ` Shawn Anastasio
2020-08-20 1:05 ` Nicholas Piggin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.