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

[Nicholas Piggin <npiggin@gmail.com>]

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