| Message ID | 20200708045046.135702-2-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/watchpoint: Enable 2nd DAWR on baremetal and powervm | expand |
On Wed, Jul 8, 2020 at 2:53 PM Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > > Milton Miller reported that we are aligning start and end address to > wrong size SZ_512M. It should be SZ_512. Fix that. > > While doing this change I also found a case where ALIGN() comparison > fails. Within a given aligned range, ALIGN() of two addresses does not > match when start address is pointing to the first byte and end address > is pointing to any other byte except the first one. But that's not true > for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range > will always point to the first byte. So use ALIGN_DOWN() instead of > ALIGN(). > > Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros") > Reported-by: Milton Miller <miltonm@us.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > --- > arch/powerpc/kernel/hw_breakpoint.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c > index 0000daf0e1da..031e6defc08e 100644 > --- a/arch/powerpc/kernel/hw_breakpoint.c > +++ b/arch/powerpc/kernel/hw_breakpoint.c > @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) > if (dawr_enabled()) { > max_len = DAWR_MAX_LEN; > /* DAWR region can't cross 512 bytes boundary */ > - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M)) > + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512)) I wonder if you should use end_addr - 1, but rather end_addr. For example: 512 -> 1023, because of the -1, 1024 will now be included in this range meaning 513 bytes? > return -EINVAL; > } else if (IS_ENABLED(CONFIG_PPC_8xx)) { > /* 8xx can setup a range without limitation */ > -- > 2.26.2 >
Le 08/07/2020 à 09:44, Jordan Niethe a écrit : > On Wed, Jul 8, 2020 at 2:53 PM Ravi Bangoria > <ravi.bangoria@linux.ibm.com> wrote: >> >> Milton Miller reported that we are aligning start and end address to >> wrong size SZ_512M. It should be SZ_512. Fix that. >> >> While doing this change I also found a case where ALIGN() comparison >> fails. Within a given aligned range, ALIGN() of two addresses does not >> match when start address is pointing to the first byte and end address >> is pointing to any other byte except the first one. But that's not true >> for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range >> will always point to the first byte. So use ALIGN_DOWN() instead of >> ALIGN(). >> >> Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros") >> Reported-by: Milton Miller <miltonm@us.ibm.com> >> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> >> --- >> arch/powerpc/kernel/hw_breakpoint.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c >> index 0000daf0e1da..031e6defc08e 100644 >> --- a/arch/powerpc/kernel/hw_breakpoint.c >> +++ b/arch/powerpc/kernel/hw_breakpoint.c >> @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) >> if (dawr_enabled()) { >> max_len = DAWR_MAX_LEN; >> /* DAWR region can't cross 512 bytes boundary */ >> - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M)) >> + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512)) > I wonder if you should use end_addr - 1, but rather end_addr. For example: > 512 -> 1023, because of the -1, 1024 will now be included in this > range meaning 513 bytes? end_addr is not part of the range. If you want the range [512;1023], it means addr 512 len 512, that is end_addr = addr + len = 1024 Christophe
On Wed, Jul 8, 2020 at 2:53 PM Ravi Bangoria <ravi.bangoria@linux.ibm.com> wrote: > > Milton Miller reported that we are aligning start and end address to > wrong size SZ_512M. It should be SZ_512. Fix that. > > While doing this change I also found a case where ALIGN() comparison > fails. Within a given aligned range, ALIGN() of two addresses does not > match when start address is pointing to the first byte and end address > is pointing to any other byte except the first one. But that's not true > for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range > will always point to the first byte. So use ALIGN_DOWN() instead of > ALIGN(). > > Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros") > Reported-by: Milton Miller <miltonm@us.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> I tested this with the ptrace-hwbreak selftest. Can confirm without also changing to ALIGN_DOWN() then these tests will fail. Tested-by: Jordan Niethe <jniethe5@gmail.com> > --- > arch/powerpc/kernel/hw_breakpoint.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c > index 0000daf0e1da..031e6defc08e 100644 > --- a/arch/powerpc/kernel/hw_breakpoint.c > +++ b/arch/powerpc/kernel/hw_breakpoint.c > @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) > if (dawr_enabled()) { > max_len = DAWR_MAX_LEN; > /* DAWR region can't cross 512 bytes boundary */ > - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M)) > + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512)) > return -EINVAL; > } else if (IS_ENABLED(CONFIG_PPC_8xx)) { > /* 8xx can setup a range without limitation */ > -- > 2.26.2 >
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c index 0000daf0e1da..031e6defc08e 100644 --- a/arch/powerpc/kernel/hw_breakpoint.c +++ b/arch/powerpc/kernel/hw_breakpoint.c @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw) if (dawr_enabled()) { max_len = DAWR_MAX_LEN; /* DAWR region can't cross 512 bytes boundary */ - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M)) + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512)) return -EINVAL; } else if (IS_ENABLED(CONFIG_PPC_8xx)) { /* 8xx can setup a range without limitation */
Milton Miller reported that we are aligning start and end address to wrong size SZ_512M. It should be SZ_512. Fix that. While doing this change I also found a case where ALIGN() comparison fails. Within a given aligned range, ALIGN() of two addresses does not match when start address is pointing to the first byte and end address is pointing to any other byte except the first one. But that's not true for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range will always point to the first byte. So use ALIGN_DOWN() instead of ALIGN(). Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros") Reported-by: Milton Miller <miltonm@us.ibm.com> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> --- arch/powerpc/kernel/hw_breakpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)