[PATCH v4 02/16] xmon: Move out-of-line instructions to text section

[Jordan Niethe]

On Mon, Mar 23, 2020 at 5:05 PM Balamuruhan S <bala24 at linux.ibm.com> wrote:
>
> On Fri, 2020-03-20 at 16:17 +1100, Jordan Niethe wrote:
> > 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].
>
> Can we have it in separate commits ?
>         * introduce the array bpt_table[] and make struct bpt::instr a
>                 pointer to the instructions in bpt_table[].
>         * place the array in text section.
Yeah I can split it if that would be clearer.
>
> > Currently we only need
> > the copied instruction followed by a trap, so 2 words per breakpoint.
> >
> > Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> > ---
> > v4: New to series
> > ---
> >  arch/powerpc/kernel/vmlinux.lds.S |  2 +-
> >  arch/powerpc/xmon/xmon.c          | 22 +++++++++++++---------
> >  2 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/vmlinux.lds.S
> > b/arch/powerpc/kernel/vmlinux.lds.S
> > index b4c89a1acebb..e90845b8c300 100644
> > --- a/arch/powerpc/kernel/vmlinux.lds.S
> > +++ b/arch/powerpc/kernel/vmlinux.lds.S
> > @@ -86,7 +86,7 @@ SECTIONS
> >               ALIGN_FUNCTION();
> >  #endif
> >               /* careful! __ftr_alt_* sections need to be close to
> > .text */
> > -             *(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup
> > __ftr_alt_* .ref.text);
> > +             *(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup
> > __ftr_alt_* .ref.text .text.xmon_bpts);
> >  #ifdef CONFIG_PPC64
> >               *(.tramp.ftrace.text);
> >  #endif
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 02e3bd62cab4..7875d1a37770 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -97,7 +97,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;
> > @@ -109,6 +109,7 @@ struct bpt {
> >  #define BP_DABR              4
> >
> >  #define NBPTS        256
> > +#define BPT_WORDS    2
> >  static struct bpt bpts[NBPTS];
> >  static struct bpt dabr;
> >  static struct bpt *iabr;
> > @@ -116,6 +117,8 @@ static unsigned bpinstr = 0x7fe00008;     /* trap
> > */
> >
> >  #define BP_NUM(bp)   ((bp) - bpts + 1)
> >
> > +static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS *
> > BPT_WORDS];
> > +
> >  /* Prototypes */
> >  static int cmds(struct pt_regs *);
> >  static int mread(unsigned long, void *, int);
> > @@ -852,16 +855,16 @@ static struct bpt *at_breakpoint(unsigned long
> > pc)
> >  static struct bpt *in_breakpoint_table(unsigned long nip, unsigned
> > long *offp)
> >  {
> >       unsigned long off;
> > +     unsigned long bp_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]))
> > +     bp_off = off % (sizeof(unsigned int) * BPT_WORDS);
> > +     if (bp_off != 0 && bp_off != 4)
> >               return NULL;
> > -     *offp = off - offsetof(struct bpt, instr[0]);
> > -     return (struct bpt *) (nip - off);
> > +     *offp = bp_off;
> > +     return bpts + ((off - bp_off) / (sizeof(unsigned int) *
> > BPT_WORDS));
>
> `(off - bp_off) / (sizeof(unsigned int) * BPT_WORDS)` seems to be the
> actual breakpoint offset. Can we have something like,
>
> #define NBPTS  256
> #define BPT_WORDS      2
> #define BPT_WORDS_SIZE (sizeof(unsigned int) * BPT_WORDS)
> #define BPT_OFFSET(off, bp_word_off) ((off - bp_word_off) / \                                   BPT_WORDS_SIZE)
> ;
> :::
> :::
> :::
> bp_word_off = off % BPT_WORDS_SIZE;
> if (bp_word_off != 0 && bp_word_off != 4)
>         return NULL;
> *offp = bp_word_off;
> return bpts + BPT_OFFSET(off, bp_word_off);
I do agree this is not very clear int terms of the calculations I
don't really want to introduce some macros to be used just once. I
will try to think of a way to make the calculations look more clear.
>
> -- Bala
>