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

From: Balamuruhan S <bala24@linux.ibm.com>
To: Jordan Niethe <jniethe5@gmail.com>, linuxppc-dev@lists.ozlabs.org
Cc: alistair@popple.id.au, dja@axtens.net, npiggin@gmail.com
Subject: Re: [PATCH v5 02/21] powerpc/xmon: Move out-of-line instructions to text section
Date: Tue, 07 Apr 2020 12:15:28 +0530	[thread overview]
Message-ID: <d86f48e6e36a3fde35024f01996744b20fd577a1.camel@linux.ibm.com> (raw)
In-Reply-To: <20200406080936.7180-3-jniethe5@gmail.com>

On Mon, 2020-04-06 at 18:09 +1000, 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]. Currently we only need
> the copied instruction followed by a trap, so 2 words per breakpoint.

we do 2 things here and we discussed to split it into 2 commits, may be
it is up to your thoughts and leave it to you. But otherwise rest looks
good to me.

-- Bala

> 
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v4: New to series
> v5: - Do not use __section(), use a .space directive in .S file
>     - 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))
> +extern unsigned int bpt_table[NBPTS * BPT_WORDS];
> +
>  /* 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;
> +	if (*offp != 0 && *offp != 4)
>  		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
> 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 */
> +