Re: [PATCH v8 23/30] powerpc: Add prefixed instructions to instruction data type

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

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
>> For powerpc64, redefine the ppc_inst type so both word and prefixed
>> instructions can be represented. On powerpc32 the type will remain the
>> same.  Update places which had assumed instructions to be 4 bytes long.

...

>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index c0a35e4586a5..217897927926 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -105,11 +105,49 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
>>   #define __put_user_inatomic(x, ptr) \
>>   	__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>   
>> +#ifdef __powerpc64__
>
> Replace by CONFIG_PPC64
>
>> +#define __get_user_instr(x, ptr)			\
>> +({							\
>> +	long __gui_ret = 0;				\
>> +	unsigned long __gui_ptr = (unsigned long)ptr;	\
>> +	struct ppc_inst __gui_inst;			\
>> +	unsigned int prefix, suffix;			\
>> +	__gui_ret = __get_user(prefix, (unsigned int __user *)__gui_ptr);	\
>
> __get_user() can be costly especially with KUAP. I think you should 
> perform a 64 bits read and fallback on a 32 bits read only if the 64 
> bits read failed.

I worry that might break something.

It _should_ be safe, but I'm paranoid.

If we think the KUAP overhead is a problem then I think we'd be better
off pulling the KUAP disable/enable into this macro.

>> diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
>> index 2bd2b752de4f..a8238eff3a31 100644
>> --- a/arch/powerpc/lib/feature-fixups.c
>> +++ b/arch/powerpc/lib/feature-fixups.c
>> @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value, struct fixup_entry *fcur)
>>   	src = alt_start;
>>   	dest = start;
>>   
>> -	for (; src < alt_end; src++, dest++) {
>> +	for (; src < alt_end; src = (void *)src + ppc_inst_len(ppc_inst_read(src)),
>> +	     (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
>
> Can we do this outside the for() for readability ?

I have an idea for cleaning these up, will post it as a follow-up to the series.

>> diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
>> index 08dedd927268..eb6f9ee28ac6 100644
>> --- a/arch/powerpc/lib/inst.c
>> +++ b/arch/powerpc/lib/inst.c
>> @@ -3,9 +3,49 @@
>>    *  Copyright 2020, IBM Corporation.
>>    */
>>   
>> +#include <asm/ppc-opcode.h>
>>   #include <linux/uaccess.h>
>>   #include <asm/inst.h>
>>   
>> +#ifdef __powerpc64__
>> +int probe_user_read_inst(struct ppc_inst *inst,
>> +			 struct ppc_inst *nip)
>> +{
>> +	unsigned int val, suffix;
>> +	int err;
>> +
>> +	err = probe_user_read(&val, nip, sizeof(val));
>
> A user read is costly with KUAP. Can we do a 64 bits read and perform a 
> 32 bits read only when 64 bits read fails ?

Same comment as above.


cheers