Re: [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
On Wed, Feb 26, 2020 at 6:04 PM Nicholas Piggin wrote: > > Jordan Niethe's on February 26, 2020 2:07 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(). > > > > Signed-off-by: Jordan Niethe > > --- > > arch/powerpc/xmon/xmon.c | 13 ++--- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > > index 897e512c6379..a673cf55641c 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)); > > @@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a) > > for (bp = bpts; bp < [NBPTS]; ++bp) { > > if (!bp->enabled && atomic_read(>ref_count) == 0) { > > bp->address = a; > > - bp->instr[1] = bpinstr; > > - store_inst(>instr[1]); > > + patch_instruction(>instr[1], bpinstr); > > return bp; > > } > > } > > @@ -913,7 +907,7 @@ static void insert_bpts(void) > > bp->enabled = 0; > > continue; > > } > > - store_inst(>instr[0]); > > + patch_instruction(>instr[0], bp->instr[0]); > > Hmm that's a bit weird. Can you read instructions into a local variable > first, do the checks on them, then patch them into their execution > location? I agree it is weird, local variables would be better. > > Otherwise, good cleanup. > > Thanks, > Nick
Re: [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
Jordan Niethe's on February 26, 2020 2:07 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(). > > Signed-off-by: Jordan Niethe > --- > arch/powerpc/xmon/xmon.c | 13 ++--- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index 897e512c6379..a673cf55641c 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)); > @@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a) > for (bp = bpts; bp < [NBPTS]; ++bp) { > if (!bp->enabled && atomic_read(>ref_count) == 0) { > bp->address = a; > - bp->instr[1] = bpinstr; > - store_inst(>instr[1]); > + patch_instruction(>instr[1], bpinstr); > return bp; > } > } > @@ -913,7 +907,7 @@ static void insert_bpts(void) > bp->enabled = 0; > continue; > } > - store_inst(>instr[0]); > + patch_instruction(>instr[0], bp->instr[0]); Hmm that's a bit weird. Can you read instructions into a local variable first, do the checks on them, then patch them into their execution location? Otherwise, good cleanup. Thanks, Nick
[PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
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(). Signed-off-by: Jordan Niethe --- arch/powerpc/xmon/xmon.c | 13 ++--- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index 897e512c6379..a673cf55641c 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)); @@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a) for (bp = bpts; bp < [NBPTS]; ++bp) { if (!bp->enabled && atomic_read(>ref_count) == 0) { bp->address = a; - bp->instr[1] = bpinstr; - store_inst(>instr[1]); + patch_instruction(>instr[1], bpinstr); return bp; } } @@ -913,7 +907,7 @@ static void insert_bpts(void) bp->enabled = 0; continue; } - store_inst(>instr[0]); + patch_instruction(>instr[0], bp->instr[0]); if (bp->enabled & BP_CIABR) continue; if (patch_instruction((unsigned int *)bp->address, @@ -923,7 +917,6 @@ static void insert_bpts(void) bp->enabled &= ~BP_TRAP; continue; } - store_inst((void *)bp->address); } } @@ -958,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