From: Michael Ellerman <mpe@ellerman•id.au>
To: Nicholas Piggin <npiggin@gmail•com>, linuxppc-dev@lists•ozlabs.org
Cc: Nicholas Piggin <npiggin@gmail•com>
Subject: Re: [PATCH 1/2] powerpc/64s: remove PROT_SAO support
Date: Fri, 12 Jun 2020 16:14:49 +1000 [thread overview]
Message-ID: <87d064g692.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20200607120209.463501-1-npiggin@gmail.com>
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
next prev parent reply other threads:[~2020-06-12 6:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-06-29 4:50 ` [PATCH 1/2] powerpc/64s: remove PROT_SAO support 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d064g692.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=npiggin@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox