Re: [PATCH v5 03/13] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64

From: Balamuruhan S <bala24@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@c-s.fr>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	mikey@neuling.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 03/13] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Date: Mon, 30 Mar 2020 19:01:56 +0530	[thread overview]
Message-ID: <16b3ca310e6f5efb1ba1380e3ff80a1beea5a267.camel@linux.ibm.com> (raw)
In-Reply-To: <af38b87a7e1e3efe4f9b664eaeb029e6e7d69fdb.1582848567.git.christophe.leroy@c-s.fr>

On Fri, 2020-02-28 at 00:14 +0000, Christophe Leroy wrote:
> 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   |  2 ++
>  arch/powerpc/kernel/ptrace/ptrace.c | 18 +++---------------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ptrace.h
> b/arch/powerpc/include/asm/ptrace.h
> index ee3ada66deb5..8e1953d99353 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -276,6 +276,8 @@ static inline unsigned long
> regs_get_kernel_stack_nth(struct pt_regs *regs,
>  #endif /* __ASSEMBLY__ */
>  
>  #ifndef __powerpc64__
> +/* We need PT_SOFTE defined at all time to avoid #ifdefs */
> +#define PT_SOFTE PT_MQ
>  #else /* __powerpc64__ */
>  #define PT_FPSCR32 (PT_FPR0 + 2*32 + 1)	/* each FP reg occupies 2 32-
> bit userspace slots */
>  #define PT_VR0_32 164	/* each Vector reg occupies 4 slots in 32-bit
> */
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c
> b/arch/powerpc/kernel/ptrace/ptrace.c
> index 7ed54dbb2d7e..3dd94c296ac7 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -274,17 +274,15 @@ int ptrace_get_reg(struct task_struct *task, int regno,
> unsigned long *data)
>  	if (regno == PT_DSCR)
>  		return get_user_dscr(task, data);
>  
> -#ifdef CONFIG_PPC64
>  	/*
>  	 * softe copies paca->irq_soft_mask variable state. Since irq_soft_mask
> is
>  	 * no more used as a flag, lets force usr to alway see the softe value
> as 1
>  	 * which means interrupts are not soft disabled.
>  	 */
> -	if (regno == PT_SOFTE) {
> +	if (IS_ENABLED(CONFIG_PPC64) && regno == PT_SOFTE) {
>  		*data = 1;
>  		return  0;
>  	}
> -#endif
>  
>  	regs_max = sizeof(struct user_pt_regs) / sizeof(unsigned long);
>  	if (regno < regs_max) {
> @@ -1998,7 +1996,6 @@ static const struct user_regset_view
> user_ppc_native_view = {
>  	.regsets = native_regsets, .n = ARRAY_SIZE(native_regsets)
>  };
>  
> -#ifdef CONFIG_PPC64

should we care for this ? 

/*
 * These are the regset flavors matching the CONFIG_PPC32 native set.
 */
static const struct user_regset compat_regsets[] = {
        [REGSET_GPR] = {
                .core_note_type = NT_PRSTATUS, .n = ELF_NGREG,
                .size = sizeof(compat_long_t), .align = sizeof(compat_long_t),
                .get = gpr32_get, .set = gpr32_set
        },
        [REGSET_FPR] = {
                .core_note_type = NT_PRFPREG, .n = ELF_NFPREG,
                .size = sizeof(double), .align = sizeof(double),
                .get = fpr_get, .set = fpr_set
        },

>  #include <linux/compat.h>

can we move it to head if we do not need the ifdef ?

rest looks good to me.

-- Bala

>  
>  static int gpr32_get_common(struct task_struct *target,
> @@ -2272,14 +2269,11 @@ static const struct user_regset_view
> user_ppc_compat_view = {
>  	.name = "ppc", .e_machine = EM_PPC, .ei_osabi = ELF_OSABI,
>  	.regsets = compat_regsets, .n = ARRAY_SIZE(compat_regsets)
>  };
> -#endif	/* CONFIG_PPC64 */
>  
>  const struct user_regset_view *task_user_regset_view(struct task_struct
> *task)
>  {
> -#ifdef CONFIG_PPC64
> -	if (test_tsk_thread_flag(task, TIF_32BIT))
> +	if (IS_ENABLED(CONFIG_PPC64) && test_tsk_thread_flag(task, TIF_32BIT))
>  		return &user_ppc_compat_view;
> -#endif
>  	return &user_ppc_native_view;
>  }
>  
> @@ -3063,11 +3057,7 @@ long arch_ptrace(struct task_struct *child, long
> request,
>  		else
>  			dbginfo.num_data_bps = 0;
>  		dbginfo.num_condition_regs = 0;
> -#ifdef CONFIG_PPC64
> -		dbginfo.data_bp_alignment = 8;
> -#else
> -		dbginfo.data_bp_alignment = 4;
> -#endif
> +		dbginfo.data_bp_alignment = sizeof(long);
>  		dbginfo.sizeof_condition = 0;
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
> @@ -3304,12 +3294,10 @@ long do_syscall_trace_enter(struct pt_regs *regs)
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>  		trace_sys_enter(regs, regs->gpr[0]);
>  
> -#ifdef CONFIG_PPC64
>  	if (!is_32bit_task())
>  		audit_syscall_entry(regs->gpr[0], regs->gpr[3], regs->gpr[4],
>  				    regs->gpr[5], regs->gpr[6]);
>  	else
> -#endif
>  		audit_syscall_entry(regs->gpr[0],
>  				    regs->gpr[3] & 0xffffffff,
>  				    regs->gpr[4] & 0xffffffff,