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

[Balamuruhan S <bala24@linux.ibm.com>]

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 it 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.

> 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
> ---
>  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_WOR
DS_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);

-- Bala
>  }
 
 static struct bpt *new_breakpoint(unsigned long a)
@@ -876,7
> +879,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;
 		}
 	}