Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs
 CONFIG_PPC64
To:     Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>, mikey@neuling.org
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <cover.1561735587.git.christophe.leroy@c-s.fr>
 <34af3942cd27f6b5365caae772fb8e0af44763d5.1561735587.git.christophe.leroy@c-s.fr>
 <878sks1csl.fsf@mpe.ellerman.id.au>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <66e65eec-ce09-4e7e-aa71-38d19b3d4fee@c-s.fr>
Date:   Wed, 26 Feb 2020 13:06:33 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <878sks1csl.fsf@mpe.ellerman.id.au>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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