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

From: Michael Ellerman <mpe@ellerman.id.au>
To: Christophe Leroy <christophe.leroy@c-s.fr>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	mikey@neuling.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Date: Mon, 24 Feb 2020 21:48:26 +1100	[thread overview]
Message-ID: <878sks1csl.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <34af3942cd27f6b5365caae772fb8e0af44763d5.1561735587.git.christophe.leroy@c-s.fr>

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.

>  	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

>  #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.

cheers