Re: [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr

From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
	mpe@ellerman.id.au, mikey@neuling.org
Cc: apopple@linux.ibm.com, paulus@samba.org, npiggin@gmail.com,
	naveen.n.rao@linux.vnet.ibm.com, peterz@infradead.org,
	jolsa@kernel.org, oleg@redhat.com, fweisbec@gmail.com,
	mingo@kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr
Date: Tue, 17 Mar 2020 12:14:10 +0100	[thread overview]
Message-ID: <dabb2823-783a-6a3f-4f04-f3200a1086fc@c-s.fr> (raw)
In-Reply-To: <20200309085806.155823-16-ravi.bangoria@linux.ibm.com>

Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
> Add support for 2nd DAWR in xmon. With this, we can have two
> simultaneous breakpoints from xmon.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
>   1 file changed, 69 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index ac18fe3e4295..20adc83404c8 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -110,7 +110,7 @@ struct bpt {
>   
>   #define NBPTS	256
>   static struct bpt bpts[NBPTS];
> -static struct bpt dabr;
> +static struct bpt dabr[HBP_NUM_MAX];
>   static struct bpt *iabr;
>   static unsigned bpinstr = 0x7fe00008;	/* trap */
>   
> @@ -786,10 +786,17 @@ static int xmon_sstep(struct pt_regs *regs)
>   
>   static int xmon_break_match(struct pt_regs *regs)
>   {
> +	int i;
> +
>   	if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
>   		return 0;
> -	if (dabr.enabled == 0)
> -		return 0;
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (dabr[i].enabled)
> +			goto found;
> +	}
> +	return 0;
> +
> +found:
>   	xmon_core(regs, 0);
>   	return 1;
>   }
> @@ -928,13 +935,16 @@ static void insert_bpts(void)
>   
>   static void insert_cpu_bpts(void)
>   {
> +	int i;
>   	struct arch_hw_breakpoint brk;
>   
> -	if (dabr.enabled) {
> -		brk.address = dabr.address;
> -		brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> -		brk.len = DABR_MAX_LEN;
> -		__set_breakpoint(&brk, 0);
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (dabr[i].enabled) {
> +			brk.address = dabr[i].address;
> +			brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> +			brk.len = 8;
> +			__set_breakpoint(&brk, i);
> +		}
>   	}
>   
>   	if (iabr)
> @@ -1348,6 +1358,35 @@ static long check_bp_loc(unsigned long addr)
>   	return 1;
>   }
>   
> +static int free_data_bpt(void)

This names suggests the function frees a breakpoint.
I guess it should be find_free_data_bpt()

> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!dabr[i].enabled)
> +			return i;
> +	}
> +	printf("Couldn't find free breakpoint register\n");
> +	return -1;
> +}
> +
> +static void print_data_bpts(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_wp_slots(); i++) {
> +		if (!dabr[i].enabled)
> +			continue;
> +
> +		printf("   data   "REG"  [", dabr[i].address);
> +		if (dabr[i].enabled & 1)
> +			printf("r");
> +		if (dabr[i].enabled & 2)
> +			printf("w");
> +		printf("]\n");
> +	}
> +}
> +
>   static char *breakpoint_help_string =
>       "Breakpoint command usage:\n"
>       "b                show breakpoints\n"
> @@ -1381,10 +1420,9 @@ bpt_cmds(void)
>   			printf("Hardware data breakpoint not supported on this cpu\n");
>   			break;
>   		}
> -		if (dabr.enabled) {
> -			printf("Couldn't find free breakpoint register\n");
> +		i = free_data_bpt();
> +		if (i < 0)
>   			break;
> -		}
>   		mode = 7;
>   		cmd = inchar();
>   		if (cmd == 'r')
> @@ -1393,15 +1431,15 @@ bpt_cmds(void)
>   			mode = 6;
>   		else
>   			termch = cmd;
> -		dabr.address = 0;
> -		dabr.enabled = 0;
> -		if (scanhex(&dabr.address)) {
> -			if (!is_kernel_addr(dabr.address)) {
> +		dabr[i].address = 0;
> +		dabr[i].enabled = 0;
> +		if (scanhex(&dabr[i].address)) {
> +			if (!is_kernel_addr(dabr[i].address)) {
>   				printf(badaddr);
>   				break;
>   			}
> -			dabr.address &= ~HW_BRK_TYPE_DABR;
> -			dabr.enabled = mode | BP_DABR;
> +			dabr[i].address &= ~HW_BRK_TYPE_DABR;
> +			dabr[i].enabled = mode | BP_DABR;
>   		}
>   
>   		force_enable_xmon();
> @@ -1440,7 +1478,9 @@ bpt_cmds(void)
>   			for (i = 0; i < NBPTS; ++i)
>   				bpts[i].enabled = 0;
>   			iabr = NULL;
> -			dabr.enabled = 0;
> +			for (i = 0; i < nr_wp_slots(); i++)
> +				dabr[i].enabled = 0;
> +
>   			printf("All breakpoints cleared\n");
>   			break;
>   		}
> @@ -1474,14 +1514,7 @@ bpt_cmds(void)
>   		if (xmon_is_ro || !scanhex(&a)) {
>   			/* print all breakpoints */
>   			printf("   type            address\n");
> -			if (dabr.enabled) {
> -				printf("   data   "REG"  [", dabr.address);
> -				if (dabr.enabled & 1)
> -					printf("r");
> -				if (dabr.enabled & 2)
> -					printf("w");
> -				printf("]\n");
> -			}
> +			print_data_bpts();
>   			for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
>   				if (!bp->enabled)
>   					continue;
> @@ -1941,8 +1974,13 @@ static void dump_207_sprs(void)
>   
>   	printf("hfscr  = %.16lx  dhdes = %.16lx rpr    = %.16lx\n",
>   		mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
> -	printf("dawr   = %.16lx  dawrx = %.16lx ciabr  = %.16lx\n",
> -		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
> +	printf("dawr0  = %.16lx dawrx0 = %.16lx\n",
> +		mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
> +	if (nr_wp_slots() > 1) {
> +		printf("dawr1  = %.16lx dawrx1 = %.16lx\n",
> +			mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
> +	}
> +	printf("ciabr  = %.16lx\n", mfspr(SPRN_CIABR));
>   #endif
>   }
>   
> @@ -3862,10 +3900,9 @@ static void clear_all_bpt(void)
>   		bpts[i].enabled = 0;
>   
>   	/* Clear any data or iabr breakpoints */
> -	if (iabr || dabr.enabled) {
> -		iabr = NULL;
> -		dabr.enabled = 0;
> -	}
> +	iabr = NULL;
> +	for (i = 0; i < nr_wp_slots(); i++)
> +		dabr[i].enabled = 0;
>   }
>   
>   #ifdef CONFIG_DEBUG_FS
> 

Christophe