by Christophe Leroy

[permalink] [raw]

Subject: Re: [PATCH v5 10/13] powerpc/ptrace: split out ADV_DEBUG_REGS related functions.

Le 24/03/2020 à 07:23, Michael Ellerman a écrit :
> Christophe Leroy <[email protected]> writes:
>> On 03/20/2020 02:12 AM, Michael Ellerman wrote:
>>> Christophe Leroy <[email protected]> writes:
>>>> Move ADV_DEBUG_REGS functions out of ptrace.c, into
>>>> ptrace-adv.c and ptrace-noadv.c
>>>>
>>>> Signed-off-by: Christophe Leroy <[email protected]>
>>>> ---
>>>> v4: Leave hw_breakpoint.h for ptrace.c
>>>> ---
>>>> arch/powerpc/kernel/ptrace/Makefile | 4 +
>>>> arch/powerpc/kernel/ptrace/ptrace-adv.c | 468 ++++++++++++++++
>>>> arch/powerpc/kernel/ptrace/ptrace-decl.h | 5 +
>>>> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 236 ++++++++
>>>> arch/powerpc/kernel/ptrace/ptrace.c | 650 ----------------------
>>>> 5 files changed, 713 insertions(+), 650 deletions(-)
>>>> create mode 100644 arch/powerpc/kernel/ptrace/ptrace-adv.c
>>>> create mode 100644 arch/powerpc/kernel/ptrace/ptrace-noadv.c
>>>
>>> This is somehow breaking the ptrace-hwbreak selftest on Power8:
>>>
>>> test: ptrace-hwbreak
>>> tags: git_version:v5.6-rc6-892-g7a285a6067d6
>>> PTRACE_SET_DEBUGREG, WO, len: 1: Ok
>>> PTRACE_SET_DEBUGREG, WO, len: 2: Ok
>>> PTRACE_SET_DEBUGREG, WO, len: 4: Ok
>>> PTRACE_SET_DEBUGREG, WO, len: 8: Ok
>>> PTRACE_SET_DEBUGREG, RO, len: 1: Ok
>>> PTRACE_SET_DEBUGREG, RO, len: 2: Ok
>>> PTRACE_SET_DEBUGREG, RO, len: 4: Ok
>>> PTRACE_SET_DEBUGREG, RO, len: 8: Ok
>>> PTRACE_SET_DEBUGREG, RW, len: 1: Ok
>>> PTRACE_SET_DEBUGREG, RW, len: 2: Ok
>>> PTRACE_SET_DEBUGREG, RW, len: 4: Ok
>>> PTRACE_SET_DEBUGREG, RW, len: 8: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO, len: 1: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RO, len: 1: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RW, len: 1: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, WO, len: 6: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, RO, len: 6: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, RW, len: 6: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW UNALIGNED, WO, len: 6: Ok
>>> PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW UNALIGNED, RO, len: 6: Fail
>>> failure: ptrace-hwbreak
>>>
>>> I haven't had time to work out why yet.
>>>
>>
>> A (big) part of commit c3f68b0478e7 ("powerpc/watchpoint: Fix ptrace
>> code that muck around with address/len") was lost during rebase.
>>
>> I'll send a fix, up to you to squash it in or commit it as is.
>
> Thanks.
>

One person asked me if I sent the fix already. So yes, it is there:

https://patchwork.ozlabs.org/patch/1259348/

Christophe

2020-03-30 14:02:32

by Balamuruhan S

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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,