Re: [PATCH v4 01/16] powerpc/xmon: Remove store_inst() for patch_instruction()

[Nicholas Piggin <npiggin@gmail.com>]

Jordan Niethe's on March 20, 2020 3:17 pm:
> For modifying instructions in xmon, patch_instruction() can serve the
> same role that store_inst() is performing with the advantage of not
> being specific to xmon. In some places patch_instruction() is already
> being using followed by store_inst(). In these cases just remove the
> store_inst(). Otherwise replace store_inst() with patch_instruction().

LGTM

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v4: Read into a local variable
> ---
>  arch/powerpc/xmon/xmon.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index e8c84d265602..02e3bd62cab4 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -325,11 +325,6 @@ static inline void sync(void)
>  	asm volatile("sync; isync");
>  }
>  
> -static inline void store_inst(void *p)
> -{
> -	asm volatile ("dcbst 0,%0; sync; icbi 0,%0; isync" : : "r" (p));
> -}
> -
>  static inline void cflush(void *p)
>  {
>  	asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p));
> @@ -881,8 +876,7 @@ 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;
> -			bp->instr[1] = bpinstr;
> -			store_inst(&bp->instr[1]);
> +			patch_instruction(&bp->instr[1], bpinstr);
>  			return bp;
>  		}
>  	}
> @@ -894,25 +888,26 @@ static struct bpt *new_breakpoint(unsigned long a)
>  static void insert_bpts(void)
>  {
>  	int i;
> +	unsigned int instr;
>  	struct bpt *bp;
>  
>  	bp = bpts;
>  	for (i = 0; i < NBPTS; ++i, ++bp) {
>  		if ((bp->enabled & (BP_TRAP|BP_CIABR)) == 0)
>  			continue;
> -		if (mread(bp->address, &bp->instr[0], 4) != 4) {
> +		if (mread(bp->address, &instr, 4) != 4) {
>  			printf("Couldn't read instruction at %lx, "
>  			       "disabling breakpoint there\n", bp->address);
>  			bp->enabled = 0;
>  			continue;
>  		}
> -		if (IS_MTMSRD(bp->instr[0]) || IS_RFID(bp->instr[0])) {
> +		if (IS_MTMSRD(instr) || IS_RFID(instr)) {
>  			printf("Breakpoint at %lx is on an mtmsrd or rfid "
>  			       "instruction, disabling it\n", bp->address);
>  			bp->enabled = 0;
>  			continue;
>  		}
> -		store_inst(&bp->instr[0]);
> +		patch_instruction(bp->instr, instr);
>  		if (bp->enabled & BP_CIABR)
>  			continue;
>  		if (patch_instruction((unsigned int *)bp->address,
> @@ -922,7 +917,6 @@ static void insert_bpts(void)
>  			bp->enabled &= ~BP_TRAP;
>  			continue;
>  		}
> -		store_inst((void *)bp->address);
>  	}
>  }
>  
> @@ -957,8 +951,6 @@ static void remove_bpts(void)
>  			(unsigned int *)bp->address, bp->instr[0]) != 0)
>  			printf("Couldn't remove breakpoint at %lx\n",
>  			       bp->address);
> -		else
> -			store_inst((void *)bp->address);
>  	}
>  }
>  
> -- 
> 2.17.1
> 
>