From: "liwei (GF)" <liwei391@huawei.com> To: Doug Anderson <dianders@chromium.org> Cc: Daniel Thompson <daniel.thompson@linaro.org>, Jason Wessel <jason.wessel@windriver.com>, Marc Zyngier <maz@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Masami Hiramatsu <mhiramat@kernel.org>, David Miller <davem@davemloft.net>, Will Deacon <will@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Linux ARM <linux-arm-kernel@lists.infradead.org>, LKML <linux-kernel@vger.kernel.org>, <liwei1412@163.com> Subject: Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag() Date: Sat, 16 May 2020 16:47:07 +0800 [thread overview] Message-ID: <d5bb9ccf-6047-13d9-45b3-18421629e83f@huawei.com> (raw) In-Reply-To: <CAD=FV=VVz4QnQ6AWAsCMxw6Zne6es0omvJ--Gnag=PXkMPt42g@mail.gmail.com> Hi Douglas, On 2020/5/14 8:21, Doug Anderson wrote: (SNIP) >> +/* >> + * Interrupts need to be disabled before single-step mode is set, and not >> + * reenabled until after single-step mode ends. >> + * Without disabling interrupt on local CPU, there is a chance of >> + * interrupt occurrence in the period of exception return and start of >> + * out-of-line single-step, that result in wrongly single stepping >> + * into the interrupt handler. >> + */ >> +void kernel_prepare_single_step(unsigned long *flags, >> + struct pt_regs *regs) >> +{ >> + *flags = regs->pstate & DAIF_MASK; >> + regs->pstate |= PSR_I_BIT; >> + /* Unmask PSTATE.D for enabling software step exceptions. */ >> + regs->pstate &= ~PSR_D_BIT; >> +} >> +NOKPROBE_SYMBOL(kernel_prepare_single_step); > > nit: why not just return unsigned long rather than passing by reference? Because i just extract this function from kprobes_save_local_irqflag(), i think return unsigned long is fine. > >> + >> +void kernel_cleanup_single_step(unsigned long flags, >> + struct pt_regs *regs) >> +{ >> + regs->pstate &= ~DAIF_MASK; >> + regs->pstate |= flags; >> +} >> +NOKPROBE_SYMBOL(kernel_cleanup_single_step); >> + >> /* ptrace API */ >> void user_enable_single_step(struct task_struct *task) >> { >> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c >> index d1c95dcf1d78..c655b6b543e3 100644 >> --- a/arch/arm64/kernel/probes/kprobes.c >> +++ b/arch/arm64/kernel/probes/kprobes.c >> @@ -168,30 +168,6 @@ static void __kprobes set_current_kprobe(struct kprobe *p) >> __this_cpu_write(current_kprobe, p); >> } >> >> -/* >> - * Interrupts need to be disabled before single-step mode is set, and not >> - * reenabled until after single-step mode ends. >> - * Without disabling interrupt on local CPU, there is a chance of >> - * interrupt occurrence in the period of exception return and start of >> - * out-of-line single-step, that result in wrongly single stepping >> - * into the interrupt handler. >> - */ >> -static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb, >> - struct pt_regs *regs) >> -{ >> - kcb->saved_irqflag = regs->pstate & DAIF_MASK; >> - regs->pstate |= PSR_I_BIT; >> - /* Unmask PSTATE.D for enabling software step exceptions. */ >> - regs->pstate &= ~PSR_D_BIT; >> -} >> - >> -static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb, >> - struct pt_regs *regs) >> -{ >> - regs->pstate &= ~DAIF_MASK; >> - regs->pstate |= kcb->saved_irqflag; >> -} >> - >> static void __kprobes >> set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr) >> { >> @@ -227,7 +203,7 @@ static void __kprobes setup_singlestep(struct kprobe *p, >> set_ss_context(kcb, slot); /* mark pending ss */ >> >> /* IRQs and single stepping do not mix well. */ >> - kprobes_save_local_irqflag(kcb, regs); >> + kernel_prepare_single_step(&kcb->saved_irqflag, regs); > > Is there some reason to have two functions? It seems like every time > you call kernel_enable_single_step() you'd want to call > kernel_prepare_single_step(). ...and every time you call > kernel_disable_single_step() you'd want to call > kernel_cleanup_single_step(). > > Maybe you can just add the flags parameter to > kernel_enable_single_step() / kernel_disable_single_step() and put the > code in there? > As kernel_enable_single_step() / kernel_disable_single_step() are also called in breakpoint_handler() and watchpoint_handler(), i am not sure it's a right thing to put the daif flag prepare/cleanup into them, especially we don't have a context to save the flags. Thanks, Wei
WARNING: multiple messages have this Message-ID (diff)
From: "liwei (GF)" <liwei391@huawei.com> To: Doug Anderson <dianders@chromium.org> Cc: Mark Rutland <mark.rutland@arm.com>, Daniel Thompson <daniel.thompson@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Marc Zyngier <maz@kernel.org>, LKML <linux-kernel@vger.kernel.org>, liwei1412@163.com, Masami Hiramatsu <mhiramat@kernel.org>, Jason Wessel <jason.wessel@windriver.com>, Will Deacon <will@kernel.org>, David Miller <davem@davemloft.net>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag() Date: Sat, 16 May 2020 16:47:07 +0800 [thread overview] Message-ID: <d5bb9ccf-6047-13d9-45b3-18421629e83f@huawei.com> (raw) In-Reply-To: <CAD=FV=VVz4QnQ6AWAsCMxw6Zne6es0omvJ--Gnag=PXkMPt42g@mail.gmail.com> Hi Douglas, On 2020/5/14 8:21, Doug Anderson wrote: (SNIP) >> +/* >> + * Interrupts need to be disabled before single-step mode is set, and not >> + * reenabled until after single-step mode ends. >> + * Without disabling interrupt on local CPU, there is a chance of >> + * interrupt occurrence in the period of exception return and start of >> + * out-of-line single-step, that result in wrongly single stepping >> + * into the interrupt handler. >> + */ >> +void kernel_prepare_single_step(unsigned long *flags, >> + struct pt_regs *regs) >> +{ >> + *flags = regs->pstate & DAIF_MASK; >> + regs->pstate |= PSR_I_BIT; >> + /* Unmask PSTATE.D for enabling software step exceptions. */ >> + regs->pstate &= ~PSR_D_BIT; >> +} >> +NOKPROBE_SYMBOL(kernel_prepare_single_step); > > nit: why not just return unsigned long rather than passing by reference? Because i just extract this function from kprobes_save_local_irqflag(), i think return unsigned long is fine. > >> + >> +void kernel_cleanup_single_step(unsigned long flags, >> + struct pt_regs *regs) >> +{ >> + regs->pstate &= ~DAIF_MASK; >> + regs->pstate |= flags; >> +} >> +NOKPROBE_SYMBOL(kernel_cleanup_single_step); >> + >> /* ptrace API */ >> void user_enable_single_step(struct task_struct *task) >> { >> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c >> index d1c95dcf1d78..c655b6b543e3 100644 >> --- a/arch/arm64/kernel/probes/kprobes.c >> +++ b/arch/arm64/kernel/probes/kprobes.c >> @@ -168,30 +168,6 @@ static void __kprobes set_current_kprobe(struct kprobe *p) >> __this_cpu_write(current_kprobe, p); >> } >> >> -/* >> - * Interrupts need to be disabled before single-step mode is set, and not >> - * reenabled until after single-step mode ends. >> - * Without disabling interrupt on local CPU, there is a chance of >> - * interrupt occurrence in the period of exception return and start of >> - * out-of-line single-step, that result in wrongly single stepping >> - * into the interrupt handler. >> - */ >> -static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb, >> - struct pt_regs *regs) >> -{ >> - kcb->saved_irqflag = regs->pstate & DAIF_MASK; >> - regs->pstate |= PSR_I_BIT; >> - /* Unmask PSTATE.D for enabling software step exceptions. */ >> - regs->pstate &= ~PSR_D_BIT; >> -} >> - >> -static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb, >> - struct pt_regs *regs) >> -{ >> - regs->pstate &= ~DAIF_MASK; >> - regs->pstate |= kcb->saved_irqflag; >> -} >> - >> static void __kprobes >> set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr) >> { >> @@ -227,7 +203,7 @@ static void __kprobes setup_singlestep(struct kprobe *p, >> set_ss_context(kcb, slot); /* mark pending ss */ >> >> /* IRQs and single stepping do not mix well. */ >> - kprobes_save_local_irqflag(kcb, regs); >> + kernel_prepare_single_step(&kcb->saved_irqflag, regs); > > Is there some reason to have two functions? It seems like every time > you call kernel_enable_single_step() you'd want to call > kernel_prepare_single_step(). ...and every time you call > kernel_disable_single_step() you'd want to call > kernel_cleanup_single_step(). > > Maybe you can just add the flags parameter to > kernel_enable_single_step() / kernel_disable_single_step() and put the > code in there? > As kernel_enable_single_step() / kernel_disable_single_step() are also called in breakpoint_handler() and watchpoint_handler(), i am not sure it's a right thing to put the daif flag prepare/cleanup into them, especially we don't have a context to save the flags. Thanks, Wei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-16 8:47 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-09 21:41 [PATCH 0/4] arm64: kgdb/kdb: Fix single-step debugging issues Wei Li 2020-05-09 21:41 ` Wei Li 2020-05-09 21:41 ` [PATCH 1/4] arm64: kgdb: Fix single-step exception handling oops Wei Li 2020-05-09 21:41 ` Wei Li 2020-05-14 0:21 ` Doug Anderson 2020-05-14 0:21 ` Doug Anderson 2020-05-09 21:41 ` [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag() Wei Li 2020-05-09 21:41 ` Wei Li 2020-05-10 8:59 ` Masami Hiramatsu 2020-05-10 8:59 ` Masami Hiramatsu 2020-05-14 0:21 ` Doug Anderson 2020-05-14 0:21 ` Doug Anderson 2020-05-16 8:47 ` liwei (GF) [this message] 2020-05-16 8:47 ` liwei (GF) 2020-05-16 16:17 ` Doug Anderson 2020-05-16 16:17 ` Doug Anderson 2020-05-18 15:14 ` Masami Hiramatsu 2020-05-18 15:14 ` Masami Hiramatsu 2020-05-09 21:41 ` [PATCH 3/4] arm64: kgdb: Fix single-stepping into the irq handler wrongly Wei Li 2020-05-09 21:41 ` Wei Li 2020-05-14 0:21 ` Doug Anderson 2020-05-14 0:21 ` Doug Anderson 2020-05-09 21:41 ` [PATCH 4/4] arm64: kgdb: Set PSTATE.SS to 1 to reenable single-step Wei Li 2020-05-09 21:41 ` Wei Li 2020-05-14 0:23 ` Doug Anderson 2020-05-14 0:23 ` Doug Anderson 2020-05-16 8:20 ` liwei (GF) 2020-05-16 8:20 ` liwei (GF) 2020-05-14 0:34 ` [PATCH 0/4] arm64: kgdb/kdb: Fix single-step debugging issues Doug Anderson 2020-05-14 0:34 ` Doug Anderson 2020-05-16 8:20 ` liwei (GF) 2020-05-16 8:20 ` liwei (GF) 2020-06-29 21:20 ` Doug Anderson 2020-06-29 21:20 ` Doug Anderson 2020-06-30 7:22 ` Will Deacon 2020-06-30 7:22 ` Will Deacon 2020-07-06 21:37 ` Doug Anderson 2020-07-06 21:37 ` Doug Anderson 2020-07-08 22:06 ` Will Deacon 2020-07-08 22:06 ` Will Deacon 2020-07-08 22:22 ` Doug Anderson 2020-07-08 22:22 ` Doug Anderson 2020-07-07 1:37 ` liwei (GF) 2020-07-07 1:37 ` liwei (GF) 2020-07-08 22:02 ` Will Deacon 2020-07-08 22:02 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d5bb9ccf-6047-13d9-45b3-18421629e83f@huawei.com \ --to=liwei391@huawei.com \ --cc=catalin.marinas@arm.com \ --cc=daniel.thompson@linaro.org \ --cc=davem@davemloft.net \ --cc=dianders@chromium.org \ --cc=jason.wessel@windriver.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=liwei1412@163.com \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=mhiramat@kernel.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.