From: Ravi Bangoria <ravi.bangoria@linux.ibm.com> To: Christophe Leroy <christophe.leroy@c-s.fr> Cc: mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org, mikey@neuling.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, npiggin@gmail.com, naveen.n.rao@linux.vnet.ibm.com, Ravi Bangoria <ravi.bangoria@linux.ibm.com> Subject: Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target Date: Wed, 19 Jun 2019 12:21:37 +0530 [thread overview] Message-ID: <1f3873b7-d924-61ad-2f0e-f6cc12c012ea@linux.ibm.com> (raw) In-Reply-To: <3390c3c2-8290-da55-a183-872c593c7b1e@c-s.fr> On 6/18/19 12:16 PM, Christophe Leroy wrote: >> +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */ >> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) >> +{ >> + u16 length_max = 8; >> + u16 final_len; > > You should be more consistent in naming. If one is called final_len, the other one should be called max_len. Copy/paste :). Will change it. > >> + unsigned long start_addr, end_addr; >> + >> + final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr); >> + >> + if (dawr_enabled()) { >> + length_max = 512; >> + /* DAWR region can't cross 512 bytes boundary */ >> + if ((start_addr >> 9) != (end_addr >> 9)) >> + return -EINVAL; >> + } >> + >> + if (final_len > length_max) >> + return -EINVAL; >> + >> + return 0; >> +} >> + > > Is many places, we have those numeric 512 and 9 shift. Could we replace them by some symbol, for instance DAWR_SIZE and DAWR_SHIFT ? I don't see any other place where we check for boundary limit. [...] > >> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk, >> + unsigned long *start_addr, >> + unsigned long *end_addr) >> +{ >> + *start_addr = brk->address & ~HW_BREAKPOINT_ALIGN; >> + *end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN; >> + return *end_addr - *start_addr + 1; >> +} > > This function gives horrible code (a couple of unneeded store/re-read and read/re-read). > > 000006bc <hw_breakpoint_get_final_len>: > 6bc: 81 23 00 00 lwz r9,0(r3) > 6c0: 55 29 00 38 rlwinm r9,r9,0,0,28 > 6c4: 91 24 00 00 stw r9,0(r4) > 6c8: 81 43 00 00 lwz r10,0(r3) > 6cc: a1 23 00 06 lhz r9,6(r3) > 6d0: 38 6a ff ff addi r3,r10,-1 > 6d4: 7c 63 4a 14 add r3,r3,r9 > 6d8: 60 63 00 07 ori r3,r3,7 > 6dc: 90 65 00 00 stw r3,0(r5) > 6e0: 38 63 00 01 addi r3,r3,1 > 6e4: 81 24 00 00 lwz r9,0(r4) > 6e8: 7c 69 18 50 subf r3,r9,r3 > 6ec: 54 63 04 3e clrlwi r3,r3,16 > 6f0: 4e 80 00 20 blr > > Below code gives something better: > > u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk, > unsigned long *start_addr, > unsigned long *end_addr) > { > unsigned long address = brk->address; > unsigned long len = brk->len; > unsigned long start = address & ~HW_BREAKPOINT_ALIGN; > unsigned long end = (address + len - 1) | HW_BREAKPOINT_ALIGN; > > *start_addr = start; > *end_addr = end; > return end - start + 1; > } > > 000006bc <hw_breakpoint_get_final_len>: > 6bc: 81 43 00 00 lwz r10,0(r3) > 6c0: a1 03 00 06 lhz r8,6(r3) > 6c4: 39 2a ff ff addi r9,r10,-1 > 6c8: 7d 28 4a 14 add r9,r8,r9 > 6cc: 55 4a 00 38 rlwinm r10,r10,0,0,28 > 6d0: 61 29 00 07 ori r9,r9,7 > 6d4: 91 44 00 00 stw r10,0(r4) > 6d8: 20 6a 00 01 subfic r3,r10,1 > 6dc: 91 25 00 00 stw r9,0(r5) > 6e0: 7c 63 4a 14 add r3,r3,r9 > 6e4: 54 63 04 3e clrlwi r3,r3,16 > 6e8: 4e 80 00 20 blr > > > And regardless, that's a pitty to have this function using pointers which are from local variables in the callers, as we loose the benefit of registers. Couldn't this function go in the .h as a static inline ? I'm sure the result would be worth it. This is obviously a bit of optimization, but I like Mikey's idea of storing start_addr and end_addr in the arch_hw_breakpoint. That way we don't have to recalculate length every time in set_dawr.
WARNING: multiple messages have this Message-ID (diff)
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com> To: Christophe Leroy <christophe.leroy@c-s.fr> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>, mikey@neuling.org, linux-kernel@vger.kernel.org, npiggin@gmail.com, paulus@samba.org, naveen.n.rao@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target Date: Wed, 19 Jun 2019 12:21:37 +0530 [thread overview] Message-ID: <1f3873b7-d924-61ad-2f0e-f6cc12c012ea@linux.ibm.com> (raw) In-Reply-To: <3390c3c2-8290-da55-a183-872c593c7b1e@c-s.fr> On 6/18/19 12:16 PM, Christophe Leroy wrote: >> +/* Maximum len for DABR is 8 bytes and DAWR is 512 bytes */ >> +static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) >> +{ >> + u16 length_max = 8; >> + u16 final_len; > > You should be more consistent in naming. If one is called final_len, the other one should be called max_len. Copy/paste :). Will change it. > >> + unsigned long start_addr, end_addr; >> + >> + final_len = hw_breakpoint_get_final_len(hw, &start_addr, &end_addr); >> + >> + if (dawr_enabled()) { >> + length_max = 512; >> + /* DAWR region can't cross 512 bytes boundary */ >> + if ((start_addr >> 9) != (end_addr >> 9)) >> + return -EINVAL; >> + } >> + >> + if (final_len > length_max) >> + return -EINVAL; >> + >> + return 0; >> +} >> + > > Is many places, we have those numeric 512 and 9 shift. Could we replace them by some symbol, for instance DAWR_SIZE and DAWR_SHIFT ? I don't see any other place where we check for boundary limit. [...] > >> +u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk, >> + unsigned long *start_addr, >> + unsigned long *end_addr) >> +{ >> + *start_addr = brk->address & ~HW_BREAKPOINT_ALIGN; >> + *end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN; >> + return *end_addr - *start_addr + 1; >> +} > > This function gives horrible code (a couple of unneeded store/re-read and read/re-read). > > 000006bc <hw_breakpoint_get_final_len>: > 6bc: 81 23 00 00 lwz r9,0(r3) > 6c0: 55 29 00 38 rlwinm r9,r9,0,0,28 > 6c4: 91 24 00 00 stw r9,0(r4) > 6c8: 81 43 00 00 lwz r10,0(r3) > 6cc: a1 23 00 06 lhz r9,6(r3) > 6d0: 38 6a ff ff addi r3,r10,-1 > 6d4: 7c 63 4a 14 add r3,r3,r9 > 6d8: 60 63 00 07 ori r3,r3,7 > 6dc: 90 65 00 00 stw r3,0(r5) > 6e0: 38 63 00 01 addi r3,r3,1 > 6e4: 81 24 00 00 lwz r9,0(r4) > 6e8: 7c 69 18 50 subf r3,r9,r3 > 6ec: 54 63 04 3e clrlwi r3,r3,16 > 6f0: 4e 80 00 20 blr > > Below code gives something better: > > u16 hw_breakpoint_get_final_len(struct arch_hw_breakpoint *brk, > unsigned long *start_addr, > unsigned long *end_addr) > { > unsigned long address = brk->address; > unsigned long len = brk->len; > unsigned long start = address & ~HW_BREAKPOINT_ALIGN; > unsigned long end = (address + len - 1) | HW_BREAKPOINT_ALIGN; > > *start_addr = start; > *end_addr = end; > return end - start + 1; > } > > 000006bc <hw_breakpoint_get_final_len>: > 6bc: 81 43 00 00 lwz r10,0(r3) > 6c0: a1 03 00 06 lhz r8,6(r3) > 6c4: 39 2a ff ff addi r9,r10,-1 > 6c8: 7d 28 4a 14 add r9,r8,r9 > 6cc: 55 4a 00 38 rlwinm r10,r10,0,0,28 > 6d0: 61 29 00 07 ori r9,r9,7 > 6d4: 91 44 00 00 stw r10,0(r4) > 6d8: 20 6a 00 01 subfic r3,r10,1 > 6dc: 91 25 00 00 stw r9,0(r5) > 6e0: 7c 63 4a 14 add r3,r3,r9 > 6e4: 54 63 04 3e clrlwi r3,r3,16 > 6e8: 4e 80 00 20 blr > > > And regardless, that's a pitty to have this function using pointers which are from local variables in the callers, as we loose the benefit of registers. Couldn't this function go in the .h as a static inline ? I'm sure the result would be worth it. This is obviously a bit of optimization, but I like Mikey's idea of storing start_addr and end_addr in the arch_hw_breakpoint. That way we don't have to recalculate length every time in set_dawr.
next prev parent reply other threads:[~2019-06-19 6:51 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-18 4:27 [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 4:27 ` [PATCH 1/5] Powerpc/hw-breakpoint: Replace stale do_dabr() with do_break() Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 6:02 ` Michael Neuling 2019-06-18 6:02 ` Michael Neuling 2019-06-18 6:14 ` Christophe Leroy 2019-06-18 6:14 ` Christophe Leroy 2019-06-18 4:27 ` [PATCH 2/5] Powerpc/hw-breakpoint: Refactor hw_breakpoint_arch_parse() Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 6:21 ` Christophe Leroy 2019-06-18 6:21 ` Christophe Leroy 2019-06-18 7:10 ` Ravi Bangoria 2019-06-18 7:10 ` Ravi Bangoria 2019-06-18 4:27 ` [PATCH 3/5] Powerpc/hw-breakpoint: Refactor set_dawr() Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 6:11 ` Michael Neuling 2019-06-18 6:11 ` Michael Neuling 2019-06-18 7:13 ` Ravi Bangoria 2019-06-18 7:13 ` Ravi Bangoria 2019-06-18 6:24 ` Christophe Leroy 2019-06-18 6:24 ` Christophe Leroy 2019-06-18 4:27 ` [PATCH 4/5] Powerpc/hw-breakpoint: Optimize disable path Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 6:15 ` Michael Neuling 2019-06-18 6:15 ` Michael Neuling 2019-06-19 6:02 ` Ravi Bangoria 2019-06-19 6:02 ` Ravi Bangoria 2019-06-18 6:31 ` Christophe Leroy 2019-06-18 6:31 ` Christophe Leroy 2019-06-19 6:14 ` Ravi Bangoria 2019-06-19 6:14 ` Ravi Bangoria 2019-06-18 4:27 ` [PATCH 5/5] Powerpc/Watchpoint: Fix length calculation for unaligned target Ravi Bangoria 2019-06-18 4:27 ` Ravi Bangoria 2019-06-18 6:46 ` Christophe Leroy 2019-06-18 6:46 ` Christophe Leroy 2019-06-19 6:51 ` Ravi Bangoria [this message] 2019-06-19 6:51 ` Ravi Bangoria 2019-06-18 13:32 ` Michael Neuling 2019-06-18 13:32 ` Michael Neuling 2019-06-19 7:45 ` Ravi Bangoria 2019-06-19 7:45 ` Ravi Bangoria 2019-06-18 6:01 ` [PATCH 0/5] Powerpc/hw-breakpoint: Fixes plus Code refactor Christophe Leroy 2019-06-18 6:01 ` Christophe Leroy 2019-06-18 6:17 ` Michael Neuling 2019-06-18 6:17 ` Michael Neuling 2019-06-19 7:47 ` Ravi Bangoria 2019-06-19 7:47 ` 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=1f3873b7-d924-61ad-2f0e-f6cc12c012ea@linux.ibm.com \ --to=ravi.bangoria@linux.ibm.com \ --cc=benh@kernel.crashing.org \ --cc=christophe.leroy@c-s.fr \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mikey@neuling.org \ --cc=mpe@ellerman.id.au \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=npiggin@gmail.com \ --cc=paulus@samba.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.