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

[Christophe Leroy]

Le 24/02/2020 Ã 11:48, Michael Ellerman a ÃcritÂ:
> Christophe Leroy <christophe.leroy@xxxxxx> writes:
> > Drop a bunch of #ifdefs CONFIG_PPC64 that are not vital.
> >
> > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> > ---
> >   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