| Message ID | 20200501034220.8982-5-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (54dc28ff5e0b3585224d49a31b53e030342ca5c3) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
I probably would have just folded this change into patch 2 but it looks fine to me. Reviewed-by: Alistair Popple <alistair@popple.id.au> On Friday, 1 May 2020 1:41:56 PM AEST Jordan Niethe wrote: > A modulo operation is used for calculating the current offset from a > breakpoint within the breakpoint table. As instruction lengths are > always a power of 2, this can be replaced with a bitwise 'and'. The > current check for word alignment can be replaced with checking that the > lower 2 bits are not set. > > Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v6: New to series > --- > arch/powerpc/xmon/xmon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index bbfea22f4a96..e122f0c8a044 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -857,8 +857,8 @@ static struct bpt *in_breakpoint_table(unsigned long > nip, unsigned long *offp) off = nip - (unsigned long) bpt_table; > if (off >= sizeof(bpt_table)) > return NULL; > - *offp = off % BPT_SIZE; > - if (*offp != 0 && *offp != 4) > + *offp = off & (BPT_SIZE - 1); > + if (off & 3) > return NULL; > return bpts + (off / BPT_SIZE); > }
Jordan Niethe <jniethe5@gmail.com> writes: > A modulo operation is used for calculating the current offset from a > breakpoint within the breakpoint table. As instruction lengths are > always a power of 2, this can be replaced with a bitwise 'and'. The > current check for word alignment can be replaced with checking that the > lower 2 bits are not set. > > Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v6: New to series > --- > arch/powerpc/xmon/xmon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index bbfea22f4a96..e122f0c8a044 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -857,8 +857,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp) > off = nip - (unsigned long) bpt_table; > if (off >= sizeof(bpt_table)) > return NULL; > - *offp = off % BPT_SIZE; > - if (*offp != 0 && *offp != 4) > + *offp = off & (BPT_SIZE - 1); > + if (off & 3) > return NULL; It would be even better if you didn't hard code the 3 wouldn't it? eg: + *offp = off & (BPT_SIZE - 1); + if (off & (BPT_SIZE - 1)) return NULL; cheers
On Tue, May 5, 2020 at 5:08 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Jordan Niethe <jniethe5@gmail.com> writes: > > A modulo operation is used for calculating the current offset from a > > breakpoint within the breakpoint table. As instruction lengths are > > always a power of 2, this can be replaced with a bitwise 'and'. The > > current check for word alignment can be replaced with checking that the > > lower 2 bits are not set. > > > > Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr> > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v6: New to series > > --- > > arch/powerpc/xmon/xmon.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > > index bbfea22f4a96..e122f0c8a044 100644 > > --- a/arch/powerpc/xmon/xmon.c > > +++ b/arch/powerpc/xmon/xmon.c > > @@ -857,8 +857,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp) > > off = nip - (unsigned long) bpt_table; > > if (off >= sizeof(bpt_table)) > > return NULL; > > - *offp = off % BPT_SIZE; > > - if (*offp != 0 && *offp != 4) > > + *offp = off & (BPT_SIZE - 1); > > + if (off & 3) > > return NULL; > > It would be even better if you didn't hard code the 3 wouldn't it? > The three is just checking word alignment, which I think was the intention of the previous - if (*offp != 0 && *offp != 4) But using BPT_SIZE is is a different calculation. BPT_SIZE == 2 * sizeof(unsigned int) == 8 Which would mean the trap of the breakpoint pair of instructions would return NULL. > eg: > > + *offp = off & (BPT_SIZE - 1); > + if (off & (BPT_SIZE - 1)) > return NULL; > > cheers
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index bbfea22f4a96..e122f0c8a044 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -857,8 +857,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp) off = nip - (unsigned long) bpt_table; if (off >= sizeof(bpt_table)) return NULL; - *offp = off % BPT_SIZE; - if (*offp != 0 && *offp != 4) + *offp = off & (BPT_SIZE - 1); + if (off & 3) return NULL; return bpts + (off / BPT_SIZE); }
A modulo operation is used for calculating the current offset from a breakpoint within the breakpoint table. As instruction lengths are always a power of 2, this can be replaced with a bitwise 'and'. The current check for word alignment can be replaced with checking that the lower 2 bits are not set. Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v6: New to series --- arch/powerpc/xmon/xmon.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)