From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
mpe@ellerman.id.au, mikey@neuling.org
Cc: apopple@linux.ibm.com, paulus@samba.org, npiggin@gmail.com,
naveen.n.rao@linux.vnet.ibm.com, peterz@infradead.org,
jolsa@kernel.org, oleg@redhat.com, fweisbec@gmail.com,
mingo@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 12/16] powerpc/watchpoint: Use builtin ALIGN*() macros
Date: Tue, 14 Apr 2020 07:48:33 +0200 [thread overview]
Message-ID: <a2d7abe8-44e0-9780-6603-00f16300c2ca@c-s.fr> (raw)
In-Reply-To: <20200414031659.58875-13-ravi.bangoria@linux.ibm.com>
Le 14/04/2020 à 05:16, Ravi Bangoria a écrit :
> Currently we calculate hw aligned start and end addresses manually.
> Replace them with builtin ALIGN_DOWN() and ALIGN() macros.
>
> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++--
> arch/powerpc/kernel/hw_breakpoint.c | 12 ++++++------
> arch/powerpc/kernel/process.c | 8 ++++----
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
> 4 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index d472b2eb757e..add5aa076919 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -34,10 +34,11 @@ struct arch_hw_breakpoint {
> #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
> HW_BRK_TYPE_HYP)
>
> +/* Minimum granularity */
> #ifdef CONFIG_PPC_8xx
> -#define HW_BREAKPOINT_ALIGN 0x3
> +#define HW_BREAKPOINT_SIZE 0x4
> #else
> -#define HW_BREAKPOINT_ALIGN 0x7
> +#define HW_BREAKPOINT_SIZE 0x8
> #endif
>
> #define DABR_MAX_LEN 8
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 319a761b7412..02ffd14f4519 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -145,10 +145,10 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
> * <---8 bytes--->
> *
> * In this case, we should configure hw as:
> - * start_addr = address & ~HW_BREAKPOINT_ALIGN
> + * start_addr = address & ~(HW_BREAKPOINT_SIZE - 1)
> * len = 16 bytes
> *
> - * @start_addr and @end_addr are inclusive.
> + * @start_addr is inclusive but @end_addr is exclusive.
> */
> static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
> {
> @@ -156,14 +156,14 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
> u16 hw_len;
> unsigned long start_addr, end_addr;
>
> - start_addr = hw->address & ~HW_BREAKPOINT_ALIGN;
> - end_addr = (hw->address + hw->len - 1) | HW_BREAKPOINT_ALIGN;
> - hw_len = end_addr - start_addr + 1;
> + start_addr = ALIGN_DOWN(hw->address, HW_BREAKPOINT_SIZE);
> + end_addr = ALIGN(hw->address + hw->len, HW_BREAKPOINT_SIZE);
> + hw_len = end_addr - start_addr;
>
> if (dawr_enabled()) {
> max_len = DAWR_MAX_LEN;
> /* DAWR region can't cross 512 bytes boundary */
> - if ((start_addr >> 9) != (end_addr >> 9))
> + if ((start_addr >> 9) != ((end_addr - 1) >> 9))
What about:
if (ALIGN(start_addr, SZ_512M) != ALIGN(end - 1, SZ_512M))
> return -EINVAL;
> } else if (IS_ENABLED(CONFIG_PPC_8xx)) {
> /* 8xx can setup a range without limitation */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index aab82ab80dfa..06679adac447 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -800,12 +800,12 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
> unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
> LCTRL1_CRWF_RW;
> unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
> - unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
> - unsigned long end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
> + unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
> + unsigned long end_addr = ALIGN(brk->address + brk->len, HW_BREAKPOINT_SIZE);
>
> if (start_addr == 0)
> lctrl2 |= LCTRL2_LW0LA_F;
> - else if (end_addr == ~0U)
> + else if (end_addr - 1 == ~0U)
What about:
else if (end_addr == 0)
> lctrl2 |= LCTRL2_LW0LA_E;
> else
> lctrl2 |= LCTRL2_LW0LA_EandF;
> @@ -821,7 +821,7 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
> lctrl1 |= LCTRL1_CRWE_WO | LCTRL1_CRWF_WO;
>
> mtspr(SPRN_CMPE, start_addr - 1);
> - mtspr(SPRN_CMPF, end_addr + 1);
> + mtspr(SPRN_CMPF, end_addr);
> mtspr(SPRN_LCTRL1, lctrl1);
> mtspr(SPRN_LCTRL2, lctrl2);
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 08cb8c1b504c..697c7e4b5877 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -216,7 +216,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
> if ((unsigned long)bp_info->addr >= TASK_SIZE)
> return -EIO;
>
> - brk.address = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> + brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
> brk.type = HW_BRK_TYPE_TRANSLATE;
> brk.len = DABR_MAX_LEN;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
>
Christophe
next prev parent reply other threads:[~2020-04-14 5:48 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-14 3:16 [PATCH v3 00/16] powerpc/watchpoint: Preparation for more than one watchpoint Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 01/16] powerpc/watchpoint: Rename current DAWR macros Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 02/16] powerpc/watchpoint: Add SPRN macros for second DAWR Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 03/16] powerpc/watchpoint: Introduce function to get nr watchpoints dynamically Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 04/16] powerpc/watchpoint/ptrace: Return actual num of available watchpoints Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 05/16] powerpc/watchpoint: Provide DAWR number to set_dawr Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 06/16] powerpc/watchpoint: Provide DAWR number to __set_breakpoint Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 07/16] powerpc/watchpoint: Get watchpoint count dynamically while disabling them Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 08/16] powerpc/watchpoint: Disable all available watchpoints when !dawr_force_enable Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 09/16] powerpc/watchpoint: Convert thread_struct->hw_brk to an array Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 10/16] powerpc/watchpoint: Use loop for thread_struct->ptrace_bps Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 11/16] powerpc/watchpoint: Introduce is_ptrace_bp() function Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 12/16] powerpc/watchpoint: Use builtin ALIGN*() macros Ravi Bangoria
2020-04-14 5:48 ` Christophe Leroy [this message]
2020-04-24 3:24 ` Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint Ravi Bangoria
2020-04-14 5:53 ` Christophe Leroy
2020-04-24 3:32 ` Ravi Bangoria
2020-04-24 5:48 ` Christophe Leroy
2020-04-14 3:16 ` [PATCH v3 14/16] powerpc/watchpoint: Don't allow concurrent perf and ptrace events Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 15/16] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting Ravi Bangoria
2020-04-14 3:16 ` [PATCH v3 16/16] powerpc/watchpoint/xmon: Support 2nd dawr Ravi Bangoria
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=a2d7abe8-44e0-9780-6603-00f16300c2ca@c-s.fr \
--to=christophe.leroy@c-s.fr \
--cc=apopple@linux.ibm.com \
--cc=fweisbec@gmail.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mikey@neuling.org \
--cc=mingo@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=naveen.n.rao@linux.vnet.ibm.com \
--cc=npiggin@gmail.com \
--cc=oleg@redhat.com \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@linux.ibm.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).