Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64

[Christophe Leroy <christophe.leroy@c-s.fr>]

Le 24/02/2020 à 11:48, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Drop a bunch of #ifdefs CONFIG_PPC64 that are not vital.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   arch/powerpc/include/asm/ptrace.h      |  9 ++++-----
>>   arch/powerpc/include/uapi/asm/ptrace.h | 12 ++++--------
>>   arch/powerpc/kernel/ptrace/ptrace.c    | 24 +++---------------------
>>   3 files changed, 11 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index faa5a338ac5a..1506a9c61d50 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -36,11 +36,10 @@ struct pt_regs
>>   			unsigned long link;
>>   			unsigned long xer;
>>   			unsigned long ccr;
>> -#ifdef CONFIG_PPC64
>> -			unsigned long softe;
>> -#else
>> -			unsigned long mq;
>> -#endif
>> +			union {
>> +				unsigned long softe;
>> +				unsigned long mq;
>> +			};
>>   			unsigned long trap;
>>   			unsigned long dar;
>>   			unsigned long dsisr;
>> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
>> index f5f1ccc740fc..37d7befbb8dc 100644
>> --- a/arch/powerpc/include/uapi/asm/ptrace.h
>> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
>> @@ -43,12 +43,11 @@ struct pt_regs
>>   	unsigned long link;
>>   	unsigned long xer;
>>   	unsigned long ccr;
>> -#ifdef __powerpc64__
>> -	unsigned long softe;		/* Soft enabled/disabled */
>> -#else
>> -	unsigned long mq;		/* 601 only (not used at present) */
>> +	union {
>> +		unsigned long softe;	/* Soft enabled/disabled */
>> +		unsigned long mq;	/* 601 only (not used at present) */
>>   					/* Used on APUS to hold IPL value. */
>> -#endif
>> +	};
> 
> As Andreas pointed out this is not safe as this is a uapi header.

Ok, dropped

> 
>>   	unsigned long trap;		/* Reason for being here */
>>   	/* N.B. for critical exceptions on 4xx, the dar and dsisr
>>   	   fields are overloaded to hold srr0 and srr1. */
>> @@ -105,11 +104,8 @@ struct pt_regs
>>   #define PT_LNK	36
>>   #define PT_XER	37
>>   #define PT_CCR	38
>> -#ifndef __powerpc64__
>>   #define PT_MQ	39
>> -#else
>>   #define PT_SOFTE 39
>> -#endif
> 
> I'd also rather leave that as it is.
> 
> There's a slim chance it could break some code that already has either
> of those defined.
> 
> If you need them both defined to make other code work in the kernel
> that's fine, in the kernel header we can do:
> 
> // Ensure these are always defined inside the kernel to avoid #ifdefs
> #ifdef CONFIG_PPC64
> #define PT_MQ	39
> #else
> #define PT_SOFTE 39
> #endif

Ok.

Only the PT_SOFTE is missing, I added the following in the relevant case:

#define PT_SOFTE PT_MQ

> 
> 
>>   #define PT_TRAP	40
>>   #define PT_DAR	41
>>   #define PT_DSISR 42
>> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
>> index 684b0b315c32..0afb223c4d57 100644
>> --- a/arch/powerpc/kernel/ptrace/ptrace.c
>> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
>> @@ -113,11 +113,8 @@ static const struct pt_regs_offset regoffset_table[] = {
>>   	REG_OFFSET_NAME(link),
>>   	REG_OFFSET_NAME(xer),
>>   	REG_OFFSET_NAME(ccr),
>> -#ifdef CONFIG_PPC64
>>   	REG_OFFSET_NAME(softe),
>> -#else
>>   	REG_OFFSET_NAME(mq),
>> -#endif
> 
> Pretty sure that will cause breakage. The offset is ABI.

Ok, dropped

Christophe