Re: [PATCH v5 02/21] powerpc/xmon: Move out-of-line instructions to text section

[Jordan Niethe <jniethe5@gmail.com>]

On Thu, Apr 9, 2020 at 4:11 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 06/04/2020 à 10:09, Jordan Niethe a écrit :
> > To execute an instruction out of line after a breakpoint, the NIP is set
> > to the address of struct bpt::instr. Here a copy of the instruction that
> > was replaced with a breakpoint is kept, along with a trap so normal flow
> > can be resumed after XOLing. The struct bpt's are located within the
> > data section. This is problematic as the data section may be marked as
> > no execute.
> >
> > Instead of each struct bpt holding the instructions to be XOL'd, make a
> > new array, bpt_table[], with enough space to hold instructions for the
> > number of supported breakpoints. Place this array in the text section.
> > Make struct bpt::instr a pointer to the instructions in bpt_table[]
> > associated with that breakpoint. This association is a simple mapping:
> > bpts[n] -> bpt_table[n * words per breakpoint]. Currently we only need
> > the copied instruction followed by a trap, so 2 words per breakpoint.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > v5: - Do not use __section(), use a .space directive in .S file
>
> I was going to comment to use __section() instead of creating a
> dedicated .S file.
>
> Why did you change that in v5 ?
I noticed with some toolchains I was getting this message:
Warning: setting incorrect section attributes for .text.xmon_bpts
I was talking with mpe about it and he said that the usual way for
doing things like this was with .space in a .S file so I changed to
that way.
>
> >      - Simplify in_breakpoint_table() calculation
> >      - Define BPT_SIZE
> > ---
> >   arch/powerpc/xmon/Makefile    |  2 +-
> >   arch/powerpc/xmon/xmon.c      | 23 +++++++++++++----------
> >   arch/powerpc/xmon/xmon_bpts.S |  8 ++++++++
> >   arch/powerpc/xmon/xmon_bpts.h |  8 ++++++++
> >   4 files changed, 30 insertions(+), 11 deletions(-)
> >   create mode 100644 arch/powerpc/xmon/xmon_bpts.S
> >   create mode 100644 arch/powerpc/xmon/xmon_bpts.h
> >
> > diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile
> > index c3842dbeb1b7..515a13ea6f28 100644
> > --- a/arch/powerpc/xmon/Makefile
> > +++ b/arch/powerpc/xmon/Makefile
> > @@ -21,7 +21,7 @@ endif
> >
> >   ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
> >
> > -obj-y                        += xmon.o nonstdio.o spr_access.o
> > +obj-y                        += xmon.o nonstdio.o spr_access.o xmon_bpts.o
> >
> >   ifdef CONFIG_XMON_DISASSEMBLY
> >   obj-y                       += ppc-dis.o ppc-opc.o
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 02e3bd62cab4..049375206510 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -62,6 +62,7 @@
> >
> >   #include "nonstdio.h"
> >   #include "dis-asm.h"
> > +#include "xmon_bpts.h"
> >
> >   #ifdef CONFIG_SMP
> >   static cpumask_t cpus_in_xmon = CPU_MASK_NONE;
> > @@ -97,7 +98,7 @@ static long *xmon_fault_jmp[NR_CPUS];
> >   /* Breakpoint stuff */
> >   struct bpt {
> >       unsigned long   address;
> > -     unsigned int    instr[2];
> > +     unsigned int    *instr;
> >       atomic_t        ref_count;
> >       int             enabled;
> >       unsigned long   pad;
> > @@ -108,7 +109,6 @@ struct bpt {
> >   #define BP_TRAP             2
> >   #define BP_DABR             4
> >
> > -#define NBPTS        256
> >   static struct bpt bpts[NBPTS];
> >   static struct bpt dabr;
> >   static struct bpt *iabr;
> > @@ -116,6 +116,10 @@ static unsigned bpinstr = 0x7fe00008;    /* trap */
> >
> >   #define BP_NUM(bp)  ((bp) - bpts + 1)
> >
> > +#define BPT_SIZE     (sizeof(unsigned int) * 2)
> > +#define BPT_WORDS    (BPT_SIZE / sizeof(unsigned int))
>
> Wouldn't it make more sense to do the following ? :
>
> #define BPT_WORDS       2
> #define BPT_SIZE        (BPT_WORDS * sizeof(unsigned int))
I defined it like that so when we changed unsigned int -> struct
ppc_inst it would be the correct size whether or not the struct
included a suffix.
Otherwise it would be more straightforward to do it like that.
>
> > +extern unsigned int bpt_table[NBPTS * BPT_WORDS];
>
> Should go in xmon_bpts.h if we keep the definition in xmon_bpts.S
Right.
>
> > +
> >   /* Prototypes */
> >   static int cmds(struct pt_regs *);
> >   static int mread(unsigned long, void *, int);
> > @@ -853,15 +857,13 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> >   {
> >       unsigned long off;
> >
> > -     off = nip - (unsigned long) bpts;
> > -     if (off >= sizeof(bpts))
> > +     off = nip - (unsigned long) bpt_table;
> > +     if (off >= sizeof(bpt_table))
> >               return NULL;
> > -     off %= sizeof(struct bpt);
> > -     if (off != offsetof(struct bpt, instr[0])
> > -         && off != offsetof(struct bpt, instr[1]))
> > +     *offp = off % BPT_SIZE;
>
> Can we use logical operation instead ?
Sure, I was just adapting how it was already but that would probably be clearer.
>
>         *offp = off & (BPT_SIZE - 1);
>
> > +     if (*offp != 0 && *offp != 4)
>
> Could be:
>         if (off & 3)
>
> >               return NULL;
> > -     *offp = off - offsetof(struct bpt, instr[0]);
> > -     return (struct bpt *) (nip - off);
> > +     return bpts + (off / BPT_SIZE);
> >   }
> >
> >   static struct bpt *new_breakpoint(unsigned long a)
> > @@ -876,7 +878,8 @@ static struct bpt *new_breakpoint(unsigned long a)
> >       for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> >               if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> >                       bp->address = a;
> > -                     patch_instruction(&bp->instr[1], bpinstr);
> > +                     bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS);
> > +                     patch_instruction(bp->instr + 1, bpinstr);
> >                       return bp;
> >               }
> >       }
> > diff --git a/arch/powerpc/xmon/xmon_bpts.S b/arch/powerpc/xmon/xmon_bpts.S
> > new file mode 100644
> > index 000000000000..ebb2dbc70ca8
> > --- /dev/null
> > +++ b/arch/powerpc/xmon/xmon_bpts.S
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <asm/ppc_asm.h>
> > +#include <asm/asm-compat.h>
> > +#include "xmon_bpts.h"
> > +
> > +.global bpt_table
> > +bpt_table:
> > +     .space NBPTS * 8
>
> Should use BPT_SIZE instead of raw coding 8.
Right.
>
> > diff --git a/arch/powerpc/xmon/xmon_bpts.h b/arch/powerpc/xmon/xmon_bpts.h
> > new file mode 100644
> > index 000000000000..840e70be7945
> > --- /dev/null
> > +++ b/arch/powerpc/xmon/xmon_bpts.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef XMON_BPTS_H
> > +#define XMON_BPTS_H
> > +
> > +#define NBPTS        256
> > +
> > +#endif /* XMON_BPTS_H */
> > +
> >
>
> I think it would be better to split this patch in two patches:
> 1/ First patch to move breakpoints out of struct bpt into a bpt_table.
> 2/ Second patch to move bpt_table into .text section.
Bala suggested so too, I will separate them next time.
>
> Christophe